| Message ID | 20211022064748.4173718-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: shmem: fix uninitialized variable use in me_pagecache_clean() | expand |
On Thu, Oct 21, 2021 at 11:47 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > It appears that the has_extra_refcount() is now in the wrong place: > > mm/memory-failure.c:892:6: error: variable 'extra_pins' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (!mapping) { > ^~~~~~~~ > mm/memory-failure.c:915:32: note: uninitialized use occurs here > if (has_extra_refcount(ps, p, extra_pins)) > ^~~~~~~~~~ > mm/memory-failure.c:892:2: note: remove the 'if' if its condition is always false > if (!mapping) { > ^~~~~~~~~~~~~~~ > mm/memory-failure.c:879:6: error: variable 'extra_pins' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (PageAnon(p)) { > ^~~~~~~~~~~ > mm/memory-failure.c:915:32: note: uninitialized use occurs here > if (has_extra_refcount(ps, p, extra_pins)) > ^~~~~~~~~~ > mm/memory-failure.c:879:2: note: remove the 'if' if its condition is always false > if (PageAnon(p)) { > ^~~~~~~~~~~~~~~~~~ > mm/memory-failure.c:871:17: note: initialize the variable 'extra_pins' to silence this warning > bool extra_pins; > ^ > = 0 > > In both of those cases, we already set an error code and don't > need to override that one. Hi Arnd, Thanks for catching this. There has been a fix (https://lore.kernel.org/linux-mm/20211021180336.2328086-1-nathan@kernel.org/). But I think yours makes more sense. It seems better to have the "out:" label after has_extra_refcount(). Andrew, Could you please take this patch? And either keeping or dropping Nathan's patch is fine to me. Thanks. > > Fixes: d882a43a0011 ("mm: shmem: don't truncate page if memory failure happens") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > This is caused by a commit im -mm, so the commit ID is not stable. > If the fix is correct, I'd suggest folding it into the original > change > --- > mm/memory-failure.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 3b04f0361a58..e8c38e27b753 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -909,12 +909,12 @@ static int me_pagecache_clean(struct page_state *ps, struct page *p) > * Open: to take i_rwsem or not for this? Right now we don't. > */ > ret = truncate_error_page(p, page_to_pfn(p), mapping); > -out: > - unlock_page(p); > - > if (has_extra_refcount(ps, p, extra_pins)) > ret = MF_FAILED; > > +out: > + unlock_page(p); > + > return ret; > } > > -- > 2.29.2 >
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 3b04f0361a58..e8c38e27b753 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -909,12 +909,12 @@ static int me_pagecache_clean(struct page_state *ps, struct page *p) * Open: to take i_rwsem or not for this? Right now we don't. */ ret = truncate_error_page(p, page_to_pfn(p), mapping); -out: - unlock_page(p); - if (has_extra_refcount(ps, p, extra_pins)) ret = MF_FAILED; +out: + unlock_page(p); + return ret; }