linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
@ 2020-07-03  6:56 Chengguang Xu
  2020-07-03 19:20 ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Chengguang Xu @ 2020-07-03  6:56 UTC (permalink / raw)
  To: hughd, akpm; +Cc: linux-mm, Chengguang Xu

new_attr is allocated with kvmalloc() so should be freed
with kvfree().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 mm/shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index a0dbe62f8042..b2abca3f7f33 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
 		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
 					  GFP_KERNEL);
 		if (!new_xattr->name) {
-			kfree(new_xattr);
+			kvfree(new_xattr);
 			return -ENOMEM;
 		}
 
-- 
2.20.1




^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
  2020-07-03  6:56 [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs() Chengguang Xu
@ 2020-07-03 19:20 ` Andrew Morton
  2020-07-03 20:15   ` Hugh Dickins
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2020-07-03 19:20 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: hughd, linux-mm

On Fri,  3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@mykernel.net> wrote:

> new_attr is allocated with kvmalloc() so should be freed
> with kvfree().
> 
> ...
>
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
>  		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>  					  GFP_KERNEL);
>  		if (!new_xattr->name) {
> -			kfree(new_xattr);
> +			kvfree(new_xattr);
>  			return -ENOMEM;
>  		}

Indeed.  Maybe simple_xattr_alloc() should have been called
simple_xattr_kvmalloc().


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
  2020-07-03 19:20 ` Andrew Morton
@ 2020-07-03 20:15   ` Hugh Dickins
  2020-07-04  1:59     ` cgxu
  0 siblings, 1 reply; 6+ messages in thread
From: Hugh Dickins @ 2020-07-03 20:15 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Andrew Morton, Daniel Xu, hughd, linux-mm

On Fri, 3 Jul 2020, Andrew Morton wrote:
> On Fri,  3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@mykernel.net> wrote:
> 
> > new_attr is allocated with kvmalloc() so should be freed
> > with kvfree().
> > 
> > ...
> >
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
> >  		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
> >  					  GFP_KERNEL);
> >  		if (!new_xattr->name) {
> > -			kfree(new_xattr);
> > +			kvfree(new_xattr);
> >  			return -ENOMEM;
> >  		}
> 
> Indeed.  Maybe simple_xattr_alloc() should have been called
> simple_xattr_kvmalloc().

That would give a better hint, true. But it's been simple_xattr_alloc()
for a while, and "alloc" doesn't claim "kmalloc" or "__get_free_page"
or whatever, so its new users ought to check, and its old users ought
to be updated when a change is made.

This is a
Fixes: fdc85222d58e ("kernfs: kvmalloc xattr value instead of kmalloc")
Cc: stable@vger.kernel.org # v5.7

(Though I hope the restricted use of xattrs on tmpfs cannot actually
get into multi-page allocations.)

It's a good catch, Chengguang, thank you: but isn't your patch
incomplete?  It looks like this omission goes beyond mm/shmem:
include/linux/xattr.h contains a simple_xattrs_free(), used by
both shmem and kernfs, which also says "kfree(xattr)" still.

Please extend and re-subject and re-Cc your fix to cover that
too (and check nothing else has been missed) - thanks.

Hugh


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
  2020-07-03 20:15   ` Hugh Dickins
@ 2020-07-04  1:59     ` cgxu
  2020-07-04  2:20       ` Hugh Dickins
  0 siblings, 1 reply; 6+ messages in thread
From: cgxu @ 2020-07-04  1:59 UTC (permalink / raw)
  To: Hugh Dickins; +Cc: Andrew Morton, Daniel Xu, linux-mm

