diff mbox

[v4] Add security.* XATTR support for the UBIFS

Message ID 1336915488-23943-1-git-send-email-snijsure@grid-net.com
State New, archived
Headers show

Commit Message

Subodh Nijsure May 13, 2012, 1:24 p.m. UTC 
From: Subodh Nijsure <snijsure@grid-net.com>

Also fix couple of bugs in UBIFS extended attribute length calculation.

Changes in v4:
        Fix lock issues introduced in v3.
        Tested with CONFIG_SECURITY enabled & disabled.

Changes in v3:
	 Remove #ifdef CONFIG_UBIFS_FS_XATTR

Changes in v2:
         Instead of just handling security.selinux extended attribute handle
         all security.* attributes.

TESTING: Tested on  MX28 based platforms using Micron MT29F2G08ABAEAH4 NAND
         With these change we are able to label UBIFS filesystem with
         security.selinux and run system with selinux enabled.
         This change also allows one to set other security.* extended
         attributes, such as security.smack security.evm, security.ima
         Ran integck test on UBI filesystem.
         This patch set has been tested with CONFIG_LOCKDEP=y and other options
         suggested in Submitchecklist

Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
---
 fs/ubifs/dir.c     |   29 ++++++++-
 fs/ubifs/file.c    |    4 +
 fs/ubifs/journal.c |   12 +++-
 fs/ubifs/super.c   |    1 +
 fs/ubifs/ubifs.h   |    8 +++
 fs/ubifs/xattr.c   |  164 ++++++++++++++++++++++++++++++++++++++++++++++------
 6 files changed, 193 insertions(+), 25 deletions(-)

Comments

