Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] fixes for ubifs xattr operations
@ 2020-06-30 13:04 Hou Tao
  2020-06-30 13:04 ` [PATCH 1/3] ubifs: check the remaining name buffer during xattr list Hou Tao
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Hou Tao @ 2020-06-30 13:04 UTC (permalink / raw)
  To: Richard Weinberger, linux-mtd; +Cc: houtao1

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] ubifs: check the remaining name buffer during xattr list
  2020-06-30 13:04 [PATCH 0/3] fixes for ubifs xattr operations Hou Tao
@ 2020-06-30 13:04 ` Hou Tao
  2020-06-30 13:04 ` [PATCH 2/3] ubifs: protect assertion of xattr value size by ui_mutex during xattr get Hou Tao
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2020-06-30 13:04 UTC (permalink / raw)
  To: Richard Weinberger, linux-mtd; +Cc: houtao1

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] ubifs: protect assertion of xattr value size by ui_mutex during xattr get
  2020-06-30 13:04 [PATCH 0/3] fixes for ubifs xattr operations Hou Tao
  2020-06-30 13:04 ` [PATCH 1/3] ubifs: check the remaining name buffer during xattr list Hou Tao
@ 2020-06-30 13:04 ` Hou Tao
  2020-06-30 13:04 ` [PATCH 3/3] ubifs: ensure only one in-memory xattr inode is created Hou Tao
  2020-06-30 13:15 ` [PATCH 0/3] fixes for ubifs xattr operations Richard Weinberger
  3 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2020-06-30 13:04 UTC (permalink / raw)
  To: Richard Weinberger, linux-mtd; +Cc: houtao1

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] ubifs: ensure only one in-memory xattr inode is created
  2020-06-30 13:04 [PATCH 0/3] fixes for ubifs xattr operations Hou Tao
  2020-06-30 13:04 ` [PATCH 1/3] ubifs: check the remaining name buffer during xattr list Hou Tao
  2020-06-30 13:04 ` [PATCH 2/3] ubifs: protect assertion of xattr value size by ui_mutex during xattr get Hou Tao
@ 2020-06-30 13:04 ` Hou Tao
  2020-06-30 13:15 ` [PATCH 0/3] fixes for ubifs xattr operations Richard Weinberger
  3 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2020-06-30 13:04 UTC (permalink / raw)
  To: Richard Weinberger, linux-mtd; +Cc: houtao1

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] fixes for ubifs xattr operations
  2020-06-30 13:04 [PATCH 0/3] fixes for ubifs xattr operations Hou Tao
                   ` (2 preceding siblings ...)
  2020-06-30 13:04 ` [PATCH 3/3] ubifs: ensure only one in-memory xattr inode is created Hou Tao
@ 2020-06-30 13:15 ` Richard Weinberger
  2020-07-01  1:11   ` Hou Tao
  3 siblings, 1 reply; 6+ messages in thread
From: Richard Weinberger @ 2020-06-30 13:15 UTC (permalink / raw)
  To: Hou Tao; +Cc: linux-mtd

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] fixes for ubifs xattr operations
  2020-06-30 13:15 ` [PATCH 0/3] fixes for ubifs xattr operations Richard Weinberger
@ 2020-07-01  1:11   ` Hou Tao
  0 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2020-07-01  1:11 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-mtd

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 13:04 [PATCH 0/3] fixes for ubifs xattr operations Hou Tao
2020-06-30 13:04 ` [PATCH 1/3] ubifs: check the remaining name buffer during xattr list Hou Tao
2020-06-30 13:04 ` [PATCH 2/3] ubifs: protect assertion of xattr value size by ui_mutex during xattr get Hou Tao
2020-06-30 13:04 ` [PATCH 3/3] ubifs: ensure only one in-memory xattr inode is created Hou Tao
2020-06-30 13:15 ` [PATCH 0/3] fixes for ubifs xattr operations Richard Weinberger
2020-07-01  1:11   ` Hou Tao

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git