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

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

[Tetsuo Handa]

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()")

Is KMALLOC_MAX_SIZE intended to be used by callers like

  https://linux.googlesource.com/linux/kernel/git/torvalds/linux/+/a5a1e1f249db4e0a35d3deca0b9916b11cc1f02b%5E!

? 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().

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

[Michal Hocko]

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

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

[Michal Hocko]

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

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

[Tetsuo Handa]

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.
>