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

[Hugh Dickins <hughd@google.com>]

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