[PATCH] ubifs: make ubifs_[get|set]xattr atomic

[PATCH] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

This commit make the ubifs_[get|set]xattr protected by ui_mutex,
making xfstests/generic/037 passed.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/ubifs/xattr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 96f3448..dec1afd 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	if (err)
 		return err;
 
+	mutex_lock(&ui->ui_mutex);
 	kfree(ui->data);
 	ui->data = kmemdup(value, size, GFP_NOFS);
 	if (!ui->data) {
@@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	}
 	inode->i_size = ui->ui_size = size;
 	ui->data_len = size;
+	mutex_unlock(&ui->ui_mutex);
 
 	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = ubifs_current_time(host);
@@ -409,6 +411,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	ubifs_assert(inode->i_size == ui->data_len);
 	ubifs_assert(ubifs_inode(host)->xattr_size > ui->data_len);
 
+	mutex_lock(&ui->ui_mutex);
 	if (buf) {
 		/* If @buf is %NULL we are supposed to return the length */
 		if (ui->data_len > size) {
@@ -423,6 +426,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	err = ui->data_len;
 
 out_iput:
+	mutex_unlock(&ui->ui_mutex);
 	iput(inode);
 out_unlock:
 	kfree(xent);
-- 
1.8.4.2

Re: [PATCH] ubifs: make ubifs_[get|set]xattr atomic

[Richard Weinberger]

Am 30.07.2015 um 07:37 schrieb Dongsheng Yang:
> This commit make the ubifs_[get|set]xattr protected by ui_mutex,
> making xfstests/generic/037 passed.

Making a test pass is not a good patch description.
Please describe what needs protection and why. :-)

Thanks,
//richard

Re: [PATCH] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

On 07/30/2015 04:05 PM, Richard Weinberger wrote:
> Am 30.07.2015 um 07:37 schrieb Dongsheng Yang:
>> This commit make the ubifs_[get|set]xattr protected by ui_mutex,
>> making xfstests/generic/037 passed.
>
> Making a test pass is not a good patch description.
> Please describe what needs protection and why. :-)

Agreed, sorry for my lazy :)

Will send a V2 with more description.

Thanx
Yang
>
> Thanks,
> //richard
>

[PATCH v2] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

This commit make the ubifs_[get|set]xattr protected by ui_mutex.

Originally, there is a possibility that ubifs_getxattr to get
a wrong value.

    P1				    P2
----------			----------
ubifs_getxattr			ubifs_setxattr
				   - kfree()
   - memcpy()
				   - kmemdup()

Then ubifs_getxattr() would get a non-sense data. To solve this
problem, this commit make the xattr of ubifs_inode updated in
atomic.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
	-v2:
		Add more description in commit message
 fs/ubifs/xattr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 96f3448..dec1afd 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	if (err)
 		return err;
 
+	mutex_lock(&ui->ui_mutex);
 	kfree(ui->data);
 	ui->data = kmemdup(value, size, GFP_NOFS);
 	if (!ui->data) {
@@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	}
 	inode->i_size = ui->ui_size = size;
 	ui->data_len = size;
+	mutex_unlock(&ui->ui_mutex);
 
 	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = ubifs_current_time(host);
@@ -409,6 +411,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	ubifs_assert(inode->i_size == ui->data_len);
 	ubifs_assert(ubifs_inode(host)->xattr_size > ui->data_len);
 
