linux-kernel - Re: [PATCH] Fix WARNING: suspicious RCU usage in bch2_bucket_ref

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <btghyyjqr53g4duu2b3ucbo2yksupsox2fcsniugceej2fbfqx@7tvxssrbctoo>
Date: Wed, 10 Jul 2024 12:45:54 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Pei Li <peili.dev@...il.com>
Cc: Brian Foster <bfoster@...hat.com>, linux-bcachefs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, syzkaller-bugs@...glegroups.com, 
	linux-kernel-mentees@...ts.linuxfoundation.org, syzbot+e74fea078710bbca6f4b@...kaller.appspotmail.com
Subject: Re: [PATCH] Fix WARNING: suspicious RCU usage in
 bch2_bucket_ref_update

On Wed, Jul 10, 2024 at 08:33:24AM GMT, Pei Li wrote:
> When dereference ca->bucket_gens, we are expecting one of the three
> locks to be hold if ca->fs is not NULL.
> 
> This patch acquires mark_lock before entering __mark_pointer()
> 
> Note: testing is done by a robot and is best-effort only.
> 
> To: Kent Overstreet <kent.overstreet@...ux.dev>
> To: Brian Foster <bfoster@...hat.com>
> Cc: linux-bcachefs@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: skhan@...uxfoundation.org
> Cc: syzkaller-bugs@...glegroups.com
> Cc: linux-kernel-mentees@...ts.linuxfoundation.org
> Reported-by: syzbot+e74fea078710bbca6f4b@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e74fea078710bbca6f4b
> Tested-by: syzbot+e74fea078710bbca6f4b@...kaller.appspotmail.com
> Signed-off-by: Pei Li <peili.dev@...il.com>
> ---
> Syzbot reported the following warning:
> 
> fs/bcachefs/buckets.h:107 suspicious rcu_dereference_check() usage!
> 
> When dereference ca->bucket_gens, we are expecting one of the three
> locks to be hold if ca->fs is not NULL.
> 
> This patch acquires mark_lock before entering __mark_pointer()
> 
> Tested on:
> 
> commit:         34afb82a Merge tag '6.10-rc6-smb3-server-fixes' of git..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=1492e87e980000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=42a432cfd0e579e0
> dashboard link: https://syzkaller.appspot.com/bug?extid=e74fea078710bbca6f4b
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> patch:          https://syzkaller.appspot.com/x/patch.diff?x=15e2e87e980000
> 
> Note: testing is done by a robot and is best-effort only.
> ---
>  fs/bcachefs/buckets.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/bcachefs/buckets.c b/fs/bcachefs/buckets.c
> index 743d57eba760..03147e46dc47 100644
> --- a/fs/bcachefs/buckets.c
> +++ b/fs/bcachefs/buckets.c
> @@ -1033,8 +1033,14 @@ static int bch2_trigger_pointer(struct btree_trans *trans,
>  
>  	if (flags & BTREE_TRIGGER_transactional) {
>  		struct bkey_i_alloc_v4 *a = bch2_trans_start_alloc_update(trans, bucket);
> +
> +		percpu_down_read(&c->mark_lock);
> +
>  		ret = PTR_ERR_OR_ZERO(a) ?:
>  			__mark_pointer(trans, ca, k, &p.ptr, *sectors, bp.data_type, &a->v);
> +
> +		percpu_up_read(&c->mark_lock);

This is much heavier than we need, a more localized rcu_read_lock() is
sufficient. This is in my tree (neglected to include it with today's
pull request, whoops).

commit 5df2f4ffe8bb9bead346f4768df603fa575678df
Author: Kent Overstreet <kent.overstreet@...ux.dev>
Date:   Tue Jul 9 16:43:01 2024 -0400

    bcachefs: Fix RCU splat
    
    Reported-by: syzbot+e74fea078710bbca6f4b@...kaller.appspotmail.com
    Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>

diff --git a/fs/bcachefs/buckets.c b/fs/bcachefs/buckets.c
index 743d57eba760..314ee3e0187f 100644
--- a/fs/bcachefs/buckets.c
+++ b/fs/bcachefs/buckets.c
@@ -805,7 +805,7 @@ int bch2_bucket_ref_update(struct btree_trans *trans, struct bch_dev *ca,
 			"bucket %u:%zu gen %u (mem gen %u) data type %s: stale dirty ptr (gen %u)\n"
 			"while marking %s",
 			ptr->dev, bucket_nr, b_gen,
-			*bucket_gen(ca, bucket_nr),
+			bucket_gen_get(ca, bucket_nr),
 			bch2_data_type_str(bucket_data_type ?: ptr_data_type),
 			ptr->gen,
 			(printbuf_reset(&buf),
diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
index 80ee0be9793e..8ad4be73860c 100644
--- a/fs/bcachefs/buckets.h
+++ b/fs/bcachefs/buckets.h
@@ -116,6 +116,14 @@ static inline u8 *bucket_gen(struct bch_dev *ca, size_t b)
 	return gens->b + b;
 }
 
+static inline u8 bucket_gen_get(struct bch_dev *ca, size_t b)
+{
+	rcu_read_lock();
+	u8 gen = *bucket_gen(ca, b);
+	rcu_read_unlock();
+	return gen;
+}
+
 static inline size_t PTR_BUCKET_NR(const struct bch_dev *ca,
 				   const struct bch_extent_ptr *ptr)
 {