[RFC PATCH 0/5] UBIFS: improve xattr and add support for security.* XATTR

[RFC PATCH 0/5] UBIFS: improve xattr and add support for security.* XATTR

[Marc Kleine-Budde]

Hello,

this patch series is based on Subodh Nijsure's patch [1] from May 2012. I've
splitted up the patch into smaller changes and fixed a locking problem. This
series is tested on mx28-evk with NAND. However, With lockdep enabled a
possible circular locking dependency is detected, as reported in [2]. Otherwise
the system is stable.

Please review and comment, regards,
Marc

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-May/041503.html
[2] http://www.spinics.net/lists/kernel/msg1480694.html

[PATCH RFC 1/5] UBIFS: xattr: protect ui_size and data_len by ui_mutex

[Marc Kleine-Budde]

This patch moves the modification of ui->ui_size and ui->data_len in the
create_xattr() and change_xattr() functions, so that they are protected by the
ui_mutex as stated in the documenation of the the struct ubifs_inode.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 fs/ubifs/xattr.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 0f7139b..1395307 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -143,10 +143,10 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 		err = -ENOMEM;
 		goto out_free;
 	}
-	inode->i_size = ui->ui_size = size;
-	ui->data_len = size;
 
 	mutex_lock(&host_ui->ui_mutex);
+	inode->i_size = ui->ui_size = size;
+	ui->data_len = size;
 	host->i_ctime = ubifs_current_time(host);
 	host_ui->xattr_cnt += 1;
 	host_ui->xattr_size += CALC_DENT_SIZE(nm->len);
@@ -208,10 +208,10 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 		err = -ENOMEM;
 		goto out_free;
 	}
-	inode->i_size = ui->ui_size = size;
-	ui->data_len = size;
 
 	mutex_lock(&host_ui->ui_mutex);
+	inode->i_size = ui->ui_size = size;
+	ui->data_len = size;
 	host->i_ctime = ubifs_current_time(host);
 	host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len);
 	host_ui->xattr_size += CALC_XATTR_BYTES(size);
-- 
1.7.10.4

[PATCH RFC 2/5] UBIFS: fix couple bugs in UBIFS extended attribute length calculation

[Marc Kleine-Budde]

From: Subodh Nijsure <snijsure@grid-net.com>

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 fs/ubifs/journal.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index afaad07..6981d7d 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -572,7 +572,12 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 
 	aligned_dlen = ALIGN(dlen, 8);
 	aligned_ilen = ALIGN(ilen, 8);
-	len = aligned_dlen + aligned_ilen + UBIFS_INO_NODE_SZ;
+	/*
+	 * Make sure to account for dir_ui+data_len in length
+	 * calculation in case there is extended attribute.
+	 */
+	len = aligned_dlen + aligned_ilen +
+	      UBIFS_INO_NODE_SZ + dir_ui->data_len;
 	dent = kmalloc(len, GFP_NOFS);
 	if (!dent)
 		return -ENOMEM;
@@ -649,7 +654,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 
 	ino_key_init(c, &ino_key, dir->i_ino);
 	ino_offs += aligned_ilen;
-	err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs, UBIFS_INO_NODE_SZ);
+	err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs,
+			    UBIFS_INO_NODE_SZ + dir_ui->data_len);
 	if (err)
 		goto out_ro;
 
-- 
1.7.10.4

[PATCH RFC 3/5] UBIFS: Add xattr support for symlinks

[Marc Kleine-Budde]

From: Subodh Nijsure <snijsure@grid-net.com>

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 fs/ubifs/file.c  |    4 ++++
 fs/ubifs/xattr.c |   18 +++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 5bc7781..ed9655c 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1574,6 +1574,10 @@ const struct inode_operations ubifs_symlink_inode_operations = {
 	.follow_link = ubifs_follow_link,
 	.setattr     = ubifs_setattr,
 	.getattr     = ubifs_getattr,
+	.setxattr    = ubifs_setxattr,
+	.getxattr    = ubifs_getxattr,
+	.listxattr   = ubifs_listxattr,
+	.removexattr = ubifs_removexattr,
 };
 
 const struct file_operations ubifs_file_operations = {
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 1395307..d496aa4 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -293,18 +293,16 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
 	return ERR_PTR(-EINVAL);
 }
 
