Message ID | 20241227130717.144988-1-dmantipov@yandex.ru (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | ocfs2: uncache inode after ocfs2_search_dirblock() search failure | expand |
On 2024/12/27 21:07, Dmitry Antipov wrote: > Syzbot has reported the following BUG: > > kernel BUG at fs/ocfs2/uptodate.c:509! > ... > Call Trace: > <TASK> > ? __die_body+0x61/0xb0 > ? die+0x9f/0xc0 > ? do_trap+0x14a/0x3d0 > ? ocfs2_set_new_buffer_uptodate+0x149/0x170 > ? do_error_trap+0x1c2/0x280 > ? ocfs2_set_new_buffer_uptodate+0x149/0x170 > ? __pfx_do_error_trap+0x10/0x10 > ? handle_invalid_op+0x34/0x40 > ? ocfs2_set_new_buffer_uptodate+0x149/0x170 > ? exc_invalid_op+0x39/0x50 > ? asm_exc_invalid_op+0x1a/0x20 > ? ocfs2_set_new_buffer_uptodate+0x148/0x170 > ? ocfs2_set_new_buffer_uptodate+0x149/0x170 > ? ocfs2_set_new_buffer_uptodate+0x148/0x170 > ocfs2_group_add+0x318/0x1240 > ? mnt_get_write_access+0x68/0x2a0 > ? __pfx_ocfs2_group_add+0x10/0x10 > ? mnt_get_write_access+0x68/0x2a0 > ? __pfx_lock_release+0x10/0x10 > ? rcu_read_lock_any_held+0xb4/0x130 > ? __pfx_rcu_read_lock_any_held+0x10/0x10 > ? __pfx_lock_release+0x10/0x10 > ? mnt_get_write_access+0x223/0x2a0 > ? mnt_want_write_file+0x172/0x200 > ocfs2_ioctl+0x60b/0x750 > ? __pfx_ocfs2_ioctl+0x10/0x10 > ? lockdep_hardirqs_on_prepare+0x401/0x750 > ? bpf_lsm_file_ioctl+0x9/0x20 > ? __pfx_ocfs2_ioctl+0x10/0x10 > __se_sys_ioctl+0xfc/0x170 > do_syscall_64+0xf6/0x210 > ? exc_page_fault+0x92/0x110 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > ... > </TASK> > > In 'ocfs2_find_entry_id()', the buffer head remains cached (by > 'ocfs2_read_blocks()') regardless of the search result returned > by 'ocfs2_search_dirblock()'. In case of search failure, that > buffer head is no longer used and should be uncached before > calling 'brelse()'. This is very similar to commit 737f34137844 > ("ocfs2: uncache inode which has failed entering the group"). > Hi, don't see the relationship between above logic and the triggered bug. Could you please elobrate more how it happens? Thanks, Joseph > Reported-by: syzbot+7aef76bdb53b83d62a9e@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=7aef76bdb53b83d62a9e > Fixes: 23193e513d1c ("ocfs2: Read support for directories with inline data") > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > fs/ocfs2/dir.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index 213206ebdd58..3bfbc166d1fe 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -416,6 +416,9 @@ static struct buffer_head *ocfs2_find_entry_id(const char *name, > if (found == 1) > return di_bh; > > + /* Cached by ocfs2_read_blocks() so remove it. */ > + ocfs2_remove_from_cache(INODE_CACHE(dir), di_bh); > + > brelse(di_bh); > out: > return NULL;
On 12/28/24 3:27 PM, Joseph Qi wrote: > Hi, don't see the relationship between above logic and the triggered > bug. > Could you please elobrate more how it happens? As triggered by the syzbot's reproducer, the problem is initiated by 'mknodat()'... -> mknodat() ... -> filename_create() ... -> ocfs2_lookup() -> ocfs2_find_files_on_disk() -> ocfs2_find_entry() -> ocfs2_find_entry_id() ... -> ocfs2_read_inode_block() -> ocfs2_read_blocks() -> ocfs2_set_buffer_uptodate() ;; [1] Buffer head is now cached ... if (found == 1) return di_bh; ;; Buffer head will be used /* Hmm..... */ ;; [2] brelse(di_bh); ;; [3] Buffer head is now unused ...with followed 'ioctl()': -> ioctl(..., OCFS2_IOC_GROUP_ADD, ...) -> ocfs2_ioctl() -> ocfs2_group_add() -> ocfs2_read_blocks_sync() -> ocfs2_set_new_buffer_uptodate() ;; OOPS [4] OOPS at [4] happens becase the corresponding buffer head is not actually "new". It was "new" at [1] but was not exprunged from the inode cache before 'brelse()' at [3]. I.e. OCFS2 cache has an inconsistent view of the data provided by the block layer, and it is expected that adding 'ocfs2_remove_from_cache()' at [2] should fix this problem. Dmitry
On 2024/12/29 00:33, Dmitry Antipov wrote: > On 12/28/24 3:27 PM, Joseph Qi wrote: > >> Hi, don't see the relationship between above logic and the triggered >> bug. >> Could you please elobrate more how it happens? > > As triggered by the syzbot's reproducer, the problem is initiated by > 'mknodat()'... > > -> mknodat() > ... > -> filename_create() > ... > -> ocfs2_lookup() > -> ocfs2_find_files_on_disk() > -> ocfs2_find_entry() > -> ocfs2_find_entry_id() > ... > -> ocfs2_read_inode_block() > -> ocfs2_read_blocks() > -> ocfs2_set_buffer_uptodate() ;; [1] Buffer head is now cached > ... > if (found == 1) > return di_bh; ;; Buffer head will be used > /* Hmm..... */ ;; [2] > brelse(di_bh); ;; [3] Buffer head is now unused > > ...with followed 'ioctl()': > > -> ioctl(..., OCFS2_IOC_GROUP_ADD, ...) > -> ocfs2_ioctl() > -> ocfs2_group_add() > -> ocfs2_read_blocks_sync() > -> ocfs2_set_new_buffer_uptodate() ;; OOPS [4] This is a group bh, but the above bh in ocfs2_find_entry() seems not. > > OOPS at [4] happens becase the corresponding buffer head is not actually > "new". It was "new" at [1] but was not exprunged from the inode cache > before 'brelse()' at [3]. I.e. OCFS2 cache has an inconsistent view of > the data provided by the block layer, and it is expected that adding > 'ocfs2_remove_from_cache()' at [2] should fix this problem. > Even if found, the returned di_bh will also be brelsed in ocfs2_free_dir_lookup_result() without removing from cache. So if it is the case, it seems that it will also BUG when found. Thanks, Joseph
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index 213206ebdd58..3bfbc166d1fe 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -416,6 +416,9 @@ static struct buffer_head *ocfs2_find_entry_id(const char *name, if (found == 1) return di_bh; + /* Cached by ocfs2_read_blocks() so remove it. */ + ocfs2_remove_from_cache(INODE_CACHE(dir), di_bh); + brelse(di_bh); out: return NULL;
Syzbot has reported the following BUG: kernel BUG at fs/ocfs2/uptodate.c:509! ... Call Trace: <TASK> ? __die_body+0x61/0xb0 ? die+0x9f/0xc0 ? do_trap+0x14a/0x3d0 ? ocfs2_set_new_buffer_uptodate+0x149/0x170 ? do_error_trap+0x1c2/0x280 ? ocfs2_set_new_buffer_uptodate+0x149/0x170 ? __pfx_do_error_trap+0x10/0x10 ? handle_invalid_op+0x34/0x40 ? ocfs2_set_new_buffer_uptodate+0x149/0x170 ? exc_invalid_op+0x39/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? ocfs2_set_new_buffer_uptodate+0x148/0x170 ? ocfs2_set_new_buffer_uptodate+0x149/0x170 ? ocfs2_set_new_buffer_uptodate+0x148/0x170 ocfs2_group_add+0x318/0x1240 ? mnt_get_write_access+0x68/0x2a0 ? __pfx_ocfs2_group_add+0x10/0x10 ? mnt_get_write_access+0x68/0x2a0 ? __pfx_lock_release+0x10/0x10 ? rcu_read_lock_any_held+0xb4/0x130 ? __pfx_rcu_read_lock_any_held+0x10/0x10 ? __pfx_lock_release+0x10/0x10 ? mnt_get_write_access+0x223/0x2a0 ? mnt_want_write_file+0x172/0x200 ocfs2_ioctl+0x60b/0x750 ? __pfx_ocfs2_ioctl+0x10/0x10 ? lockdep_hardirqs_on_prepare+0x401/0x750 ? bpf_lsm_file_ioctl+0x9/0x20 ? __pfx_ocfs2_ioctl+0x10/0x10 __se_sys_ioctl+0xfc/0x170 do_syscall_64+0xf6/0x210 ? exc_page_fault+0x92/0x110 entry_SYSCALL_64_after_hwframe+0x77/0x7f ... </TASK> In 'ocfs2_find_entry_id()', the buffer head remains cached (by 'ocfs2_read_blocks()') regardless of the search result returned by 'ocfs2_search_dirblock()'. In case of search failure, that buffer head is no longer used and should be uncached before calling 'brelse()'. This is very similar to commit 737f34137844 ("ocfs2: uncache inode which has failed entering the group"). Reported-by: syzbot+7aef76bdb53b83d62a9e@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=7aef76bdb53b83d62a9e Fixes: 23193e513d1c ("ocfs2: Read support for directories with inline data") Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- fs/ocfs2/dir.c | 3 +++ 1 file changed, 3 insertions(+)