mbox series

[0/3] fixes for ubifs xattr operations

Message ID 20200630130438.141649-1-houtao1@huawei.com
Headers show
Series fixes for ubifs xattr operations | expand

Message

Hou Tao June 30, 2020, 1:04 p.m. UTC
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(-)

Comments

Richard Weinberger June 30, 2020, 1:15 p.m. UTC | #1
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
Hou Tao July 1, 2020, 1:11 a.m. UTC | #2
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
> .
>