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@xxxxxxxxxxxxxxxxxxx
>
> 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
