On Fri, 3 Jul 2020, Andrew Morton wrote:
> On Fri, 3 Jul 2020 14:56:36 +0800 Chengguang Xu <cgxu519@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx # 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
