Re: [PATCH] mm/shmem: fix freeing new_attr in shmem_initxattrs()

[Hugh Dickins <hughd@google.com>]

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