-int ubifs_setxattr(struct dentry *dentry, const char *name,
-		   const void *value, size_t size, int flags)
+static int __ubifs_setxattr(struct inode *host, const char *name,
+			    const void *value, size_t size, int flags)
 {
-	struct inode *inode, *host = dentry->d_inode;
+	struct inode *inode;
 	struct ubifs_info *c = host->i_sb->s_fs_info;
 	struct qstr nm = QSTR_INIT(name, strlen(name));
 	struct ubifs_dent_node *xent;
 	union ubifs_key key;
 	int err, type;
 
-	dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
-		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
 	ubifs_assert(mutex_is_locked(&host->i_mutex));
 
 	if (size > UBIFS_MAX_INO_DATA)
@@ -356,6 +354,16 @@ out_free:
 	return err;
 }
 
+int ubifs_setxattr(struct dentry *dentry, const char *name,
+		     const void *value, size_t size, int flags)
+{
+	dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd",
+		name, dentry->d_inode->i_ino, dentry->d_name.len,
+		dentry->d_name.name, size);
+
+	return __ubifs_setxattr(dentry->d_inode, name, value, size, flags);
+}
+
 ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 		       size_t size)
 {
-- 
1.7.10.4

[PATCH RFC 4/5] UBIFS: Add security.* XATTR support for the UBIFS

[Marc Kleine-Budde]

From: Subodh Nijsure <snijsure@grid-net.com>

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 fs/ubifs/dir.c     |   16 +++++++++++
 fs/ubifs/journal.c |    3 +-
 fs/ubifs/super.c   |    1 +
 fs/ubifs/ubifs.h   |    4 +++
 fs/ubifs/xattr.c   |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 8a57477..6aa31b5 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -280,6 +280,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
 	if (err)
 		goto out_cancel;
+
+	err = ubifs_init_security(dir, inode, &dentry->d_name);
+	if (err)
+		goto out_cancel;
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -742,6 +746,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		ubifs_err("cannot create directory, error %d", err);
 		goto out_cancel;
 	}
+
+	err = ubifs_init_security(dir, inode, &dentry->d_name);
+	if (err)
+		goto out_cancel;
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -818,6 +826,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 	err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
 	if (err)
 		goto out_cancel;
+
+	err = ubifs_init_security(dir, inode, &dentry->d_name);
+	if (err)
+		goto out_cancel;
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -894,6 +906,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 	err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
 	if (err)
 		goto out_cancel;
+
+	err = ubifs_init_security(dir, inode, &dentry->d_name);
+	if (err)
+		goto out_cancel;
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 6981d7d..b96718a 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -553,7 +553,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 
 	dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu",
 		inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino);
-	ubifs_assert(dir_ui->data_len == 0);
+	if (!xent)
+		ubifs_assert(dir_ui->data_len == 0);
 	ubifs_assert(mutex_is_locked(&dir_ui->ui_mutex));
 
 	dlen = UBIFS_DENT_NODE_SZ + nm->len + 1;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index ddc0f6a..8b55c43 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2047,6 +2047,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 	if (c->max_inode_sz > MAX_LFS_FILESIZE)
 		sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
 	sb->s_op = &ubifs_super_operations;
+	sb->s_xattr = ubifs_xattr_handlers;
 
 	mutex_lock(&c->umount_mutex);
 	err = mount_ubifs(c);
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index d133c27..9b1b6d2 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -36,6 +36,7 @@
 #include <linux/mtd/ubi.h>
 #include <linux/pagemap.h>
 #include <linux/backing-dev.h>
+#include <linux/security.h>
 #include "ubifs-media.h"
 
 /* Version of this UBIFS implementation */
@@ -1454,6 +1455,7 @@ extern spinlock_t ubifs_infos_lock;
 extern atomic_long_t ubifs_clean_zn_cnt;
 extern struct kmem_cache *ubifs_inode_slab;
 extern const struct super_operations ubifs_super_operations;
+extern const struct xattr_handler *ubifs_xattr_handlers[];
 extern const struct address_space_operations ubifs_file_address_operations;
 extern const struct file_operations ubifs_file_operations;
 extern const struct inode_operations ubifs_file_inode_operations;
@@ -1740,6 +1742,8 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 		       size_t size);
 ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
 int ubifs_removexattr(struct dentry *dentry, const char *name);
+int ubifs_init_security(struct inode *dentry, struct inode *inode,
+			const struct qstr *qstr);
 
 /* super.c */
 struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index d496aa4..aa81bd3 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -576,3 +576,82 @@ out_free:
 	kfree(xent);
 	return err;
 }