On 7/4/20 4:15 AM, Hugh Dickins wrote:
> On Fri, 3 Jul 2020, Andrew Morton wrote:
>> On Fri,  3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@mykernel.net> wrote:
>>
>>> new_attr is allocated with kvmalloc() so should be freed
>>> with kvfree().
>>>
>>> ...
>>>
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
>>>   		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
>>>   					  GFP_KERNEL);
>>>   		if (!new_xattr->name) {
>>> -			kfree(new_xattr);
>>> +			kvfree(new_xattr);
>>>   			return -ENOMEM;
>>>   		}
>> Indeed.  Maybe simple_xattr_alloc() should have been called
>> simple_xattr_kvmalloc().
> That would give a better hint, true. But it's been simple_xattr_alloc()
> for a while, and "alloc" doesn't claim "kmalloc" or "__get_free_page"
> or whatever, so its new users ought to check, and its old users ought
> to be updated when a change is made.
>
> This is a
> Fixes: fdc85222d58e ("kernfs: kvmalloc xattr value instead of kmalloc")
> Cc: stable@vger.kernel.org # v5.7
>
> (Though I hope the restricted use of xattrs on tmpfs cannot actually
> get into multi-page allocations.)
>
> It's a good catch, Chengguang, thank you: but isn't your patch
> incomplete?  It looks like this omission goes beyond mm/shmem:
> include/linux/xattr.h contains a simple_xattrs_free(), used by
> both shmem and kernfs, which also says "kfree(xattr)" still.
>
> Please extend and re-subject and re-Cc your fix to cover that
> too (and check nothing else has been missed) - thanks.

Thanks for pointing that out, I overlooked that part. Since this patch
has already merged to Andrew's tree, I would like to make another
patch to handle rest of the fixing and that probably can go into
vfs-tree or others.


Thanks,
cgxu


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
  2020-07-04  1:59     ` cgxu
@ 2020-07-04  2:20       ` Hugh Dickins
  2020-07-04  2:44         ` cgxu
  0 siblings, 1 reply; 6+ messages in thread
From: Hugh Dickins @ 2020-07-04  2:20 UTC (permalink / raw)
  To: cgxu; +Cc: Hugh Dickins, Andrew Morton, Daniel Xu, linux-mm

On Sat, 4 Jul 2020, cgxu wrote:
> On 7/4/20 4:15 AM, Hugh Dickins wrote:
> > On Fri, 3 Jul 2020, Andrew Morton wrote:
> > > On Fri,  3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@mykernel.net>
> > > wrote:
> > > 
> > > > new_attr is allocated with kvmalloc() so should be freed
> > > > with kvfree().
> > > > 
> > > > ...
> > > > 
> > > > --- a/mm/shmem.c
> > > > +++ b/mm/shmem.c
> > > > @@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
> > > >   		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
> > > > len,
> > > >   					  GFP_KERNEL);
> > > >   		if (!new_xattr->name) {
> > > > -			kfree(new_xattr);
> > > > +			kvfree(new_xattr);
> > > >   			return -ENOMEM;
> > > >   		}
> > > Indeed.  Maybe simple_xattr_alloc() should have been called
> > > simple_xattr_kvmalloc().
> > That would give a better hint, true. But it's been simple_xattr_alloc()
> > for a while, and "alloc" doesn't claim "kmalloc" or "__get_free_page"
> > or whatever, so its new users ought to check, and its old users ought
> > to be updated when a change is made.
> > 
> > This is a
> > Fixes: fdc85222d58e ("kernfs: kvmalloc xattr value instead of kmalloc")
> > Cc: stable@vger.kernel.org # v5.7
> > 
> > (Though I hope the restricted use of xattrs on tmpfs cannot actually
> > get into multi-page allocations.)
> > 
> > It's a good catch, Chengguang, thank you: but isn't your patch
> > incomplete?  It looks like this omission goes beyond mm/shmem:
> > include/linux/xattr.h contains a simple_xattrs_free(), used by
> > both shmem and kernfs, which also says "kfree(xattr)" still.
> > 
> > Please extend and re-subject and re-Cc your fix to cover that
> > too (and check nothing else has been missed) - thanks.
> 
> Thanks for pointing that out, I overlooked that part. Since this patch
> has already merged to Andrew's tree, I would like to make another
> patch to handle rest of the fixing and that probably can go into
> vfs-tree or others.

So it has.  Well, I'd prefer you to make one patch for all the fallout,
sent to Andrew, Cc'ed to the people Cc'ed on fdc85222d58e and me; then
Andrew will drop mm-shmem-fix-freeing-new_attr-in-shmem_initxattrs.patch
in favor of the new patch - which will be fixing more of mm/shmem too
(it calls the buggy inline function).  But if you or Andrew disagree,
no problem, better to fix it piece by piece than not at all!

Thanks,
Hugh


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()
  2020-07-04  2:20       ` Hugh Dickins
