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