+	mutex_lock(&ui->ui_mutex);
 	if (buf) {
 		/* If @buf is %NULL we are supposed to return the length */
 		if (ui->data_len > size) {
@@ -423,6 +426,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	err = ui->data_len;
 
 out_iput:
+	mutex_unlock(&ui->ui_mutex);
 	iput(inode);
 out_unlock:
 	kfree(xent);
-- 
1.8.4.2

Re: [PATCH v2] ubifs: make ubifs_[get|set]xattr atomic

[Richard Weinberger]

Am 31.07.2015 um 03:12 schrieb Dongsheng Yang:
> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
> 
> Originally, there is a possibility that ubifs_getxattr to get
> a wrong value.
> 
>     P1				    P2
> ----------			----------
> ubifs_getxattr			ubifs_setxattr
> 				   - kfree()
>    - memcpy()
> 				   - kmemdup()
> 
> Then ubifs_getxattr() would get a non-sense data. To solve this
> problem, this commit make the xattr of ubifs_inode updated in
> atomic.

so, ui->data needs protection?
The comment in fs/ubifs/ubifs.h does not mention ->data.
I'm asking because I want to make sure that ui_mutex is the correct lock to take.

> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
> 	-v2:
> 		Add more description in commit message
>  fs/ubifs/xattr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 96f3448..dec1afd 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
>  	if (err)
>  		return err;
>  
> +	mutex_lock(&ui->ui_mutex);
>  	kfree(ui->data);
>  	ui->data = kmemdup(value, size, GFP_NOFS);
>  	if (!ui->data) {
> @@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
>  	}
>  	inode->i_size = ui->ui_size = size;
>  	ui->data_len = size;
> +	mutex_unlock(&ui->ui_mutex);

You also need a mutex_unlock(&ui->ui_mutex) under out_free, otherwise
the if (!ui->data) { branch will trigger a deadlock.

Thanks,
//richard

Re: [PATCH v2] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

On 08/04/2015 04:27 AM, Richard Weinberger wrote:
> Am 31.07.2015 um 03:12 schrieb Dongsheng Yang:
>> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
>>
>> Originally, there is a possibility that ubifs_getxattr to get
>> a wrong value.
>>
>>      P1				    P2
>> ----------			----------
>> ubifs_getxattr			ubifs_setxattr
>> 				   - kfree()
>>     - memcpy()
>> 				   - kmemdup()
>>
>> Then ubifs_getxattr() would get a non-sense data. To solve this
>> problem, this commit make the xattr of ubifs_inode updated in
>> atomic.
>
> so, ui->data needs protection?
> The comment in fs/ubifs/ubifs.h does not mention ->data.
> I'm asking because I want to make sure that ui_mutex is the correct lock to take.

ui->data needs protection for sure, as I show above.
Without protection, there is a problem to get wrong value.

And yes, the comment does not mention the ->data. But
I think ui_mutex is a good choice for it. And not all fields
which is being protected by ui_mutex are listed in the comment,
such as xattr_names and xattr_cnt.
>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>> 	-v2:
>> 		Add more description in commit message
>>   fs/ubifs/xattr.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>> index 96f3448..dec1afd 100644
>> --- a/fs/ubifs/xattr.c
>> +++ b/fs/ubifs/xattr.c
>> @@ -208,6 +208,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
>>   	if (err)
>>   		return err;
>>
>> +	mutex_lock(&ui->ui_mutex);
>>   	kfree(ui->data);
>>   	ui->data = kmemdup(value, size, GFP_NOFS);
>>   	if (!ui->data) {
>> @@ -216,6 +217,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
>>   	}
>>   	inode->i_size = ui->ui_size = size;
>>   	ui->data_len = size;
>> +	mutex_unlock(&ui->ui_mutex);
>
> You also need a mutex_unlock(&ui->ui_mutex) under out_free, otherwise
> the if (!ui->data) { branch will trigger a deadlock.

Wow, Great!!!!
Sorry for my careless.

Yang
>
> Thanks,
> //richard
> .
>

[PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

This commit make the ubifs_[get|set]xattr protected by ui_mutex.

Originally, there is a possibility that ubifs_getxattr to get
a wrong value.

  P1                                  P2
----------                  	----------
ubifs_getxattr                      ubifs_setxattr
					- kfree()
- memcpy()
					- kmemdup()

Then ubifs_getxattr() would get a non-sense data. To solve this
problem, this commit make the xattr of ubifs_inode updated in
atomic.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
	-v2
		fix a dead lock in error flow
 fs/ubifs/xattr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 96f3448..33badeb 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -208,14 +208,17 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	if (err)
 		return err;
 
+	mutex_lock(&ui->ui_mutex);
 	kfree(ui->data);
 	ui->data = kmemdup(value, size, GFP_NOFS);
 	if (!ui->data) {
+		mutex_unlock(&ui->ui_mutex);
 		err = -ENOMEM;
 		goto out_free;
 	}
 	inode->i_size = ui->ui_size = size;
 	ui->data_len = size;
+	mutex_unlock(&ui->ui_mutex);
 
 	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = ubifs_current_time(host);
@@ -409,6 +412,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	ubifs_assert(inode->i_size == ui->data_len);
 	ubifs_assert(ubifs_inode(host)->xattr_size > ui->data_len);
 
+	mutex_lock(&ui->ui_mutex);
 	if (buf) {
 		/* If @buf is %NULL we are supposed to return the length */
 		if (ui->data_len > size) {
@@ -423,6 +427,7 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
 	err = ui->data_len;
 
 out_iput:
+	mutex_unlock(&ui->ui_mutex);
 	iput(inode);
 out_unlock:
 	kfree(xent);
-- 
1.8.4.2

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Artem Bityutskiy]

Right now, AFAICS, UBIFS uses the "host" inode mutex to serialize xattr
operations. May be the terminology is not the best, but "host" is the
non-xattr inode (say, a file inode) which "carries" the extended
attributes. So one "host" may have many xattrs.

Now, obviously, locking the entire "host" to change one xattr is very
coarse, because operations on other xattrs get blocked by operations on
other, unrelated xattrs.

So the host inode has fields like "xattr_size" - this needs to be
protected with the host mutex, but operations on individual xattrs may
be protected by xattr mutexes. However, this would be a more complex
locking scheme.


So my point is - use the "host->ui_mutex", not the xattr's mutex,
because this is how it is implemented now.

Re-working and improving locking could be a separate piece of job.

On Fri, 2015-08-07 at 14:07 +0800, Dongsheng Yang wrote:
> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
> 
> Originally, there is a possibility that ubifs_getxattr to get
> a wrong value.
> 
>   P1                                  P2
> ----------                  	----------
> ubifs_getxattr                      ubifs_setxattr
> 					- kfree()
> - memcpy()
> 					- kmemdup()
> 

...

>  
> +	mutex_lock(&ui->ui_mutex);

Or to put it differently, you need to use 'host->ui_mutex', not 'ui
->ui_mutex', because 'ui' is the xattr inode here.

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

On 08/10/2015 04:05 PM, Artem Bityutskiy wrote:
> Right now, AFAICS, UBIFS uses the "host" inode mutex to serialize xattr
> operations. May be the terminology is not the best, but "host" is the
> non-xattr inode (say, a file inode) which "carries" the extended
> attributes. So one "host" may have many xattrs.
>
> Now, obviously, locking the entire "host" to change one xattr is very
> coarse, because operations on other xattrs get blocked by operations on
> other, unrelated xattrs.
>
> So the host inode has fields like "xattr_size" - this needs to be
> protected with the host mutex, but operations on individual xattrs may
> be protected by xattr mutexes. However, this would be a more complex
> locking scheme.

Hi Atem, thanx for your explanation. :)

But I am afraid the current locking scheme is what you want that:

(host inode) xattr_size, xattr_cnt, xattr_names ------- protected by 
host->ui_mutex
(xattr inode) data ------ protected by ui->ui_mutex (with my patch applied)

So, I think fields in host are all protected by host->ui_mutex and
fields in xattr inodes are all protected by individual ui_mutex.

Am I missing something?

BTW, I found I have to add ui_mutex in create_xattr() even if you
agree with it. :)

Yang
>
>
> So my point is - use the "host->ui_mutex", not the xattr's mutex,
> because this is how it is implemented now.
>
> Re-working and improving locking could be a separate piece of job.
>
> On Fri, 2015-08-07 at 14:07 +0800, Dongsheng Yang wrote:
>> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
>>
>> Originally, there is a possibility that ubifs_getxattr to get
>> a wrong value.
>>
>>    P1                                  P2
>> ----------                  	----------
>> ubifs_getxattr                      ubifs_setxattr
>> 					- kfree()
>> - memcpy()
>> 					- kmemdup()
>>
>
> ...
>
>>
>> +	mutex_lock(&ui->ui_mutex);
>
> Or to put it differently, you need to use 'host->ui_mutex', not 'ui
> ->ui_mutex', because 'ui' is the xattr inode here.
>
> .
>

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Artem Bityutskiy]

On Tue, 2015-08-11 at 11:32 +0800, Dongsheng Yang wrote:
> So, I think fields in host are all protected by host->ui_mutex and
> fields in xattr inodes are all protected by individual ui_mutex.
> 
> Am I missing something?

OK, on a second look, your patch is probably fine.

> BTW, I found I have to add ui_mutex in create_xattr() even if you
> agree with it. :)

I am not sure about this. I thought xattrs are not visible to anyone
while they are being created, so you do not need any locking there,
since you do not have anyone to race with you.

Artem/

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

On 08/11/2015 08:46 PM, Artem Bityutskiy wrote:
> On Tue, 2015-08-11 at 11:32 +0800, Dongsheng Yang wrote:
>> So, I think fields in host are all protected by host->ui_mutex and
>> fields in xattr inodes are all protected by individual ui_mutex.
>>
>> Am I missing something?
>
> OK, on a second look, your patch is probably fine.
>
>> BTW, I found I have to add ui_mutex in create_xattr() even if you
>> agree with it. :)
>
> I am not sure about this. I thought xattrs are not visible to anyone
> while they are being created, so you do not need any locking there,
> since you do not have anyone to race with you.

Makes sense.

Yang
>
> Artem/
>
>
>

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Artem Bityutskiy]

