diff mbox series

ocfs2: uncache inode after ocfs2_search_dirblock() search failure

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

Commit Message

Dmitry Antipov Dec. 27, 2024, 1:07 p.m. UTC
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(+)

Comments

Joseph Qi Dec. 28, 2024, 12:27 p.m. UTC | #1
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;
Dmitry Antipov Dec. 28, 2024, 4:33 p.m. UTC | #2
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
Joseph Qi Dec. 30, 2024, 1:57 a.m. UTC | #3
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 mbox series

Patch

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;