Re: [PATCH] bcachefs: zero-init move_bucket struct in bch2_copygc_get_buckets()

[Gianfranco Trad <gianf.trad@gmail.com>]

On 11/11/24 21:09, Kent Overstreet wrote:
> On Mon, Nov 11, 2024 at 03:42:44PM +0100, Gianfranco Trad wrote:
>> zero-init move_bucket struct b fields in bch2_copygc_get_buckets()
>> to mitigate later uninit-value-use KMSAN reported bug.
>>
>> Reported-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=8689d10f1894eedf774d
>> Tested-by: syzbot+8689d10f1894eedf774d@syzkaller.appspotmail.com
>> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
>> ---
>>   fs/bcachefs/movinggc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
>> index d658be90f737..cdc456b03bec 100644
>> --- a/fs/bcachefs/movinggc.c
>> +++ b/fs/bcachefs/movinggc.c
>> @@ -171,7 +171,8 @@ static int bch2_copygc_get_buckets(struct moving_context *ctxt,
>>   				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
>>   				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
>>   				  0, k, ({
>> -		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
>> +		struct move_bucket b = { 0 };
>> +		b.k.bucket = u64_to_bucket(k.k->p.offset);
>>   		int ret2 = 0;
> 
> Providing any sort of initializer should cause the whole struct to be
> initialized, are you and syzbot sure this is the right fix?
You are right, there's no need to initialize the whole struct.
I'm still in the process of fully understanding what reproducer is 
trying to do.
So far with the additional findings, b.k seems not to be initialized 
prior usage in repro case, therefore memset to 0 only the b.k field 
seems enough:

diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
index d658be90f737..515b05d26d11 100644
--- a/fs/bcachefs/movinggc.c
+++ b/fs/bcachefs/movinggc.c
@@ -171,7 +171,9 @@ static int bch2_copygc_get_buckets(struct 
moving_context *ctxt,
  				  lru_pos(BCH_LRU_FRAGMENTATION_START, 0, 0),
  				  lru_pos(BCH_LRU_FRAGMENTATION_START, U64_MAX, LRU_TIME_MAX),
  				  0, k, ({
-		struct move_bucket b = { .k.bucket = u64_to_bucket(k.k->p.offset) };
+		struct move_bucket b;
+		memset(&b.k, 0, sizeof(b.k));
+		b.k.bucket = u64_to_bucket(k.k->p.offset);
  		int ret2 = 0;

  		saw++;

The above patch was already tested-by syzbot[1].

Let me know if the patch looks good enough or if I should work on it more.

Thanks for your time,

[1] https://syzkaller.appspot.com/x/log.txt?x=1733b8c0580000

--Gian