Artem Bityutskiy May 14, 2012, 1:02 p.m. UTC | #1 
On Sun, 2012-05-13 at 06:24 -0700, snijsure@grid-net.com wrote:
> +int ubifs_security_getxattr(struct dentry *d, const char *name,
> +			    void *buffer, size_t size, int flags)
> +{
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;
> +	return __ubifs_getxattr(d->d_inode, name, buffer, size);
> +}
> +
> +
> +int ubifs_security_setxattr(struct dentry *d, const char *name,
> +			    const void *value, size_t size,
> +			    int flags, int handler_flags)
> +{
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;

Should this check pushed town to __ubifs_getxattr/__ubifs_setxattr ?
Does an extended attribute in general with zero name length legitimate?
Did you check whether the generic code already performs this check?

> +	return __ubifs_setxattr(d->d_inode, 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;

Where is the already allocated memory freed in this case?

> +		}
> +		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)
> +{
> +	struct ubifs_inode *dir_ui = ubifs_inode(inode);
> +	int err = 0;
> +
> +	mutex_lock(&inode->i_mutex);
> +	mutex_lock(&dir_ui->ui_mutex);
> +

You do not actually need these mutexes, because "inode" is new, it is
not added to any lists yet, so you own it entirely. Which means that you
do not even need to introduce this helper function - just call
'security_inode_init_security()' directly.
Subodh Nijsure May 14, 2012, 9:09 p.m. UTC | #2 
On 05/14/2012 06:02 AM, Artem Bityutskiy wrote:
> On Sun, 2012-05-13 at 06:24 -0700, snijsure@grid-net.com wrote:
>> +int ubifs_security_getxattr(struct dentry *d, const char *name,
>> +			    void *buffer, size_t size, int flags)
>> +{
>> +	if (strcmp(name, "") == 0)
>> +		return -EINVAL;
>> +	return __ubifs_getxattr(d->d_inode, name, buffer, size);
>> +}
>> +
>> +
>> +int ubifs_security_setxattr(struct dentry *d, const char *name,
>> +			    const void *value, size_t size,
>> +			    int flags, int handler_flags)
>> +{
>> +	if (strcmp(name, "") == 0)
>> +		return -EINVAL;
> Should this check pushed town to __ubifs_getxattr/__ubifs_setxattr ?
If you really want to move that check into __ubifs_get/setxattr we can 
do that.
However the above implementation is consistent with ext2/ext3/ext4/jffs 
implementation.
> Does an extended attribute in general with zero name length legitimate?
My preference would be to remain consistent with interpretation of other 
file systems, in terms of what constitutes an
invalid parameter. ext* filesystems seem to be declaring a blank 
extended attribute as invalid parameter. Man page for setxattr/getxattr 
don't explicitly state as such though.
> Did you check whether the generic code already performs this check?
I didn't see a generic code that performs this check.

>> +	return __ubifs_setxattr(d->d_inode, 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;
> Where is the already allocated memory freed in this case?
In this particular case kmalloc() failed and we are returning ENOMEM 
error, and in case of success, we do free the allocated memory.
>
>> +		}
>> +		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)
>> +{
>> +	struct ubifs_inode *dir_ui = ubifs_inode(inode);
>> +	int err = 0;
>> +
>> +	mutex_lock(&inode->i_mutex);
>> +	mutex_lock(&dir_ui->ui_mutex);
>> +
> You do not actually need these mutexes, because "inode" is new, it is
> not added to any lists yet, so you own it entirely. Which means that you
> do not even need to introduce this helper function - just call
> 'security_inode_init_security()' directly.
Okay, I can change the code to directly call the 
security_inode_init_security().
I will wait couple days to see if there are other comments trickle in 
before submitting v5.
It would great if someone else can run UBIFS with extended attributes 
enabled and provide an ACK! ;-)

-Subodh
Artem Bityutskiy May 15, 2012, 10:29 a.m. UTC | #3 
On Mon, 2012-05-14 at 14:09 -0700, Subodh Nijsure wrote:
> On 05/14/2012 06:02 AM, Artem Bityutskiy wrote:
> > On Sun, 2012-05-13 at 06:24 -0700, snijsure@grid-net.com wrote:
> >> +int ubifs_security_getxattr(struct dentry *d, const char *name,
> >> +			    void *buffer, size_t size, int flags)
> >> +{
> >> +	if (strcmp(name, "") == 0)
> >> +		return -EINVAL;
> >> +	return __ubifs_getxattr(d->d_inode, name, buffer, size);
> >> +}
> >> +
> >> +
> >> +int ubifs_security_setxattr(struct dentry *d, const char *name,
> >> +			    const void *value, size_t size,
> >> +			    int flags, int handler_flags)
> >> +{
> >> +	if (strcmp(name, "") == 0)
> >> +		return -EINVAL;
> > Should this check pushed town to __ubifs_getxattr/__ubifs_setxattr ?
> If you really want to move that check into __ubifs_get/setxattr we can 
> do that.

Yes, if other FSes have this check - please add it there.

> However the above implementation is consistent with ext2/ext3/ext4/jffs 
> implementation.

OK, but on the other hand - how much sense does it make to have these
trivial wrappers? Should we have a wrapper per-check? :-)

BTW, to they have to be non-static?

> > Does an extended attribute in general with zero name length legitimate?
> My preference would be to remain consistent with interpretation of other 
> file systems, in terms of what constitutes an
> invalid parameter. ext* filesystems seem to be declaring a blank 
> extended attribute as invalid parameter. Man page for setxattr/getxattr 
> don't explicitly state as such though.

Sure, let's add this check - I guess I was not careful enough and missed
it.

> > Did you check whether the generic code already performs this check?
> I didn't see a generic code that performs this check.

OK, thanks.

> >> +	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;
> > Where is the already allocated memory freed in this case?
> In this particular case kmalloc() failed and we are returning ENOMEM 
> error, and in case of success, we do free the allocated memory.

Indeed, sorry for silly question.

> > You do not actually need these mutexes, because "inode" is new, it is
> > not added to any lists yet, so you own it entirely. Which means that you
> > do not even need to introduce this helper function - just call
> > 'security_inode_init_security()' directly.
> Okay, I can change the code to directly call the 
> security_inode_init_security().

OK, thanks!

> It would great if someone else can run UBIFS with extended attributes 
> enabled and provide an ACK! ;-)

I will run it once you send the patch I cannot nit-pick on anymore (aka
perfect patch) :-)))

