| Message ID | tencent_8C1ACE487B4E6C302EE56D8C95C0E8E2EF0A@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hfs: fix deadlock in hfs_extend_file | expand |
On Tue, Jan 02, 2024 at 08:36:51PM +0800, Edward Adam Davis wrote: > [syz report] > syz-executor279/5059 is trying to acquire lock: > ffff888079c100f8 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 > > but task is already holding lock: > ffff888079c10778 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&HFS_I(tree->inode)->extents_lock); > lock(&HFS_I(tree->inode)->extents_lock); > > *** DEADLOCK *** > [Analysis] > hfs_extend_file()-> > hfs_ext_read_extent()-> > __hfs_ext_cache_extent()-> > __hfs_ext_write_extent()-> > hfs_bmap_reserve()-> > hfs_extend_file()-> > > When an inode has both the HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW flags, it will > enter the above loop and trigger a deadlock. > > [Fix] > In hfs_ext_read_extent(), check if the above two flags exist simultaneously, > and exit the subsequent process when the conditions are met. Why is this the correct fix? Seems to me that returning -ENOENT here is going to lead to an error being reported to the user when the user has done nothing wrong? > Reported-and-tested-by: syzbot+41a88b825a315aac2254@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/hfs/extent.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c > index 6d1878b99b30..1b02c7b6a10c 100644 > --- a/fs/hfs/extent.c > +++ b/fs/hfs/extent.c > @@ -197,6 +197,10 @@ static int hfs_ext_read_extent(struct inode *inode, u16 block) > block < HFS_I(inode)->cached_start + HFS_I(inode)->cached_blocks) > return 0; > > + if (HFS_I(inode)->flags & HFS_FLG_EXT_DIRTY && > + HFS_I(inode)->flags & HFS_FLG_EXT_NEW) > + return -ENOENT; > + > res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd); > if (!res) { > res = __hfs_ext_cache_extent(&fd, inode, block); > -- > 2.43.0 > >
> On Jan 2, 2024, at 3:36 PM, Edward Adam Davis <eadavis@qq.com> wrote: > > [syz report] > syz-executor279/5059 is trying to acquire lock: > ffff888079c100f8 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 > > but task is already holding lock: > ffff888079c10778 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&HFS_I(tree->inode)->extents_lock); > lock(&HFS_I(tree->inode)->extents_lock); > > *** DEADLOCK *** > [Analysis] > hfs_extend_file()-> > hfs_ext_read_extent()-> > __hfs_ext_cache_extent()-> > __hfs_ext_write_extent()-> > hfs_bmap_reserve()-> > hfs_extend_file()-> > > When an inode has both the HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW flags, it will > enter the above loop and trigger a deadlock. > > [Fix] > In hfs_ext_read_extent(), check if the above two flags exist simultaneously, > and exit the subsequent process when the conditions are met. > > Reported-and-tested-by: syzbot+41a88b825a315aac2254@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/hfs/extent.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c > index 6d1878b99b30..1b02c7b6a10c 100644 > --- a/fs/hfs/extent.c > +++ b/fs/hfs/extent.c > @@ -197,6 +197,10 @@ static int hfs_ext_read_extent(struct inode *inode, u16 block) > block < HFS_I(inode)->cached_start + HFS_I(inode)->cached_blocks) > return 0; > > + if (HFS_I(inode)->flags & HFS_FLG_EXT_DIRTY && > + HFS_I(inode)->flags & HFS_FLG_EXT_NEW) > + return -ENOENT; > + I don’t think that fix can be so simple. It looks like the code requires significant refactoring. Because, currently, it looks like bad recursion: hfs_extend_file() finally calls hfs_extend_file(). And it smells really badly. Also, from the logical point of view, hfs_ext_read_extent() method calls __hfs_ext_write_extent() that sounds like not good logic. I believe we need more serious refactoring of hfs_extend_file() logic. Potentially, hfs_extend_file() can check that we have HFS_FLG_EXT_DIRTY and execute logic of write extent without calling himself again. But I haven’t clear picture of necessary refactoring efforts yet. Thanks, Slava.
diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c index 6d1878b99b30..1b02c7b6a10c 100644 --- a/fs/hfs/extent.c +++ b/fs/hfs/extent.c @@ -197,6 +197,10 @@ static int hfs_ext_read_extent(struct inode *inode, u16 block) block < HFS_I(inode)->cached_start + HFS_I(inode)->cached_blocks) return 0; + if (HFS_I(inode)->flags & HFS_FLG_EXT_DIRTY && + HFS_I(inode)->flags & HFS_FLG_EXT_NEW) + return -ENOENT; + res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd); if (!res) { res = __hfs_ext_cache_extent(&fd, inode, block);
[syz report] syz-executor279/5059 is trying to acquire lock: ffff888079c100f8 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 but task is already holding lock: ffff888079c10778 (&HFS_I(tree->inode)->extents_lock){+.+.}-{3:3}, at: hfs_extend_file+0xa2/0xb10 fs/hfs/extent.c:397 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&HFS_I(tree->inode)->extents_lock); lock(&HFS_I(tree->inode)->extents_lock); *** DEADLOCK *** [Analysis] hfs_extend_file()-> hfs_ext_read_extent()-> __hfs_ext_cache_extent()-> __hfs_ext_write_extent()-> hfs_bmap_reserve()-> hfs_extend_file()-> When an inode has both the HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW flags, it will enter the above loop and trigger a deadlock. [Fix] In hfs_ext_read_extent(), check if the above two flags exist simultaneously, and exit the subsequent process when the conditions are met. Reported-and-tested-by: syzbot+41a88b825a315aac2254@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/hfs/extent.c | 4 ++++ 1 file changed, 4 insertions(+)