[PATCH 0/3] fixes for ubifs xattr operations

[PATCH 0/3] fixes for ubifs xattr operations

[Hou Tao]

Hi,

The patch set tries to fix the race between ubifs xattr read
operations and write operations.

Now inode_lock() is acquired during xattr write ops (set & remove),
however no extra lock is taken in xattr read ops (list & get), and
it leads to three problems:

(1) ubifs_listxattr() may incur memory corruption and assertion failure

(2) ubifs_xattr_get() may incur assertion failure

(3) ubifs_xattr_get() may return a stale xattr value

To fix it, instead of adding a xattr-related rwsem for ubifs inode,
we decide to fix these problems simply and still do xattr read operation
locklessly. The major concern is that xattr read operations (list & get)
may return xattr names or values which is still in creation or removal
process, but we think that's OK because no stale name or value is
returned, just either the old ones or the new ones.

Comments are weclome.

Regards,
Tao

Hou Tao (3):
  ubifs: check the remaining name buffer during xattr list
  ubifs: protect assertion of xattr value size by ui_mutex during xattr
    get
  ubifs: ensure only one in-memory xattr inode is created

 fs/ubifs/xattr.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.25.0.4.g0ad7144999


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] ubifs: check the remaining name buffer during xattr list

[Hou Tao]

When there are concurrent xattr list and xattr write operations,
it is possible xattr_names + xattr_cnt has been increased a lot
by xattr write op since its last read in the begin of ubifs_listxattr().
So ubifs_listxattr() may find these newly updated or added xattrs,
try to copy these xattr names regardless of the remaing buffer size,
and lead to the corruption of buffer and assertion failure.

Simply fixing it by checking the remaining size of name buffer
before copying the xattr name.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/ubifs/xattr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 9aefbb60074f..5591b9fa1d86 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -429,6 +429,12 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		fname_len(&nm) = le16_to_cpu(xent->nlen);
 
 		if (xattr_visible(xent->name)) {
+			if (size - written < fname_len(&nm) + 1) {
+				kfree(pxent);
+				kfree(xent);
+				return -ERANGE;
+			}
+
 			memcpy(buffer + written, fname_name(&nm), fname_len(&nm) + 1);
 			written += fname_len(&nm) + 1;
 		}
-- 
2.25.0.4.g0ad7144999


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] ubifs: protect assertion of xattr value size by ui_mutex during xattr get

[Hou Tao]

ubifs_xattr_get() may race with change_xattr() which will
update inode->i_size and ui->data_len accordingly, and
it will fail the assertion: inode->i_size == ui->data_len,
so protect the assertion by ui_mutex.

For assertion: host_ui->xattr_size > ui->data_len, it can not been
ensured even both host_ui->ui_mutex and ui->ui_mutex are acquired,
because the xattr value may has been removed by remove_xattr() and
xattr_size has already been decreased, so just remove it.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/ubifs/xattr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 5591b9fa1d86..82be2c2d2db5 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -356,10 +356,9 @@ ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf,
 	}
 
 	ui = ubifs_inode(inode);
-	ubifs_assert(c, inode->i_size == ui->data_len);
-	ubifs_assert(c, ubifs_inode(host)->xattr_size > ui->data_len);
 
 	mutex_lock(&ui->ui_mutex);