@ 2020-07-04  2:44         ` cgxu
  0 siblings, 0 replies; 6+ messages in thread
From: cgxu @ 2020-07-04  2:44 UTC (permalink / raw)
  To: Hugh Dickins; +Cc: Andrew Morton, Daniel Xu, linux-mm

On 7/4/20 10:20 AM, Hugh Dickins wrote:
> On Sat, 4 Jul 2020, cgxu wrote:
>> On 7/4/20 4:15 AM, Hugh Dickins wrote:
>>> On Fri, 3 Jul 2020, Andrew Morton wrote:
>>>> On Fri,  3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@mykernel.net>
>>>> wrote:
>>>>
>>>>> new_attr is allocated with kvmalloc() so should be freed
>>>>> with kvfree().
>>>>>
>>>>> ...
>>>>>
>>>>> --- a/mm/shmem.c
>>>>> +++ b/mm/shmem.c
>>>>> @@ -3178,7 +3178,7 @@ static int shmem_initxattrs(struct inode *inode,
>>>>>    		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
>>>>> len,
>>>>>    					  GFP_KERNEL);
>>>>>    		if (!new_xattr->name) {
>>>>> -			kfree(new_xattr);
>>>>> +			kvfree(new_xattr);
>>>>>    			return -ENOMEM;
>>>>>    		}
>>>> Indeed.  Maybe simple_xattr_alloc() should have been called
>>>> simple_xattr_kvmalloc().
>>> That would give a better hint, true. But it's been simple_xattr_alloc()
>>> for a while, and "alloc" doesn't claim "kmalloc" or "__get_free_page"
>>> or whatever, so its new users ought to check, and its old users ought
>>> to be updated when a change is made.
>>>
>>> This is a
>>> Fixes: fdc85222d58e ("kernfs: kvmalloc xattr value instead of kmalloc")
>>> Cc: stable@vger.kernel.org # v5.7
>>>
>>> (Though I hope the restricted use of xattrs on tmpfs cannot actually
>>> get into multi-page allocations.)
>>>
>>> It's a good catch, Chengguang, thank you: but isn't your patch
>>> incomplete?  It looks like this omission goes beyond mm/shmem:
>>> include/linux/xattr.h contains a simple_xattrs_free(), used by
>>> both shmem and kernfs, which also says "kfree(xattr)" still.
>>>
>>> Please extend and re-subject and re-Cc your fix to cover that
>>> too (and check nothing else has been missed) - thanks.
>> Thanks for pointing that out, I overlooked that part. Since this patch
>> has already merged to Andrew's tree, I would like to make another
>> patch to handle rest of the fixing and that probably can go into
>> vfs-tree or others.
> So it has.  Well, I'd prefer you to make one patch for all the fallout,
> sent to Andrew, Cc'ed to the people Cc'ed on fdc85222d58e and me; then
> Andrew will drop mm-shmem-fix-freeing-new_attr-in-shmem_initxattrs.patch
> in favor of the new patch - which will be fixing more of mm/shmem too
> (it calls the buggy inline function).  But if you or Andrew disagree,
> no problem, better to fix it piece by piece than not at all!

That's also fine for me, let me send v2 that includes all of the fixing.

Thanks,
cgxu





^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-04  2:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-03  6:56 [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs() Chengguang Xu
2020-07-03 19:20 ` Andrew Morton
2020-07-03 20:15   ` Hugh Dickins
2020-07-04  1:59     ` cgxu
2020-07-04  2:20       ` Hugh Dickins
2020-07-04  2:44         ` cgxu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).