* [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
@ 2019-07-02 0:51 Henry Burns
2019-07-02 1:00 ` Shakeel Butt
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Henry Burns @ 2019-07-02 0:51 UTC (permalink / raw)
To: Vitaly Wool, Andrew Morton
Cc: Vitaly Vul, Mike Rapoport, Xidong Wang, Shakeel Butt,
Jonathan Adams, linux-mm, linux-kernel, Henry Burns
__SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
lock the page. Following zsmalloc.c's example we call trylock_page() and
unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
passed in locked, as documentation.
Signed-off-by: Henry Burns <henryburns@google.com>
Suggested-by: Vitaly Wool <vitalywool@gmail.com>
---
Changelog since v1:
- Added an if statement around WARN_ON(trylock_page(page)) to avoid
unlocking a page locked by a someone else.
mm/z3fold.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index e174d1549734..6341435b9610 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
set_bit(PAGE_HEADLESS, &page->private);
goto headless;
}
- __SetPageMovable(page, pool->inode->i_mapping);
+ if (!WARN_ON(!trylock_page(page))) {
+ __SetPageMovable(page, pool->inode->i_mapping);
+ unlock_page(page);
+ }
z3fold_page_lock(zhdr);
found:
@@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
VM_BUG_ON_PAGE(!PageMovable(page), page);
VM_BUG_ON_PAGE(!PageIsolated(page), page);
+ VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
zhdr = page_address(page);
pool = zhdr_to_pool(zhdr);
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 0:51 [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() Henry Burns @ 2019-07-02 1:00 ` Shakeel Butt 2019-07-02 1:16 ` Henry Burns 2019-07-02 9:53 ` Vitaly Wool 2019-07-02 18:58 ` David Rientjes 2 siblings, 1 reply; 11+ messages in thread From: Shakeel Butt @ 2019-07-02 1:00 UTC (permalink / raw) To: Henry Burns Cc: Vitaly Wool, Andrew Morton, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote: > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > lock the page. Following zsmalloc.c's example we call trylock_page() and > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > passed in locked, as documentation. > > Signed-off-by: Henry Burns <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > --- > Changelog since v1: > - Added an if statement around WARN_ON(trylock_page(page)) to avoid > unlocking a page locked by a someone else. > > mm/z3fold.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/z3fold.c b/mm/z3fold.c > index e174d1549734..6341435b9610 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > set_bit(PAGE_HEADLESS, &page->private); > goto headless; > } > - __SetPageMovable(page, pool->inode->i_mapping); > + if (!WARN_ON(!trylock_page(page))) { > + __SetPageMovable(page, pool->inode->i_mapping); > + unlock_page(page); > + } Can you please comment why lock_page() is not used here? > z3fold_page_lock(zhdr); > > found: > @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa > > VM_BUG_ON_PAGE(!PageMovable(page), page); > VM_BUG_ON_PAGE(!PageIsolated(page), page); > + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage); > > zhdr = page_address(page); > pool = zhdr_to_pool(zhdr); > -- > 2.22.0.410.gd8fdbe21b5-goog > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 1:00 ` Shakeel Butt @ 2019-07-02 1:16 ` Henry Burns 2019-07-02 21:19 ` Andrew Morton 0 siblings, 1 reply; 11+ messages in thread From: Henry Burns @ 2019-07-02 1:16 UTC (permalink / raw) To: Shakeel Butt Cc: Vitaly Wool, Andrew Morton, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote: > > > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > > lock the page. Following zsmalloc.c's example we call trylock_page() and > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > > passed in locked, as documentation. > > > > Signed-off-by: Henry Burns <henryburns@google.com> > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > --- > > Changelog since v1: > > - Added an if statement around WARN_ON(trylock_page(page)) to avoid > > unlocking a page locked by a someone else. > > > > mm/z3fold.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/mm/z3fold.c b/mm/z3fold.c > > index e174d1549734..6341435b9610 100644 > > --- a/mm/z3fold.c > > +++ b/mm/z3fold.c > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > > set_bit(PAGE_HEADLESS, &page->private); > > goto headless; > > } > > - __SetPageMovable(page, pool->inode->i_mapping); > > + if (!WARN_ON(!trylock_page(page))) { > > + __SetPageMovable(page, pool->inode->i_mapping); > > + unlock_page(page); > > + } > > Can you please comment why lock_page() is not used here? Since z3fold_alloc can be called in atomic or non atomic context, calling lock_page() could trigger a number of warnings about might_sleep() being called in atomic context. WARN_ON should avoid the problem described above as well, and in any weird condition where someone else has the page lock, we can avoid calling __SetPageMovable(). > > > z3fold_page_lock(zhdr); > > > > found: > > @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa > > > > VM_BUG_ON_PAGE(!PageMovable(page), page); > > VM_BUG_ON_PAGE(!PageIsolated(page), page); > > + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage); > > > > zhdr = page_address(page); > > pool = zhdr_to_pool(zhdr); > > -- > > 2.22.0.410.gd8fdbe21b5-goog > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 1:16 ` Henry Burns @ 2019-07-02 21:19 ` Andrew Morton 2019-07-02 22:17 ` Henry Burns 0 siblings, 1 reply; 11+ messages in thread From: Andrew Morton @ 2019-07-02 21:19 UTC (permalink / raw) To: Henry Burns Cc: Shakeel Butt, Vitaly Wool, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> Are these the same person? > Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() > Date: Mon, 1 Jul 2019 18:16:30 -0700 > > On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote: > > > > > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > > > lock the page. Following zsmalloc.c's example we call trylock_page() and > > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > > > passed in locked, as documentation. The changelog still doesn't mention that this bug triggers a VM_BUG_ON_PAGE(). It should do so. I did this: : __SetPageMovable() expects its page to be locked, but z3fold.c doesn't : lock the page. This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page) : in __SetPageMovable(). : : Following zsmalloc.c's example we call trylock_page() and unlock_page(). : Also make z3fold_page_migrate() assert that newpage is passed in locked, : as per the documentation. I'll add a cc:stable to this fix. > > > Signed-off-by: Henry Burns <henryburns@google.com> > > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > > --- > > > Changelog since v1: > > > - Added an if statement around WARN_ON(trylock_page(page)) to avoid > > > unlocking a page locked by a someone else. > > > > > > mm/z3fold.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/z3fold.c b/mm/z3fold.c > > > index e174d1549734..6341435b9610 100644 > > > --- a/mm/z3fold.c > > > +++ b/mm/z3fold.c > > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > > > set_bit(PAGE_HEADLESS, &page->private); > > > goto headless; > > > } > > > - __SetPageMovable(page, pool->inode->i_mapping); > > > + if (!WARN_ON(!trylock_page(page))) { > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > + unlock_page(page); > > > + } > > > > Can you please comment why lock_page() is not used here? Shakeel asked "please comment" (ie, please add a code comment), not "please comment on". Subtle ;) > Since z3fold_alloc can be called in atomic or non atomic context, > calling lock_page() could trigger a number of > warnings about might_sleep() being called in atomic context. WARN_ON > should avoid the problem described > above as well, and in any weird condition where someone else has the > page lock, we can avoid calling > __SetPageMovable(). I think this will suffice: --- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix +++ a/mm/z3fold.c @@ -919,6 +919,9 @@ retry: set_bit(PAGE_HEADLESS, &page->private); goto headless; } + /* + * z3fold_alloc() can be called from atomic contexts, hence the trylock + */ if (!WARN_ON(!trylock_page(page))) { __SetPageMovable(page, pool->inode->i_mapping); unlock_page(page); However this code would be more effective if z3fold_alloc() were to be told when it is running in non-atomic context so it can perform a sleeping lock_page() in that case. That's an improvement to consider for later, please. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 21:19 ` Andrew Morton @ 2019-07-02 22:17 ` Henry Burns 2019-07-02 22:24 ` Andrew Morton 2019-07-03 5:54 ` Vitaly Wool 0 siblings, 2 replies; 11+ messages in thread From: Henry Burns @ 2019-07-02 22:17 UTC (permalink / raw) To: Andrew Morton Cc: Shakeel Butt, Vitaly Wool, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > > > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> > > Are these the same person? I Think it's the same person, but i wasn't sure which email to include because one was in the list of maintainers and I had contacted the other earlier. > > > Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() > > Date: Mon, 1 Jul 2019 18:16:30 -0700 > > > > On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote: > > > > > > > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > > > > lock the page. Following zsmalloc.c's example we call trylock_page() and > > > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > > > > passed in locked, as documentation. > > The changelog still doesn't mention that this bug triggers a > VM_BUG_ON_PAGE(). It should do so. I did this: > > : __SetPageMovable() expects its page to be locked, but z3fold.c doesn't > : lock the page. This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page) > : in __SetPageMovable(). > : > : Following zsmalloc.c's example we call trylock_page() and unlock_page(). > : Also make z3fold_page_migrate() assert that newpage is passed in locked, > : as per the documentation. > > I'll add a cc:stable to this fix. > > > > > Signed-off-by: Henry Burns <henryburns@google.com> > > > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > > > --- > > > > Changelog since v1: > > > > - Added an if statement around WARN_ON(trylock_page(page)) to avoid > > > > unlocking a page locked by a someone else. > > > > > > > > mm/z3fold.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/mm/z3fold.c b/mm/z3fold.c > > > > index e174d1549734..6341435b9610 100644 > > > > --- a/mm/z3fold.c > > > > +++ b/mm/z3fold.c > > > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > > > > set_bit(PAGE_HEADLESS, &page->private); > > > > goto headless; > > > > } > > > > - __SetPageMovable(page, pool->inode->i_mapping); > > > > + if (!WARN_ON(!trylock_page(page))) { > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > + unlock_page(page); > > > > + } > > > > > > Can you please comment why lock_page() is not used here? > > Shakeel asked "please comment" (ie, please add a code comment), not > "please comment on". Subtle ;) > > > Since z3fold_alloc can be called in atomic or non atomic context, > > calling lock_page() could trigger a number of > > warnings about might_sleep() being called in atomic context. WARN_ON > > should avoid the problem described > > above as well, and in any weird condition where someone else has the > > page lock, we can avoid calling > > __SetPageMovable(). > > I think this will suffice: > > --- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix > +++ a/mm/z3fold.c > @@ -919,6 +919,9 @@ retry: > set_bit(PAGE_HEADLESS, &page->private); > goto headless; > } > + /* > + * z3fold_alloc() can be called from atomic contexts, hence the trylock > + */ > if (!WARN_ON(!trylock_page(page))) { > __SetPageMovable(page, pool->inode->i_mapping); > unlock_page(page); > > However this code would be more effective if z3fold_alloc() were to be > told when it is running in non-atomic context so it can perform a > sleeping lock_page() in that case. That's an improvement to consider > for later, please. > z3fold_alloc() can tell when its called in atomic context, new patch incoming! I'm thinking something like this: > > > > + if (can_sleep) { > > > > + lock_page(page); > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > + unlock_page(page); > > > > + } else { > > > > + if (!WARN_ON(!trylock_page(page))) { > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > + unlock_page(page); > > > > + } else { > > > > + pr_err("Newly allocated z3fold page is locked\n"); > > > > + WARN_ON(1); > > > > + } > > > > + } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 22:17 ` Henry Burns @ 2019-07-02 22:24 ` Andrew Morton 2019-07-03 5:53 ` Vitaly Wool 2019-07-03 5:54 ` Vitaly Wool 1 sibling, 1 reply; 11+ messages in thread From: Andrew Morton @ 2019-07-02 22:24 UTC (permalink / raw) To: Henry Burns Cc: Shakeel Butt, Vitaly Wool, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote: > > > > > + if (can_sleep) { > > > > > + lock_page(page); > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > + unlock_page(page); > > > > > + } else { > > > > > + if (!WARN_ON(!trylock_page(page))) { > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > + unlock_page(page); > > > > > + } else { > > > > > + pr_err("Newly allocated z3fold page is locked\n"); > > > > > + WARN_ON(1); The WARN_ON will have already warned in this case. But the whole idea of warning in this case may be undesirable. We KNOW that the warning will sometimes trigger (yes?). So what's the point in scaring users? Also, pr_err(...)+WARN_ON(1) can basically be replaced with WARN(1, ...)? > > > > > + } > > > > > + } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 22:24 ` Andrew Morton @ 2019-07-03 5:53 ` Vitaly Wool 2019-07-03 16:39 ` Henry Burns 0 siblings, 1 reply; 11+ messages in thread From: Vitaly Wool @ 2019-07-03 5:53 UTC (permalink / raw) To: Andrew Morton Cc: Henry Burns, Shakeel Butt, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote: > > > > > > > + if (can_sleep) { > > > > > > + lock_page(page); > > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > > + unlock_page(page); > > > > > > + } else { > > > > > > + if (!WARN_ON(!trylock_page(page))) { > > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > > + unlock_page(page); > > > > > > + } else { > > > > > > + pr_err("Newly allocated z3fold page is locked\n"); > > > > > > + WARN_ON(1); > > The WARN_ON will have already warned in this case. > > But the whole idea of warning in this case may be undesirable. We KNOW > that the warning will sometimes trigger (yes?). So what's the point in > scaring users? Well, normally a newly allocated page that we own should not be locked by someone else so this is worth a warning IMO. With that said, the else branch here appears to be redundant. ~Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-03 5:53 ` Vitaly Wool @ 2019-07-03 16:39 ` Henry Burns 0 siblings, 0 replies; 11+ messages in thread From: Henry Burns @ 2019-07-03 16:39 UTC (permalink / raw) To: Vitaly Wool Cc: Andrew Morton, Shakeel Butt, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Tue, Jul 2, 2019 at 10:54 PM Vitaly Wool <vitalywool@gmail.com> wrote: > > On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote: > > > > > > > > > + if (can_sleep) { > > > > > > > + lock_page(page); > > > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > > > + unlock_page(page); > > > > > > > + } else { > > > > > > > + if (!WARN_ON(!trylock_page(page))) { > > > > > > > + __SetPageMovable(page, pool->inode->i_mapping); > > > > > > > + unlock_page(page); > > > > > > > + } else { > > > > > > > + pr_err("Newly allocated z3fold page is locked\n"); > > > > > > > + WARN_ON(1); > > > > The WARN_ON will have already warned in this case. > > > > But the whole idea of warning in this case may be undesirable. We KNOW > > that the warning will sometimes trigger (yes?). So what's the point in > > scaring users? > > Well, normally a newly allocated page that we own should not be locked > by someone else so this is worth a warning IMO. With that said, the > else branch here appears to be redundant. The else branch has been removed, and I think it's possible (albeit unlikely) that the trylock could fail due to either compaction or kstaled (In which case the page just won't be movable). Also Vitaly, do you have a preference between the two emails? I'm not sure which one to include. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 22:17 ` Henry Burns 2019-07-02 22:24 ` Andrew Morton @ 2019-07-03 5:54 ` Vitaly Wool 1 sibling, 0 replies; 11+ messages in thread From: Vitaly Wool @ 2019-07-03 5:54 UTC (permalink / raw) To: Henry Burns Cc: Andrew Morton, Shakeel Butt, Vitaly Vul, Mike Rapoport, Xidong Wang, Jonathan Adams, Linux MM, LKML On Wed, Jul 3, 2019 at 12:18 AM Henry Burns <henryburns@google.com> wrote: > > On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > > > > > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> > > > > Are these the same person? > I Think it's the same person, but i wasn't sure which email to include > because one was > in the list of maintainers and I had contacted the other earlier. This is the same person, it's the transliteration done differently that caused this :) ~Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 0:51 [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() Henry Burns 2019-07-02 1:00 ` Shakeel Butt @ 2019-07-02 9:53 ` Vitaly Wool 2019-07-02 18:58 ` David Rientjes 2 siblings, 0 replies; 11+ messages in thread From: Vitaly Wool @ 2019-07-02 9:53 UTC (permalink / raw) To: Henry Burns Cc: Andrew Morton, Vitaly Vul, Mike Rapoport, Xidong Wang, Shakeel Butt, Jonathan Adams, Linux-MM, LKML On Tue, Jul 2, 2019 at 3:51 AM Henry Burns <henryburns@google.com> wrote: > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > lock the page. Following zsmalloc.c's example we call trylock_page() and > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > passed in locked, as documentation. > > Signed-off-by: Henry Burns <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> Acked-by: Vitaly Wool <vitalywool@gmail.com> Thanks! > --- > Changelog since v1: > - Added an if statement around WARN_ON(trylock_page(page)) to avoid > unlocking a page locked by a someone else. > > mm/z3fold.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/z3fold.c b/mm/z3fold.c > index e174d1549734..6341435b9610 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, > set_bit(PAGE_HEADLESS, &page->private); > goto headless; > } > - __SetPageMovable(page, pool->inode->i_mapping); > + if (!WARN_ON(!trylock_page(page))) { > + __SetPageMovable(page, pool->inode->i_mapping); > + unlock_page(page); > + } > z3fold_page_lock(zhdr); > > found: > @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa > > VM_BUG_ON_PAGE(!PageMovable(page), page); > VM_BUG_ON_PAGE(!PageIsolated(page), page); > + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage); > > zhdr = page_address(page); > pool = zhdr_to_pool(zhdr); > -- > 2.22.0.410.gd8fdbe21b5-goog > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() 2019-07-02 0:51 [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() Henry Burns 2019-07-02 1:00 ` Shakeel Butt 2019-07-02 9:53 ` Vitaly Wool @ 2019-07-02 18:58 ` David Rientjes 2 siblings, 0 replies; 11+ messages in thread From: David Rientjes @ 2019-07-02 18:58 UTC (permalink / raw) To: Henry Burns Cc: Vitaly Wool, Andrew Morton, Vitaly Vul, Mike Rapoport, Xidong Wang, Shakeel Butt, Jonathan Adams, linux-mm, linux-kernel On Mon, 1 Jul 2019, Henry Burns wrote: > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > lock the page. Following zsmalloc.c's example we call trylock_page() and > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > passed in locked, as documentation. > > Signed-off-by: Henry Burns <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-03 16:39 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-02 0:51 [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() Henry Burns 2019-07-02 1:00 ` Shakeel Butt 2019-07-02 1:16 ` Henry Burns 2019-07-02 21:19 ` Andrew Morton 2019-07-02 22:17 ` Henry Burns 2019-07-02 22:24 ` Andrew Morton 2019-07-03 5:53 ` Vitaly Wool 2019-07-03 16:39 ` Henry Burns 2019-07-03 5:54 ` Vitaly Wool 2019-07-02 9:53 ` Vitaly Wool 2019-07-02 18:58 ` David Rientjes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).