+	ubifs_assert(c, inode->i_size == ui->data_len);
 	if (buf) {
 		/* If @buf is %NULL we are supposed to return the length */
 		if (ui->data_len > size) {
-- 
2.25.0.4.g0ad7144999


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] ubifs: ensure only one in-memory xattr inode is created

[Hou Tao]

ubifs may create two in-memory inodes for one xattr if
there are concurrent ubifs_xattr_get() and ubifs_xattr_set(),
as show in the following case:

ubifs_xattr_get()              ubifs_xattr_set()
                                 // the first created inode A
                                 // fill inode A
                                 ubifs_new_inode()
                                 ubifs_jnl_update()
                                   // mapping xattr name to inum
                                   ubifs_tnc_add_nm()
                                   // add xattr inode node
                                   ubifs_tnc_add()

  // find inum through xattr name
  ubifs_tnc_lookup_nm()
  iget_xattr()
    ubifs_iget()
      // not found in hash table
      // so create a new inode B
      // and keep it in hash table
      iget_locked()
      // find xattr inode node
      // fill inode B
      ubifs_tnc_lookup
      unlock_new_inode
                                 // inode A is also inserted into
                                 // hash table

If we update the xattr value afterwards, only the values in inode A will
be updated. So when we ty to remove the xattr name, and in the same
time get the xattr name, ubifs_xattr_get() may return the stale value
in inode B, as show in the following case:

ubifs_xattr_get()              ubifs_xattr_remove()

// get xattr inum
ubifs_tnc_lookup_nm()
				// return inode A
				iget_xattr()
				clear_nlink()
				remove_xattr()
				iput()
				  evict()
				    ubifs_evict_inode()
				    remove_inode_hash()
// return inode B
// return a stale xattr value
iget_xattr()

Fix it by moving insert_inode_hash() before ubifs_jnl_update(),
but after the initialization of inode is completed, so only
one inode is created for xattr value.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/ubifs/xattr.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 82be2c2d2db5..10fcb454bb01 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -133,6 +133,15 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 	inode->i_size = ui->ui_size = size;
 	ui->data_len = size;
 
+	/*
+	 * Ensure iget_xattr() in ubifs_xattr_get() will find the inode
+	 * instead of creating a new one.
+	 * The initialization of xattr inode is completed here, so using
+	 * insert_inode_hash() instead of insert_inode_locked(). The
+	 * latter can lead to iget_xattr() return -ESTALE.
+	 */
+	insert_inode_hash(inode);
+
 	mutex_lock(&host_ui->ui_mutex);
 	host->i_ctime = current_time(host);
 	host_ui->xattr_cnt += 1;
@@ -156,7 +165,6 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 	mutex_unlock(&host_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
-	insert_inode_hash(inode);
 	iput(inode);
 	return 0;
 
-- 
2.25.0.4.g0ad7144999


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Richard Weinberger]

Tao,

----- Ursprüngliche Mail -----
> Von: "Hou Tao" <houtao1@huawei.com>
> An: "richard" <richard@nod.at>, "linux-mtd" <linux-mtd@lists.infradead.org>
> CC: "Hou Tao" <houtao1@huawei.com>
> Gesendet: Dienstag, 30. Juni 2020 15:04:35
> Betreff: [PATCH 0/3] fixes for ubifs xattr operations

> Hi,
> 
> The patch set tries to fix the race between ubifs xattr read
> operations and write operations.
> 
> Now inode_lock() is acquired during xattr write ops (set & remove),
> however no extra lock is taken in xattr read ops (list & get), and
> it leads to three problems:
> 
> (1) ubifs_listxattr() may incur memory corruption and assertion failure
> 
> (2) ubifs_xattr_get() may incur assertion failure
> 
> (3) ubifs_xattr_get() may return a stale xattr value
> 
> To fix it, instead of adding a xattr-related rwsem for ubifs inode,
> we decide to fix these problems simply and still do xattr read operation
> locklessly. The major concern is that xattr read operations (list & get)
> may return xattr names or values which is still in creation or removal
> process, but we think that's OK because no stale name or value is
> returned, just either the old ones or the new ones.
> 
> Comments are weclome.

Thanks a lot for digging into this.
Do you have a test for this? (xfstest prefered).

I'm a bit surprised that this can actually happen, I was under the impression
that vfs cares about this.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Hou Tao]

Hi,

On 2020/6/30 21:15, Richard Weinberger wrote:
> Tao,
> 
> ----- Ursprüngliche Mail -----
>> Von: "Hou Tao" <houtao1@huawei.com>
>> An: "richard" <richard@nod.at>, "linux-mtd" <linux-mtd@lists.infradead.org>
>> CC: "Hou Tao" <houtao1@huawei.com>
>> Gesendet: Dienstag, 30. Juni 2020 15:04:35
>> Betreff: [PATCH 0/3] fixes for ubifs xattr operations
> 
>> Hi,
>>
>> The patch set tries to fix the race between ubifs xattr read
>> operations and write operations.
>>
>> Now inode_lock() is acquired during xattr write ops (set & remove),
>> however no extra lock is taken in xattr read ops (list & get), and
>> it leads to three problems:
>>
>> (1) ubifs_listxattr() may incur memory corruption and assertion failure
>>
>> (2) ubifs_xattr_get() may incur assertion failure
>>
>> (3) ubifs_xattr_get() may return a stale xattr value
>>
>> To fix it, instead of adding a xattr-related rwsem for ubifs inode,
>> we decide to fix these problems simply and still do xattr read operation
>> locklessly. The major concern is that xattr read operations (list & get)
>> may return xattr names or values which is still in creation or removal
>> process, but we think that's OK because no stale name or value is
>> returned, just either the old ones or the new ones.
>>
>> Comments are weclome.
> 
> Thanks a lot for digging into this.
> Do you have a test for this? (xfstest prefered).
> 
The first two problem can be reproduced relatively easily, and the third problem
is hard to reproduce (I do it through injecting delay in xattr ops), so I will
add xfstests for the first two problem firstly, then I will try to add an xfstests
for the last one. Maybe we can add a thin "infrastructure" layer to inject delay.

> I'm a bit surprised that this can actually happen, I was under the impression
> that vfs cares about this.
> 
Most filesystems handle the race by adding a rw-semaphore, but I think we can
simply do in a "lockless" way.