Thanks!
Marc Kleine-Budde Jan. 15, 2013, 3:48 p.m. UTC | #4 
On 05/13/2012 05:24 AM, Subodh Nijsure wrote:
> From: Subodh Nijsure <snijsure@grid-net.com>
> 
> Also fix couple of bugs in UBIFS extended attribute length calculation.
> 
> Changes in v4:
>         Fix lock issues introduced in v3.
>         Tested with CONFIG_SECURITY enabled & disabled.
> 
> Changes in v3:
> 	 Remove #ifdef CONFIG_UBIFS_FS_XATTR
> 
> Changes in v2:
>          Instead of just handling security.selinux extended attribute handle
>          all security.* attributes.
> 
> TESTING: Tested on  MX28 based platforms using Micron MT29F2G08ABAEAH4 NAND
>          With these change we are able to label UBIFS filesystem with
>          security.selinux and run system with selinux enabled.
>          This change also allows one to set other security.* extended
>          attributes, such as security.smack security.evm, security.ima
>          Ran integck test on UBI filesystem.
>          This patch set has been tested with CONFIG_LOCKDEP=y and other options
>          suggested in Submitchecklist
> 
> Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>

What's the status of this patch? Was there a v5?

Marc
Marc Kleine-Budde Jan. 16, 2013, 9:48 p.m. UTC | #5 
On 01/15/2013 04:48 PM, Marc Kleine-Budde wrote:
> On 05/13/2012 05:24 AM, Subodh Nijsure wrote:
>> From: Subodh Nijsure <snijsure@grid-net.com>
>>
>> Also fix couple of bugs in UBIFS extended attribute length calculation.
>>
>> Changes in v4:
>>         Fix lock issues introduced in v3.
>>         Tested with CONFIG_SECURITY enabled & disabled.
>>
>> Changes in v3:
>> 	 Remove #ifdef CONFIG_UBIFS_FS_XATTR
>>
>> Changes in v2:
>>          Instead of just handling security.selinux extended attribute handle
>>          all security.* attributes.
>>
>> TESTING: Tested on  MX28 based platforms using Micron MT29F2G08ABAEAH4 NAND
>>          With these change we are able to label UBIFS filesystem with
>>          security.selinux and run system with selinux enabled.
>>          This change also allows one to set other security.* extended
>>          attributes, such as security.smack security.evm, security.ima
>>          Ran integck test on UBI filesystem.
>>          This patch set has been tested with CONFIG_LOCKDEP=y and other options
>>          suggested in Submitchecklist
>>
>> Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
> 
> What's the status of this patch? Was there a v5?

I've ported v4 to v3.7.2 and get this deadlock:

