Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Jaegeuk Kim]

On 04/10, Matthew Wilcox wrote:
> On Tue, Apr 10, 2018 at 10:45:45PM +0900, Minchan Kim wrote:
> > On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> > > From: Matthew Wilcox 
> > > 
> > > The page cache has used the mapping's GFP flags for allocating
> > > radix tree nodes for a long time.  It took care to always mask off the
> > > __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> > > __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> > > __GFP_DMA32 flags would also have been able to sneak through if they
> > > were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> > > location, and remove it from earlier in the callchain.
> > > 
> > > Fixes: 19f99cee206c ("f2fs: add core inode operations")
> > 
> > Why this patch fix 19f99cee206c instead of 449dd6984d0e?
> > F2FS doesn't have any problem before introducing 449dd6984d0e?
> 
> Well, there's the problem.  This bug is the combination of three different
> things:
> 
> 1. The working set code relying on list_empty.
> 2. The page cache not filtering out the bad flags.
> 3. F2FS specifying a flag nobody had ever specified before.
> 
> So what single patch does this patch fix?  I don't think it really matters.

Hope there'd be someone who does care about patch description though, IMHO,
this fixes the MM regression introduced by:
449dd6984d0e ("mm: keep page cache radix tree nodes in check") merged in v3.15,
2014.

19f99cee206c ("f2fs: add core inode operations) merged in v3.8, 2012, just
revealed this out. In fact, I've never hit this bug in old kernels.

>From the user viewpoint, may I suggest to describe what kind of symptom we're
able to see due to this bug?

Something like:

[ 7858.792946] [] __list_del_entry+0x30/0xd0
[ 7858.792951] [] list_lru_del+0xac/0x1ac
[ 7858.792957] [] page_cache_tree_insert+0xd8/0x110
[ 7858.792962] [] __add_to_page_cache_locked+0xf8/0x4e0
[ 7858.792967] [] add_to_page_cache_lru+0x50/0x1ac
[ 7858.792972] [] pagecache_get_page+0x468/0x57c
[ 7858.792979] [] __get_node_page+0x84/0x764
[ 7858.792986] [] f2fs_iget+0x264/0xdc8
[ 7858.792991] [] f2fs_lookup+0x3b4/0x660
[ 7858.792998] [] lookup_slow+0x1e4/0x348
[ 7858.793003] [] walk_component+0x21c/0x320
[ 7858.793008] [] path_lookupat+0x90/0x1bc
[ 7858.793013] [] filename_lookup+0x8c/0x1a0
[ 7858.793018] [] vfs_fstatat+0x84/0x10c
[ 7858.793023] [] SyS_newfstatat+0x28/0x64

Thanks,

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Jaegeuk Kim]

On 04/10, Matthew Wilcox wrote:
> On Tue, Apr 10, 2018 at 10:45:45PM +0900, Minchan Kim wrote:
> > On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> > > From: Matthew Wilcox 
> > > 
> > > The page cache has used the mapping's GFP flags for allocating
> > > radix tree nodes for a long time.  It took care to always mask off the
> > > __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> > > __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> > > __GFP_DMA32 flags would also have been able to sneak through if they
> > > were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> > > location, and remove it from earlier in the callchain.
> > > 
> > > Fixes: 19f99cee206c ("f2fs: add core inode operations")
> > 
> > Why this patch fix 19f99cee206c instead of 449dd6984d0e?
> > F2FS doesn't have any problem before introducing 449dd6984d0e?
> 
> Well, there's the problem.  This bug is the combination of three different
> things:
> 
> 1. The working set code relying on list_empty.
> 2. The page cache not filtering out the bad flags.
> 3. F2FS specifying a flag nobody had ever specified before.
> 
> So what single patch does this patch fix?  I don't think it really matters.

Hope there'd be someone who does care about patch description though, IMHO,
this fixes the MM regression introduced by:
449dd6984d0e ("mm: keep page cache radix tree nodes in check") merged in v3.15,
2014.

19f99cee206c ("f2fs: add core inode operations) merged in v3.8, 2012, just
revealed this out. In fact, I've never hit this bug in old kernels.

>From the user viewpoint, may I suggest to describe what kind of symptom we're
able to see due to this bug?

Something like:

[ 7858.792946] [] __list_del_entry+0x30/0xd0
[ 7858.792951] [] list_lru_del+0xac/0x1ac
[ 7858.792957] [] page_cache_tree_insert+0xd8/0x110
[ 7858.792962] [] __add_to_page_cache_locked+0xf8/0x4e0
[ 7858.792967] [] add_to_page_cache_lru+0x50/0x1ac
[ 7858.792972] [] pagecache_get_page+0x468/0x57c
[ 7858.792979] [] __get_node_page+0x84/0x764
[ 7858.792986] [] f2fs_iget+0x264/0xdc8
[ 7858.792991] [] f2fs_lookup+0x3b4/0x660
[ 7858.792998] [] lookup_slow+0x1e4/0x348
[ 7858.793003] [] walk_component+0x21c/0x320
[ 7858.793008] [] path_lookupat+0x90/0x1bc
[ 7858.793013] [] filename_lookup+0x8c/0x1a0
[ 7858.793018] [] vfs_fstatat+0x84/0x10c
[ 7858.793023] [] SyS_newfstatat+0x28/0x64

Thanks,

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 10:45:45PM +0900, Minchan Kim wrote:
> On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox 
> > 
> > The page cache has used the mapping's GFP flags for allocating
> > radix tree nodes for a long time.  It took care to always mask off the
> > __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> > __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> > __GFP_DMA32 flags would also have been able to sneak through if they
> > were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> > location, and remove it from earlier in the callchain.
> > 
> > Fixes: 19f99cee206c ("f2fs: add core inode operations")
> 
> Why this patch fix 19f99cee206c instead of 449dd6984d0e?
> F2FS doesn't have any problem before introducing 449dd6984d0e?

Well, there's the problem.  This bug is the combination of three different
things:

1. The working set code relying on list_empty.
2. The page cache not filtering out the bad flags.
3. F2FS specifying a flag nobody had ever specified before.

So what single patch does this patch fix?  I don't think it really matters.

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Matthew Wilcox]

