Re: [PATCH] fs/ntfs3: Use __GFP_NOWARN allocation at ntfs_load_attr_list()

[Michal Hocko <mhocko@suse.com>]

On Tue 03-01-23 09:01:34, Michal Hocko wrote:
> On Tue 03-01-23 09:49:22, Tetsuo Handa wrote:
> > On 2023/01/03 5:19, Michal Hocko wrote:
> > >> @@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
> > >>  
> > >>  	if (!attr->non_res) {
> > >>  		lsize = le32_to_cpu(attr->res.data_size);
> > >> -		le = kmalloc(al_aligned(lsize), GFP_NOFS);
> > >> +		le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
> > > 
> > > This looks like a bad idea in general. The allocator merely says that
> > > something is wrong and you are silencing that. The calling code should
> > > check the size for reasonable range and if larger size. Moreover, if
> > > lsize can be really more than PAGE_SIZE this should be kvmalloc instead.
> > 
> > There are already similar commits.
> > 
> >   commit 0d0f659bf713 ("fs/ntfs3: Use __GFP_NOWARN allocation at wnd_init()")
> >   commit 59bfd7a483da ("fs/ntfs3: Use __GFP_NOWARN allocation at ntfs_fill_super()")
> 
> Bad examples to follow.
> 
> > Is KMALLOC_MAX_SIZE intended to be used by callers like
> > 
> >   https://linux.googlesource.com/linux/kernel/git/torvalds/linux/+/a5a1e1f249db4e0a35d3deca0b9916b11cc1f02b%5E!
> > 
> > ?
> 
> Nope, this doesn't look right either. This all is about inhibiting the
> warning much more than actually fixing the underlying problem which
> would be either check against a _specification_ based or _reasonable_
> expectation based range or using kvmalloc instead if the range is not
> well defined.

Let me clarify some more because there are two things happening here.

kvmalloc (or its variants) should be used whenever there is a risk the
allocation request size is large (>>PAGE_SIZE) sounds like reasonable
rule of thumb because those allocations shouldn't put an additional
pressure on a fragmented system.

Any user input, and that applies to potentially crafted fs images,
should be checked for runaway values. If there is specification in
place then this is no brainer. If the value is not well specified then
there should be reasonable defensive checking done. KMALLOC_MAX_SIZE
doesn't sound like a good fit for the check as that is an internal
implementation specific constant for a particular memory allocator.
vmalloc allows much larger allocations and sometime that is a reasonable
thing to allow. So those checks should be domain specific.

-- 
Michal Hocko
SUSE Labs