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.
Thanks,
cgxu
