ubifs: Massage assert in ubifs_xattr_set() wrt. init_xattrs

xiaoleili · richardweinberger · commit d8db5b1ca9d4 · 2017-07-14T22:50:54.000+02:00
The inode is not locked in init_xattrs when creating a new inode.

Without this patch, there will occurs assert when booting or creating
a new file, if the kernel config CONFIG_SECURITY_SMACK is enabled.

Log likes:

UBIFS assert failed in ubifs_xattr_set at 298 (pid 1156)
CPU: 1 PID: 1156 Comm: ldconfig Tainted: G S 4.12.0-rc1-207440-g1e70b02 #2
Hardware name: MediaTek MT2712 evaluation board (DT)
Call trace:
[&lt;ffff000008088538&gt;] dump_backtrace+0x0/0x238
[&lt;ffff000008088834&gt;] show_stack+0x14/0x20
[&lt;ffff0000083d98d4&gt;] dump_stack+0x9c/0xc0
[&lt;ffff00000835d524&gt;] ubifs_xattr_set+0x374/0x5e0
[&lt;ffff00000835d7ec&gt;] init_xattrs+0x5c/0xb8
[&lt;ffff000008385788&gt;] security_inode_init_security+0x110/0x190
[&lt;ffff00000835e058&gt;] ubifs_init_security+0x30/0x68
[&lt;ffff00000833ada0&gt;] ubifs_mkdir+0x100/0x200
[&lt;ffff00000820669c&gt;] vfs_mkdir+0x11c/0x1b8
[&lt;ffff00000820b73c&gt;] SyS_mkdirat+0x74/0xd0
[&lt;ffff000008082f8c&gt;] __sys_trace_return+0x0/0x4

Signed-off-by: Xiaolei Li &lt;xiaolei.li@mediatek.com&gt;
Signed-off-by: Richard Weinberger &lt;richard@nod.at&gt;
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
@@ -9,8 +9,13 @@ static int ubifs_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 static int ubifs_crypt_set_context(struct inode *inode, const void *ctx,
 				   size_t len, void *fs_data)
 {
+	/*
+	 * Creating an encryption context is done unlocked since we
+	 * operate on a new inode which is not visible to other users
+	 * at this point. So, no need to check whether inode is locked.
+	 */
 	return ubifs_xattr_set(inode, UBIFS_XATTR_NAME_ENCRYPTION_CONTEXT,
-			       ctx, len, 0);
+			       ctx, len, 0, false);
 }
 
 static bool ubifs_crypt_empty_dir(struct inode *inode)
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
@@ -1755,7 +1755,7 @@ int ubifs_check_dir_empty(struct inode *dir);
 extern const struct xattr_handler *ubifs_xattr_handlers[];
 ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
 int ubifs_xattr_set(struct inode *host, const char *name, const void *value,
-		    size_t size, int flags);
+		    size_t size, int flags, bool check_lock);
 ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf,
 			size_t size);
 void ubifs_evict_xattr_inode(struct ubifs_info *c, ino_t xattr_inum);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
@@ -280,7 +280,7 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
 }
 
 int ubifs_xattr_set(struct inode *host, const char *name, const void *value,
-		    size_t size, int flags)
+		    size_t size, int flags, bool check_lock)
 {
 	struct inode *inode;
 	struct ubifs_info *c = host->i_sb->s_fs_info;
@@ -289,12 +289,7 @@ int ubifs_xattr_set(struct inode *host, const char *name, const void *value,
 	union ubifs_key key;
 	int err;
 
-	/*
-	 * Creating an encryption context is done unlocked since we
-	 * operate on a new inode which is not visible to other users
-	 * at this point.
-	 */
-	if (strcmp(name, UBIFS_XATTR_NAME_ENCRYPTION_CONTEXT) != 0)
+	if (check_lock)
 		ubifs_assert(inode_is_locked(host));
 
 	if (size > UBIFS_MAX_INO_DATA)
@@ -598,8 +593,12 @@ static int init_xattrs(struct inode *inode, const struct xattr *xattr_array,
 		}
 		strcpy(name, XATTR_SECURITY_PREFIX);
 		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+		/*
+		 * creating a new inode without holding the inode rwsem,
+		 * no need to check whether inode is locked.
+		 */
 		err = ubifs_xattr_set(inode, name, xattr->value,
-				      xattr->value_len, 0);
+				      xattr->value_len, 0, false);
 		kfree(name);
 		if (err < 0)
 			break;
@@ -646,7 +645,7 @@ static int xattr_set(const struct xattr_handler *handler,
 	name = xattr_full_name(handler, name);
 
 	if (value)
-		return ubifs_xattr_set(inode, name, value, size, flags);
+		return ubifs_xattr_set(inode, name, value, size, flags, true);
 	else
 		return ubifs_xattr_remove(inode, name);
 }

Original file line number	Diff line number	Diff line change
`@@ -9,8 +9,13 @@ static int ubifs_crypt_get_context(struct inode inode, void ctx, size_t len)`
`9`	`9`	`static int ubifs_crypt_set_context(struct inode inode, const void ctx,`
`10`	`10`	`size_t len, void *fs_data)`
`11`	`11`	`{`
	`12`	`+ /*`
	`13`	`+ * Creating an encryption context is done unlocked since we`
	`14`	`+ * operate on a new inode which is not visible to other users`
	`15`	`+ * at this point. So, no need to check whether inode is locked.`
	`16`	`+ */`
`12`	`17`	`return ubifs_xattr_set(inode, UBIFS_XATTR_NAME_ENCRYPTION_CONTEXT,`
`13`		`- ctx, len, 0);`
	`18`	`+ ctx, len, 0, false);`
`14`	`19`	`}`
`15`	`20`
`16`	`21`	`static bool ubifs_crypt_empty_dir(struct inode *inode)`