> [   18.870000] UBIFS: background thread "ubifs_bgt0_0" started, PID 101
> [   20.650000]
> [   20.650000] ======================================================
> [   20.650000] [ INFO: possible circular locking dependency detected ]
> [   20.650000] 3.7.2-00001-g8cdd29c #2 Not tainted
> [   20.650000] -------------------------------------------------------
> [   20.650000] systemd-rc-once/79 is trying to acquire lock:
> [   20.650000]  (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<c01afe64>] ubifs_init_security+0x28/0x74
> [   20.650000]
> [   20.650000] but task is already holding lock:
> [   20.650000]  (&ui->ui_mutex){+.+...}, at: [<c018a404>] ubifs_create+0x9c/0x1f8
> [   20.650000]
> [   20.650000] which lock already depends on the new lock.
> [   20.650000]
> [   20.650000]
> [   20.650000] the existing dependency chain (in reverse order) is:
> [   20.650000]
> -> #1 (&ui->ui_mutex){+.+...}:
> [   20.650000]        [<c0054c3c>] lock_acquire+0x64/0x78
> [   20.650000]        [<c0391b80>] mutex_lock_nested+0x68/0x2f8
> [   20.650000]        [<c01876fc>] ubifs_setattr+0xe4/0x3ec
> [   20.650000]        [<c00c8f48>] notify_change+0x1dc/0x324
> [   20.650000]        [<c00af3c8>] do_truncate+0x78/0x94
> [   20.650000]        [<c00bd738>] do_last.isra.24+0x530/0xc30
> [   20.650000]        [<c00bdee0>] path_openat+0xa8/0x4b8
> [   20.650000]        [<c00be5ec>] do_filp_open+0x2c/0x80
> [   20.650000]        [<c00b0128>] do_sys_open+0xe4/0x170
> [   20.650000]        [<c000e260>] ret_fast_syscall+0x0/0x38
> [   20.650000]
> -> #0 (&sb->s_type->i_mutex_key#9){+.+.+.}:
> [   20.650000]        [<c0054138>] __lock_acquire+0x1354/0x19b0
> [   20.650000]        [<c0054c3c>] lock_acquire+0x64/0x78
> [   20.650000]        [<c0391b80>] mutex_lock_nested+0x68/0x2f8
> [   20.650000]        [<c01afe64>] ubifs_init_security+0x28/0x74
> [   20.650000]        [<c018a488>] ubifs_create+0x120/0x1f8
> [   20.650000]        [<c00bb7d4>] vfs_create+0xac/0xd8
> [   20.650000]        [<c00bdcb8>] do_last.isra.24+0xab0/0xc30
> [   20.650000]        [<c00bdee0>] path_openat+0xa8/0x4b8
> [   20.650000]        [<c00be5ec>] do_filp_open+0x2c/0x80
> [   20.650000]        [<c00b0128>] do_sys_open+0xe4/0x170
> [   20.650000]        [<c000e260>] ret_fast_syscall+0x0/0x38
> [   20.650000]
> [   20.650000] other info that might help us debug this:
> [   20.650000]
> [   20.650000]  Possible unsafe locking scenario:
> [   20.650000]
> [   20.650000]        CPU0                    CPU1
> [   20.650000]        ----                    ----
> [   20.650000]   lock(&ui->ui_mutex);
> [   20.650000]                                lock(&sb->s_type->i_mutex_key#9);
> [   20.650000]                                lock(&ui->ui_mutex);
> [   20.650000]   lock(&sb->s_type->i_mutex_key#9);
> [   20.650000]
> [   20.650000]  *** DEADLOCK ***
> [   20.650000]
> [   20.650000] 3 locks held by systemd-rc-once/79:
> [   20.650000]  #0:  (sb_writers#3){.+.+.+}, at: [<c00cc300>] mnt_want_write+0x18/0x3c
> [   20.650000]  #1:  (&type->i_mutex_dir_key){+.+.+.}, at: [<c00bd4ac>] do_last.isra.24+0x2a4/0xc30
> [   20.650000]  #2:  (&ui->ui_mutex){+.+...}, at: [<c018a404>] ubifs_create+0x9c/0x1f8
> [   20.650000]
> [   20.650000] stack backtrace:
> [   20.650000] [<c00123e8>] (unwind_backtrace+0x0/0xf0) from [<c038c874>] (print_circular_bug+0x254/0x2a0)
> [   20.650000] [<c038c874>] (print_circular_bug+0x254/0x2a0) from [<c0054138>] (__lock_acquire+0x1354/0x19b0)
> [   20.650000] [<c0054138>] (__lock_acquire+0x1354/0x19b0) from [<c0054c3c>] (lock_acquire+0x64/0x78)
> [   20.650000] [<c0054c3c>] (lock_acquire+0x64/0x78) from [<c0391b80>] (mutex_lock_nested+0x68/0x2f8)
> [   20.650000] [<c0391b80>] (mutex_lock_nested+0x68/0x2f8) from [<c01afe64>] (ubifs_init_security+0x28/0x74)
> [   20.650000] [<c01afe64>] (ubifs_init_security+0x28/0x74) from [<c018a488>] (ubifs_create+0x120/0x1f8)
> [   20.650000] [<c018a488>] (ubifs_create+0x120/0x1f8) from [<c00bb7d4>] (vfs_create+0xac/0xd8)
> [   20.650000] [<c00bb7d4>] (vfs_create+0xac/0xd8) from [<c00bdcb8>] (do_last.isra.24+0xab0/0xc30)
> [   20.650000] [<c00bdcb8>] (do_last.isra.24+0xab0/0xc30) from [<c00bdee0>] (path_openat+0xa8/0x4b8)
> [   20.650000] [<c00bdee0>] (path_openat+0xa8/0x4b8) from [<c00be5ec>] (do_filp_open+0x2c/0x80)
> [   20.650000] [<c00be5ec>] (do_filp_open+0x2c/0x80) from [<c00b0128>] (do_sys_open+0xe4/0x170)
> [   20.650000] [<c00b0128>] (do_sys_open+0xe4/0x170) from [<c000e260>] (ret_fast_syscall+0x0/0x38)
> [   49.430000] UBIFS: background thread "ubifs_bgt0_0" stops

Marc
Subodh Nijsure Jan. 16, 2013, 10:38 p.m. UTC | #6 
Marc,

On 01/15/2013 04:48 PM, Marc Kleine-Budde wrote:
> On 05/13/2012 05:24 AM, Subodh Nijsure wrote:
>> From: Subodh Nijsure <snijsure@grid-net.com>
>>
>> Also fix couple of bugs in UBIFS extended attribute length calculation.
>>
>> Changes in v4:
>>         Fix lock issues introduced in v3.
>>         Tested with CONFIG_SECURITY enabled & disabled.
>>
>> Changes in v3:
>>       Remove #ifdef CONFIG_UBIFS_FS_XATTR
>>
>> Changes in v2:
>>          Instead of just handling security.selinux extended attribute handle
>>          all security.* attributes.
>>
>> TESTING: Tested on  MX28 based platforms using Micron MT29F2G08ABAEAH4 NAND
>>          With these change we are able to label UBIFS filesystem with
>>          security.selinux and run system with selinux enabled.
>>          This change also allows one to set other security.* extended
>>          attributes, such as security.smack security.evm, security.ima
>>          Ran integck test on UBI filesystem.
>>          This patch set has been tested with CONFIG_LOCKDEP=y and other options
>>          suggested in Submitchecklist
>>
>> Signed-off-by: Subodh Nijsure <snijsure@grid-net.com>
>
> What's the status of this patch? Was there a v5?


>I've ported v4 to v3.7.2 and get this deadlock:

Sorry I didn't try to push it and now I am on family leave to take care of new baby at home till end of March. 
I don't have access to hardware any more to try this at home, so I would certainly get to it till beginning of April.

-Subodh
diff mbox

Patch

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index ad6e550..0781d3a 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -288,8 +288,14 @@  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;
-	mutex_unlock(&dir_ui->ui_mutex);
+	err = ubifs_init_security(dir, inode, &dentry->d_name);
+	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);
 	insert_inode_hash(inode);
 	d_instantiate(dentry, inode);
@@ -750,8 +756,13 @@  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) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
+		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
-
 	ubifs_release_budget(c, &req);
 	d_instantiate(dentry, inode);
 	return 0;