On Fri, 2015-08-07 at 14:07 +0800, Dongsheng Yang wrote:
> +	mutex_lock(&ui->ui_mutex);
>  	kfree(ui->data);
>  	ui->data = kmemdup(value, size, GFP_NOFS);
>  	if (!ui->data) {
> +		mutex_unlock(&ui->ui_mutex);
>  		err = -ENOMEM;
>  		goto out_free;
>  	}
>  	inode->i_size = ui->ui_size = size;
>  	ui->data_len = size;
> +	mutex_unlock(&ui->ui_mutex);


Just one note - should kmemdup() be called outside of the critical
section, to make is shorter (memory allocation is a potentially slow
operation)?

Something like:

buf = kmemdup()
mutex_lock()
kfree(ui->data)
ui->data = buf
mutex_unlock()

Artem.

Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang]

On 08/11/2015 08:57 PM, Artem Bityutskiy wrote:
> On Fri, 2015-08-07 at 14:07 +0800, Dongsheng Yang wrote:
>> +	mutex_lock(&ui->ui_mutex);
>>   	kfree(ui->data);
>>   	ui->data = kmemdup(value, size, GFP_NOFS);
>>   	if (!ui->data) {
>> +		mutex_unlock(&ui->ui_mutex);
>>   		err = -ENOMEM;
>>   		goto out_free;
>>   	}
>>   	inode->i_size = ui->ui_size = size;
>>   	ui->data_len = size;
>> +	mutex_unlock(&ui->ui_mutex);
>
>
> Just one note - should kmemdup() be called outside of the critical
> section, to make is shorter (memory allocation is a potentially slow
> operation)?
>
> Something like:
>
> buf = kmemdup()
> mutex_lock()
> kfree(ui->data)
> ui->data = buf
> mutex_unlock()