Regards,
Tao

> Thanks,
> //richard
> .
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Hou Tao]

Hi Richard,

I have written two xfstest cases [1] for these fixes three months ago, could you please
check whether the proposed solution is OK ?

Regards,
Tao

[1]: https://www.spinics.net/lists/fstests/msg14383.html

On 2020/6/30 21:04, Hou Tao wrote:
> Hi,
> 
> The patch set tries to fix the race between ubifs xattr read
> operations and write operations.
> 
> Now inode_lock() is acquired during xattr write ops (set & remove),
> however no extra lock is taken in xattr read ops (list & get), and
> it leads to three problems:
> 
> (1) ubifs_listxattr() may incur memory corruption and assertion failure
> 
> (2) ubifs_xattr_get() may incur assertion failure
> 
> (3) ubifs_xattr_get() may return a stale xattr value
> 
> To fix it, instead of adding a xattr-related rwsem for ubifs inode,
> we decide to fix these problems simply and still do xattr read operation
> locklessly. The major concern is that xattr read operations (list & get)
> may return xattr names or values which is still in creation or removal
> process, but we think that's OK because no stale name or value is
> returned, just either the old ones or the new ones.
> 
> Comments are weclome.
> 
> Regards,
> Tao
> 
> Hou Tao (3):
>   ubifs: check the remaining name buffer during xattr list
>   ubifs: protect assertion of xattr value size by ui_mutex during xattr
>     get
>   ubifs: ensure only one in-memory xattr inode is created
> 
>  fs/ubifs/xattr.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Richard Weinberger]

Tao,

On Fri, Oct 23, 2020 at 9:25 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Hi Richard,
>
> I have written two xfstest cases [1] for these fixes three months ago, could you please
> check whether the proposed solution is OK ?

Thanks for your time and reminding me about this issue!
I agree that this is a problem but I'm not sure whether the lockless
solution is the right path to go.
DId you try the locking approach?

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Hou Tao]

Hi,

On 2020/11/1 5:10, Richard Weinberger wrote:
> Tao,
> 
> On Fri, Oct 23, 2020 at 9:25 AM Hou Tao <houtao1@huawei.com> wrote:
>>
>> Hi Richard,
>>
>> I have written two xfstest cases [1] for these fixes three months ago, could you please
>> check whether the proposed solution is OK ?
> 
> Thanks for your time and reminding me about this issue!
> I agree that this is a problem but I'm not sure whether the lockless
> solution is the right path to go.
> DId you try the locking approach?
> 
Yes. Adding an extra lock for xattr ops also works. I will post a V2 soon.

Regards,
Tao

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Richard Weinberger]

On Tue, Nov 3, 2020 at 3:04 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Hi,
>
> On 2020/11/1 5:10, Richard Weinberger wrote:
> > Tao,
> >
> > On Fri, Oct 23, 2020 at 9:25 AM Hou Tao <houtao1@huawei.com> wrote:
> >>
> >> Hi Richard,
> >>
> >> I have written two xfstest cases [1] for these fixes three months ago, could you please
> >> check whether the proposed solution is OK ?
> >
> > Thanks for your time and reminding me about this issue!
> > I agree that this is a problem but I'm not sure whether the lockless
> > solution is the right path to go.
> > DId you try the locking approach?
> >
> Yes. Adding an extra lock for xattr ops also works. I will post a V2 soon.

Sounds promising!
Btw. you think of an extra rw lock because host->ui_mutex is too cause
grained and
would block xattr parallel read operations?

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] fixes for ubifs xattr operations

[Hou Tao]

Hi,

On 11/3/2020 4:19 PM, Richard Weinberger wrote:
> On Tue, Nov 3, 2020 at 3:04 AM Hou Tao <houtao1@huawei.com> wrote:
>> Hi,
>>
>> On 2020/11/1 5:10, Richard Weinberger wrote:
>>> Tao,
>>>
>>> On Fri, Oct 23, 2020 at 9:25 AM Hou Tao <houtao1@huawei.com> wrote:
>>>> Hi Richard,
>>>>
>>>> I have written two xfstest cases [1] for these fixes three months ago, could you please
>>>> check whether the proposed solution is OK ?
>>> Thanks for your time and reminding me about this issue!
>>> I agree that this is a problem but I'm not sure whether the lockless
>>> solution is the right path to go.
>>> DId you try the locking approach?
>>>
>> Yes. Adding an extra lock for xattr ops also works. I will post a V2 soon.
> Sounds promising!
Sorry for the long delay.
> Btw. you think of an extra rw lock because host->ui_mutex is too cause
> grained and
> would block xattr parallel read operations?

Yes, the locking order is another concern. But we can change host->ui_mutex to a rw lock

if both the locking order and the granularity are OK. I will check both.

Regards,

Tao


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/