On 7/4/20 10:20 AM, Hugh Dickins wrote:
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!
That's also fine for me, let me send v2 that includes all of the fixing.
Thanks,
cgxu
