5.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Mike Marshall hubcap@omnibond.com
[ Upstream commit 025e880759c279ec64d0f754fe65bf45961da864 ]
Willy Tarreau w@1wt.eu forwarded me a message from Disclosure disclosure@aisle.com with the following warning:
The helper `xattr_key()` uses the pointer variable in the loop condition rather than dereferencing it. As `key` is incremented, it remains non-NULL (until it runs into unmapped memory), so the loop does not terminate on valid C strings and will walk memory indefinitely, consuming CPU or hanging the thread.
I easily reproduced this with setfattr and getfattr, causing a kernel oops, hung user processes and corrupted orangefs files. Disclosure sent along a diff (not a patch) with a suggested fix, which I based this patch on.
After xattr_key started working right, xfstest generic/069 exposed an xattr related memory leak that lead to OOM. xattr_key returns a hashed key. When adding xattrs to the orangefs xattr cache, orangefs used hash_add, a kernel hashing macro. hash_add also hashes the key using hash_log which resulted in additions to the xattr cache going to the wrong hash bucket. generic/069 tortures a single file and orangefs does a getattr for the xattr "security.capability" every time. Orangefs negative caches on xattrs which includes a kmalloc. Since adds to the xattr cache were going to the wrong bucket, every getattr for "security.capability" resulted in another kmalloc, none of which were ever freed.
I changed the two uses of hash_add to hlist_add_head instead and the memory leak ceased and generic/069 quit throwing furniture.
Signed-off-by: Mike Marshall hubcap@omnibond.com Reported-by: Stanislav Fort of Aisle Research stanislav.fort@aisle.com Signed-off-by: Sasha Levin sashal@kernel.org --- fs/orangefs/xattr.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c index bdc285aea3600..5e355d9d9a819 100644 --- a/fs/orangefs/xattr.c +++ b/fs/orangefs/xattr.c @@ -54,7 +54,9 @@ static inline int convert_to_internal_xattr_flags(int setxattr_flags) static unsigned int xattr_key(const char *key) { unsigned int i = 0; - while (key) + if (!key) + return 0; + while (*key) i += *key++; return i % 16; } @@ -175,8 +177,8 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *name, cx->length = -1; cx->timeout = jiffies + orangefs_getattr_timeout_msecs*HZ/1000; - hash_add(orangefs_inode->xattr_cache, &cx->node, - xattr_key(cx->key)); + hlist_add_head( &cx->node, + &orangefs_inode->xattr_cache[xattr_key(cx->key)]); } } goto out_release_op; @@ -229,8 +231,8 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *name, memcpy(cx->val, buffer, length); cx->length = length; cx->timeout = jiffies + HZ; - hash_add(orangefs_inode->xattr_cache, &cx->node, - xattr_key(cx->key)); + hlist_add_head(&cx->node, + &orangefs_inode->xattr_cache[xattr_key(cx->key)]); } }