+
+size_t ubifs_security_listxattr(struct dentry *d, char *list, size_t list_size,
+				const char *name, size_t name_len, int flags)
+{
+	const int prefix_len = XATTR_SECURITY_PREFIX_LEN;
+	const size_t total_len = prefix_len + name_len + 1;
+
+	if (list && total_len <= list_size) {
+		memcpy(list, XATTR_SECURITY_PREFIX, prefix_len);
+		memcpy(list + prefix_len, name, name_len);
+		list[prefix_len + name_len] = '\0';
+	}
+
+	return total_len;
+}
+
+int ubifs_security_getxattr(struct dentry *d, const char *name,
+			    void *buffer, size_t size, int flags)
+{
+	return ubifs_getxattr(d, name, buffer, size);
+}
+
+int ubifs_security_setxattr(struct dentry *d, const char *name,
+			    const void *value, size_t size,
+			    int flags, int handler_flags)
+{
+	return ubifs_setxattr(d, name, value, size, flags);
+}
+
+struct xattr_handler ubifs_xattr_security_handler = {
+	.prefix = XATTR_SECURITY_PREFIX,
+	.list   = ubifs_security_listxattr,
+	.get    = ubifs_security_getxattr,
+	.set    = ubifs_security_setxattr,
+};
+
+const struct xattr_handler *ubifs_xattr_handlers[] = {
+	&ubifs_xattr_security_handler,
+	NULL
+};
+
+static int ubifs_initxattrs(struct inode *inode,
+			    const struct xattr *xattr_array, void *fs_info)
+{
+	const struct xattr *xattr;
+	char *name;
+	int err = 0;
+
+	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+		name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
+			       strlen(xattr->name) + 1, GFP_NOFS);
+		if (!name) {
+			err = -ENOMEM;
+			break;
+		}
+		strcpy(name, XATTR_SECURITY_PREFIX);
+		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+		err = __ubifs_setxattr(inode, name, xattr->value,
+				       xattr->value_len, 0);
+		kfree(name);
+		if (err < 0)
+			break;
+	}
+
+	return err;
+}
+
+int ubifs_init_security(struct inode *dentry, struct inode *inode,
+			const struct qstr *qstr)
+{
+	int err;
+
+	mutex_lock(&inode->i_mutex);
+	err = security_inode_init_security(inode, dentry, qstr,
+					   &ubifs_initxattrs, 0);
+	mutex_unlock(&inode->i_mutex);
+
+	return err;
+}
-- 
1.7.10.4

[PATCH RFC 5/5] UBIFS: add ubifs_err() to print error reason

[Marc Kleine-Budde]

From: Subodh Nijsure <snijsure@grid-net.com>

This patch add ubifs_err() output to some error pathes to tell the user what's
going on.

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 fs/ubifs/dir.c   |   20 ++++++++++++++++----
 fs/ubifs/xattr.c |   12 ++++++++++--
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 6aa31b5..d0dd7a9 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -282,8 +282,11 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 		goto out_cancel;
 
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
-	if (err)
+	if (err) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
 		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -748,8 +751,11 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	}
 
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
-	if (err)
+	if (err) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
 		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -828,8 +834,11 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 		goto out_cancel;
 
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
-	if (err)
+	if (err) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
 		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
@@ -908,8 +917,11 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 		goto out_cancel;
 
 	err = ubifs_init_security(dir, inode, &dentry->d_name);
-	if (err)
+	if (err) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
 		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index aa81bd3..380fcc9 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -107,8 +107,11 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 				.new_ino_d = ALIGN(size, 8), .dirtied_ino = 1,
 				.dirtied_ino_d = ALIGN(host_ui->data_len, 8) };
 
-	if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE)
+	if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE) {
+		ubifs_err("ubifs xattr_cnt %d exceeds MAX_XATTR_PER_NODE (%d)",
+			  host_ui->xattr_cnt, MAX_XATTRS_PER_INODE);
 		return -ENOSPC;
+	}
 	/*
 	 * Linux limits the maximum size of the extended attribute names list
 	 * to %XATTR_LIST_MAX. This means we should not allow creating more
@@ -116,8 +119,13 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 	 * is artificial for UBIFS, though.
 	 */
 	if (host_ui->xattr_names + host_ui->xattr_cnt +
-					nm->len + 1 > XATTR_LIST_MAX)
+					nm->len + 1 > XATTR_LIST_MAX) {
+		ubifs_err("xattr name list too large %d > %d",
+			  host_ui->xattr_names + host_ui->xattr_cnt +
+			  nm->len + 1,
+			  XATTR_LIST_MAX);
 		return -ENOSPC;
+	}
 
 	err = ubifs_budget_space(c, &req);
 	if (err)
-- 
1.7.10.4

Re: [PATCH RFC 1/5] UBIFS: xattr: protect ui_size and data_len by ui_mutex

[Artem Bityutskiy]