@@ -826,8 +837,13 @@  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) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
+		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
-
 	ubifs_release_budget(c, &req);
 	insert_inode_hash(inode);
 	d_instantiate(dentry, inode);
@@ -902,8 +918,13 @@  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) {
+		ubifs_err("cannot initialize extended attribute, error %d",
+			  err);
+		goto out_cancel;
+	}
 	mutex_unlock(&dir_ui->ui_mutex);
-
 	ubifs_release_budget(c, &req);
 	insert_inode_hash(inode);
 	d_instantiate(dentry, inode);
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 0fe640c..8629da0 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1573,6 +1573,10 @@  const struct inode_operations ubifs_symlink_inode_operations = {
 	.follow_link = ubifs_follow_link,
 	.setattr     = ubifs_setattr,
 	.getattr     = ubifs_getattr,
+	.setxattr    = ubifs_symlink_setxattr,
+	.getxattr    = ubifs_symlink_getxattr,
+	.listxattr   = ubifs_listxattr,
+	.removexattr = ubifs_removexattr,
 };
 
 const struct file_operations ubifs_file_operations = {
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index bc75e9d..5086823 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;
@@ -572,7 +573,11 @@  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;
 
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 76e4e05..228c69d 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2061,6 +2061,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 93d59ac..a486c87 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;
@@ -1738,8 +1740,14 @@  int ubifs_getattr(struct vfsmount *mnt, struct dentry *dentry,
 /* xattr.c */
 int ubifs_setxattr(struct dentry *dentry, const char *name,
 		   const void *value, size_t size, int flags);
+int ubifs_init_security(struct inode *dentry, struct inode *inode,
+			const struct qstr *qstr);
+int ubifs_symlink_setxattr(struct dentry *dentry, const char *name,
+			   const void *value, size_t size, int flags);
 ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 		       size_t size);
+ssize_t ubifs_symlink_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);
 
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 85b2722..a1f289b 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -107,8 +107,12 @@  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)
+	ubifs_assert(mutex_is_locked(&host_ui->ui_mutex));
+	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 +120,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)
@@ -146,18 +155,14 @@  static int create_xattr(struct ubifs_info *c, struct inode *host,
 	inode->i_size = ui->ui_size = size;
 	ui->data_len = size;
 
-	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = ubifs_current_time(host);
 	host_ui->xattr_cnt += 1;
 	host_ui->xattr_size += CALC_DENT_SIZE(nm->len);
 	host_ui->xattr_size += CALC_XATTR_BYTES(size);
 	host_ui->xattr_names += nm->len;
-
 	err = ubifs_jnl_update(c, host, nm, inode, 0, 1);
 	if (err)
 		goto out_cancel;
-	mutex_unlock(&host_ui->ui_mutex);
-
 	ubifs_release_budget(c, &req);
 	insert_inode_hash(inode);
 	iput(inode);
@@ -167,7 +172,6 @@  out_cancel:
 	host_ui->xattr_cnt -= 1;
 	host_ui->xattr_size -= CALC_DENT_SIZE(nm->len);
 	host_ui->xattr_size -= CALC_XATTR_BYTES(size);
-	mutex_unlock(&host_ui->ui_mutex);
 out_free:
 	make_bad_inode(inode);
 	iput(inode);
@@ -209,11 +213,11 @@  static int change_xattr(struct ubifs_info *c, struct inode *host,
 		goto out_free;
 	}
 	inode->i_size = ui->ui_size = size;
-	ui->data_len = size;
 
 	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = ubifs_current_time(host);
 	host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len);
