| Message ID | 20200630130438.141649-1-houtao1@huawei.com |
|---|---|
| Headers | show |
| Series | fixes for ubifs xattr operations | expand |
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
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 > . >
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(-) >
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?
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
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?
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
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(-)