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

[PATCH v4] 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
	-v3
		move kmemdup out of lock window.
 fs/ubifs/xattr.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 96f3448..99364ae 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -200,6 +200,7 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	int err;
 	struct ubifs_inode *host_ui = ubifs_inode(host);
 	struct ubifs_inode *ui = ubifs_inode(inode);
+	void *buf = NULL;
 	struct ubifs_budget_req req = { .dirtied_ino = 2,
 		.dirtied_ino_d = ALIGN(size, 8) + ALIGN(host_ui->data_len, 8) };
 
@@ -208,14 +209,17 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
 	if (err)
 		return err;
 
-	kfree(ui->data);
-	ui->data = kmemdup(value, size, GFP_NOFS);
-	if (!ui->data) {
+	buf = kmemdup(value, size, GFP_NOFS);
+	if (!buf) {
 		err = -ENOMEM;
 		goto out_free;
 	}
+	mutex_lock(&ui->ui_mutex);
+	kfree(ui->data);
+	ui->data = buf;
 	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 +413,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 +428,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 v4] ubifs: make ubifs_[get|set]xattr atomic

[Artem Bityutskiy]

On Tue, 2015-08-18 at 12:38 +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()
> 
> Then ubifs_getxattr() would get a non-sense data. To solve this
> problem, this commit make the xattr of ubifs_inode updated in
> atomic.

Picked this one, thanks!

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

[Dongsheng Yang]

On 08/18/2015 04:43 PM, Artem Bityutskiy wrote:
> On Tue, 2015-08-18 at 12:38 +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()
>>
>> Then ubifs_getxattr() would get a non-sense data. To solve this
>> problem, this commit make the xattr of ubifs_inode updated in
>> atomic.
>
> Picked this one, thanks!

Hi Artem and Richard,
	I did not find this one in linux-ubifs.git, is that
planed for 4.4?

Yang

> .
>

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

[Richard Weinberger]

Am 07.11.2015 um 05:36 schrieb Dongsheng Yang:
> On 08/18/2015 04:43 PM, Artem Bityutskiy wrote:
>> On Tue, 2015-08-18 at 12:38 +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()
>>>
>>> Then ubifs_getxattr() would get a non-sense data. To solve this
>>> problem, this commit make the xattr of ubifs_inode updated in
>>> atomic.
>>
>> Picked this one, thanks!
> 
> Hi Artem and Richard,
>     I did not find this one in linux-ubifs.git, is that
> planed for 4.4?

Hmm, it should. Artem, forgot to push?
Anyway, I've added it now to linux-ubifs.git master.

Thanks,
//richard