+	ui->data_len = size;
 	host_ui->xattr_size += CALC_XATTR_BYTES(size);
 
 	/*
@@ -293,18 +297,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 = { .name = name, .len = 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,10 +358,29 @@  out_free:
 	return err;
 }
 
-ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
-		       size_t size)
+int ubifs_symlink_setxattr(struct dentry *dentry, const char *name,
+			   const void *value, size_t size, int flags)
 {
-	struct inode *inode, *host = dentry->d_inode;
+	struct inode *host = dentry->d_inode;
+	dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
+		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
+	return __ubifs_setxattr(host, name, value, size, flags);
+}
+
+int ubifs_setxattr(struct dentry *dentry, const char *name,
+		   const void *value, size_t size, int flags)
+{
+	struct inode *host = dentry->d_inode;
+	dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
+		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
+	return __ubifs_setxattr(host, name, value, size, flags);
+}
+
+static
+ssize_t __ubifs_getxattr(struct inode *host, const char *name, void *buf,
+			 size_t size)
+{
+	struct inode *inode;
 	struct ubifs_info *c = host->i_sb->s_fs_info;
 	struct qstr nm = { .name = name, .len = strlen(name) };
 	struct ubifs_inode *ui;
@@ -367,8 +388,8 @@  ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	union ubifs_key key;
 	int err;
 
-	dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
-		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
+	dbg_gen("xattr '%s', ino %lu  buf size %zd", name,
+		host->i_ino, size);
 
 	err = check_namespace(&nm);
 	if (err < 0)
@@ -416,6 +437,25 @@  out_unlock:
 	return err;
 }
 
+ssize_t ubifs_symlink_getxattr(struct dentry *dentry, const char *name,
+			       void *buf, size_t size)
+{
+	struct inode *host = dentry->d_inode;
+	dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
+		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
+	return __ubifs_getxattr(host, name, buf, size);
+}
+
+ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
+		       size_t size)
+{
+	struct inode *host = dentry->d_inode;
+	dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
+		host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
+	return __ubifs_getxattr(host, name, buf, size);
+}
+
+
 ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
 	union ubifs_key key;
@@ -568,3 +608,91 @@  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)
+{
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+	return __ubifs_getxattr(d->d_inode, name, buffer, size);
+}
+
+
+int ubifs_security_setxattr(struct dentry *d, const char *name,
+			    const void *value, size_t size,
+			    int flags, int handler_flags)
+{
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+	return __ubifs_setxattr(d->d_inode, 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)
+{
+	struct ubifs_inode *dir_ui = ubifs_inode(inode);
+	int err = 0;
+
+	mutex_lock(&inode->i_mutex);
+	mutex_lock(&dir_ui->ui_mutex);
+
+	err = security_inode_init_security(inode, dentry, qstr,
+					   &ubifs_initxattrs, 0);
+	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&dir_ui->ui_mutex);
+	return err;
+}