On Tue, Apr 10, 2018 at 10:45:45PM +0900, Minchan Kim wrote:
> On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox 
> > 
> > The page cache has used the mapping's GFP flags for allocating
> > radix tree nodes for a long time.  It took care to always mask off the
> > __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> > __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> > __GFP_DMA32 flags would also have been able to sneak through if they
> > were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> > location, and remove it from earlier in the callchain.
> > 
> > Fixes: 19f99cee206c ("f2fs: add core inode operations")
> 
> Why this patch fix 19f99cee206c instead of 449dd6984d0e?
> F2FS doesn't have any problem before introducing 449dd6984d0e?

Well, there's the problem.  This bug is the combination of three different
things:

1. The working set code relying on list_empty.
2. The page cache not filtering out the bad flags.
3. F2FS specifying a flag nobody had ever specified before.

So what single patch does this patch fix?  I don't think it really matters.

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Jan Kara]

On Tue 10-04-18 05:53:51, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Honza

> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3
> 
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Jan Kara]

On Tue 10-04-18 05:53:51, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Honza

> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3
> 
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Minchan Kim]

On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")

Why this patch fix 19f99cee206c instead of 449dd6984d0e?
F2FS doesn't have any problem before introducing 449dd6984d0e?


> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org
> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3
> 

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Minchan Kim]

On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")

Why this patch fix 19f99cee206c instead of 449dd6984d0e?
F2FS doesn't have any problem before introducing 449dd6984d0e?


> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org
> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3
> 

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Michal Hocko]

On Tue 10-04-18 05:53:51, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

I would push this into __radix_tree_preload...
Anyway
Acked-by: Michal Hocko 

> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Michal Hocko]

On Tue 10-04-18 05:53:51, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.
> 
> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

I would push this into __radix_tree_preload...
Anyway
Acked-by: Michal Hocko 

> ---
>  mm/filemap.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c2147682f4c3..1a4bfc5ed3dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>   VM_BUG_ON_PAGE(!PageLocked(new), new);
>   VM_BUG_ON_PAGE(new->mapping, new);
>  
> - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (!error) {
>   struct address_space *mapping = old->mapping;
>   void (*freepage)(struct page *);
> @@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
>   return error;
>   }
>  
> - error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
> + error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
>   if (error) {
>   if (!huge)
>   mem_cgroup_cancel_charge(page, memcg, false);
> @@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space 
> *mapping, pgoff_t offset,
>   if (fgp_flags & FGP_ACCESSED)
>   __SetPageReferenced(page);
>  
> - err = add_to_page_cache_lru(page, mapping, offset,
> - gfp_mask & GFP_RECLAIM_MASK);
> + err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (unlikely(err)) {
>   put_page(page);
>   page = NULL;
> @@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t 
> offset, gfp_t gfp_mask)
>   if (!page)
>   return -ENOMEM;
>  
> - ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & 
> GFP_KERNEL);
> + ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
>   if (ret == 0)
>   ret = mapping->a_ops->readpage(file, page);
>   else if (ret == -EEXIST)
> -- 
> 2.16.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Johannes Weiner]

On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.

Could you please mention the nullptr crash here, maybe even in the
patch subject? That makes it much easier to find this patch when you
run into that bug or when evaluating backport candidates.

Other than that,

> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Acked-by: Johannes Weiner 

Re: [PATCH 2/2] page cache: Mask off unwanted GFP flags

[Johannes Weiner]

On Tue, Apr 10, 2018 at 05:53:51AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox 
> 
> The page cache has used the mapping's GFP flags for allocating
> radix tree nodes for a long time.  It took care to always mask off the
> __GFP_HIGHMEM flag, and masked off other flags in other paths, but the
> __GFP_ZERO flag was still able to sneak through.  The __GFP_DMA and
> __GFP_DMA32 flags would also have been able to sneak through if they
> were ever used.  Fix them all by using GFP_RECLAIM_MASK at the innermost
> location, and remove it from earlier in the callchain.

Could you please mention the nullptr crash here, maybe even in the
patch subject? That makes it much easier to find this patch when you
run into that bug or when evaluating backport candidates.

Other than that,

> Fixes: 19f99cee206c ("f2fs: add core inode operations")
> Reported-by: Minchan Kim 
> Signed-off-by: Matthew Wilcox 
> Cc: sta...@vger.kernel.org

Acked-by: Johannes Weiner