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

[cgxu <cgxu519@mykernel.net>]

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