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@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.
>
> 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
