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

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

Konstantin, what is the expected max size?
Can this allocation for legitimate filesystem become large enough to try vmalloc() ?

On 2023/01/03 17:13, Michal Hocko wrote:
> 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.
>