| Message ID | 20200703065636.20897-1-cgxu519@mykernel.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/shmem: fix freeing new_attr in shmem_initxattrs() | expand |
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().
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
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
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
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
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; }
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(-)