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

login
register
mail settings
Submitter Marc Kleine-Budde
Date Feb. 13, 2013, 10:23 a.m.
Message ID <1360750998-15191-5-git-send-email-mkl@pengutronix.de>
Download mbox | patch
Permalink /patch/220111/
State New
Headers show

Comments

Marc Kleine-Budde - Feb. 13, 2013, 10:23 a.m.
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(-)
Artem Bityutskiy - Feb. 14, 2013, 7:28 a.m.
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?
Marc Kleine-Budde - Feb. 14, 2013, 12:05 p.m.
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
Artem Bityutskiy - Feb. 22, 2013, 7:10 a.m.
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!

Patch

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;
+}