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

[Michal Hocko <mhocko@suse.com>]

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.

> I think that, unless there is a known upper limit defined by specification,
> checking for overflow and silence like
> 
>   https://lkml.kernel.org/r/6d878e01-6c2f-8766-2578-c95030442369@I-love.SAKURA.ne.jp
> 
> is fine. These input are random values which do not need to succeed by using kvmalloc().

How can you tell the value is just a random noise or a relevant value
that people would actually want to succeede? Answer to that question
gives you a hint on how to address the issue.

-- 
Michal Hocko
SUSE Labs