Good point, will update it.

Yang
>
> Artem.
>

Re: [PATCH v2] ubifs: make ubifs_[get|set]xattr atomic

[Richard Weinberger]

Am 07.08.2015 um 07:40 schrieb Dongsheng Yang:
> On 08/04/2015 04:27 AM, Richard Weinberger wrote:
>> Am 31.07.2015 um 03:12 schrieb Dongsheng Yang:
>>> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
>>>
>>> Originally, there is a possibility that ubifs_getxattr to get
>>> a wrong value.
>>>
>>>      P1                    P2
>>> ----------            ----------
>>> ubifs_getxattr            ubifs_setxattr
>>>                    - kfree()
>>>     - memcpy()
>>>                    - kmemdup()
>>>
>>> Then ubifs_getxattr() would get a non-sense data. To solve this
>>> problem, this commit make the xattr of ubifs_inode updated in
>>> atomic.
>>
>> so, ui->data needs protection?
>> The comment in fs/ubifs/ubifs.h does not mention ->data.
>> I'm asking because I want to make sure that ui_mutex is the correct lock to take.
> 
> ui->data needs protection for sure, as I show above.
> Without protection, there is a problem to get wrong value.
> 
> And yes, the comment does not mention the ->data. But
> I think ui_mutex is a good choice for it. And not all fields
> which is being protected by ui_mutex are listed in the comment,
> such as xattr_names and xattr_cnt.

Yeah. ;-\
The comment needs an update. In UBI and UBIFS we try hard to document our locking.
Will you do a patch or shall I?

Thanks,
//richard