On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
> This patch moves the modification of ui->ui_size and ui->data_len in the
> create_xattr() and change_xattr() functions, so that they are protected by the
> ui_mutex as stated in the documenation of the the struct ubifs_inode.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

I guess this one and 2/5 should have 'Cc: stable@vger.kernel.org',
right?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH RFC 4/5] UBIFS: Add security.* XATTR support for the UBIFS

[Artem Bityutskiy]

On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -553,7 +553,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>  
>         dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu",
>                 inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino);
> -       ubifs_assert(dir_ui->data_len == 0);
> +       if (!xent)
> +               ubifs_assert(dir_ui->data_len == 0);

Shouldn't this snippet be in 2/5 instead?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH RFC 5/5] UBIFS: add ubifs_err() to print error reason

[Artem Bityutskiy]

On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
>         err = ubifs_init_security(dir, inode, &dentry->d_name);
> -       if (err)
> +       if (err) {
> +               ubifs_err("cannot initialize extended attribute, error %d",
> +                         err);
>                 goto out_cancel;
> +       }

Would you please instead make 'ubifs_init_security()' print the error
message.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH RFC 1/5] UBIFS: xattr: protect ui_size and data_len by ui_mutex

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On 02/14/2013 08:23 AM, Artem Bityutskiy wrote:
> On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
>> This patch moves the modification of ui->ui_size and ui->data_len in the
>> create_xattr() and change_xattr() functions, so that they are protected by the
>> ui_mutex as stated in the documenation of the the struct ubifs_inode.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> I guess this one and 2/5 should have 'Cc: stable@vger.kernel.org',
> right?

Will do.

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH RFC 4/5] UBIFS: Add security.* XATTR support for the UBIFS

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On 02/14/2013 08:28 AM, Artem Bityutskiy wrote:
> On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
>> --- a/fs/ubifs/journal.c
>> +++ b/fs/ubifs/journal.c
>> @@ -553,7 +553,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>>  
>>         dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu",
>>                 inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino);
>> -       ubifs_assert(dir_ui->data_len == 0);
>> +       if (!xent)
>> +               ubifs_assert(dir_ui->data_len == 0);
> 
> Shouldn't this snippet be in 2/5 instead?
will do

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH RFC 4/5] UBIFS: Add security.* XATTR support for the UBIFS

[Artem Bityutskiy]

OK, the lockdep warnings clearly tell the reason:

        CPU0                    CPU1
        ----                    ----
   lock(&ui->ui_mutex);
                                lock(&sb->s_type->i_mutex_key#10);
                                lock(&ui->ui_mutex);
   lock(&sb->s_type->i_mutex_key#10);

And then there are 2 tracebacks which are useful and show that you
unnecessarily initialize the inode security contenxt whil holding the
parent inode lock. I think you do not need to hold that lock. Move the
initialization out of the protected section.

See below my suggestions.

On Wed, 2013-02-13 at 11:23 +0100, Marc Kleine-Budde wrote:
> @@ -280,6 +280,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
>  	err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
>  	if (err)
>  		goto out_cancel;
> +
> +	err = ubifs_init_security(dir, inode, &dentry->d_name);
> +	if (err)
> +		goto out_cancel;
>  	mutex_unlock(&dir_ui->ui_mutex);

Can you move ubifs_init_security() up to before
'mutex_lock(&dir_ui->ui_mutex)'

> @@ -742,6 +746,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
...
> +	err = ubifs_init_security(dir, inode, &dentry->d_name);
> +	if (err)
> +		goto out_cancel;
>  	mutex_unlock(&dir_ui->ui_mutex);

Ditto.

> @@ -818,6 +826,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
...
> +	err = ubifs_init_security(dir, inode, &dentry->d_name);
> +	if (err)
> +		goto out_cancel;
>  	mutex_unlock(&dir_ui->ui_mutex);

Ditto.

> @@ -894,6 +906,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
...
> +	err = ubifs_init_security(dir, inode, &dentry->d_name);
> +	if (err)
> +		goto out_cancel;
>  	mutex_unlock(&dir_ui->ui_mutex);

Ditto.

> +int ubifs_init_security(struct inode *dentry, struct inode *inode,
> +			const struct qstr *qstr)
> +{
> +	int err;
> +
> +	mutex_lock(&inode->i_mutex);
> +	err = security_inode_init_security(inode, dentry, qstr,
> +					   &ubifs_initxattrs, 0);
> +	mutex_unlock(&inode->i_mutex);

I did not verify, but I doubt that you need i_mutex here, because you
only call this function when you create an inode, before it becomes
visible to VFS. Please, double-check this.

Thanks!

-- 
Best Regards,
Artem Bityutskiy