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

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

[Gianfranco Trad]

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;
 
 		saw++;
-- 
2.43.0

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

[Kent Overstreet]

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?

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

[Gianfranco Trad]

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

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

[Gianfranco Trad]

On 12/11/24 16:08, Gianfranco Trad wrote:
> 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

Hi Kent,

I wanted to follow up on this patch. Over the last few days, I've 
investigated it further and observed the following that might be of help:

1- zero-initing whole b struct (as the first patch version) leads to a 
clean log [1].
While if trying to memset to 0 only b.k field log reports [2]:

bcachefs (da441363-bb6a-4ab9-999b-c1f40db4fee2): filesystem UUID already 
open
bcachefs (da441363-bb6a-4ab9-999b-c1f40db4fee2): shutdown complete
bcachefs: bch2_fs_get_tree() error: EINVAL


2- Given both versions of the patch didn't trigger the uninit issue 
anymore I checked whether inner fields of b.k.bucket are correctly 
inited, just before the bug triggers.
b.k.bucket fields seeming to look correctly inited: snapshot = 0, offset 
= 34, inode = 0, gen = 0.

3- The first of the 2 reproducers causes a segfault:

==9335== Invalid[  264.802101][ T9346] read of size 1
==9335==    at 0x483BC39: strnlen (vg_replace_strmem.c:426)
==9335==    by 0x1098F0: netlink_query_family_id (repro.c:176)
==9335==    by 0x109A51: syz_genetlink_get_family_id (repro.c:211)
==9335==    by 0x10B476: execute_one (repro.c:2071)
==9335==    by 0x10B1A5: loop (repro.c:745)
==9335==    by 0x10B52F: main (repro.c:2088)
==9335==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

As of now, it seems unrelated to the root cause of the reported syzbot bug.


At this point, zero-initializing the entire struct seems to work 
reliably, thought I'm still trying to get the full picture on this bug.

I know you are busy, but I'd highly appreciate your thoughts on this.
I hope it might help.

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

Thanks for your time,
--Gian

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

[Kent Overstreet]

On Sun, Dec 15, 2024 at 02:58:26AM +0100, Gianfranco Trad wrote:
> Hi Kent,
> 
> I wanted to follow up on this patch. Over the last few days, I've
> investigated it further and observed the following that might be of help:
> 
> 1- zero-initing whole b struct (as the first patch version) leads to a clean
> log [1].
> While if trying to memset to 0 only b.k field log reports [2]:
> 
> bcachefs (da441363-bb6a-4ab9-999b-c1f40db4fee2): filesystem UUID already
> open
> bcachefs (da441363-bb6a-4ab9-999b-c1f40db4fee2): shutdown complete
> bcachefs: bch2_fs_get_tree() error: EINVAL
> 
> 
> 2- Given both versions of the patch didn't trigger the uninit issue anymore
> I checked whether inner fields of b.k.bucket are correctly inited, just
> before the bug triggers.
> b.k.bucket fields seeming to look correctly inited: snapshot = 0, offset =
> 34, inode = 0, gen = 0.
> 
> 3- The first of the 2 reproducers causes a segfault:
> 
> ==9335== Invalid[  264.802101][ T9346] read of size 1
> ==9335==    at 0x483BC39: strnlen (vg_replace_strmem.c:426)
> ==9335==    by 0x1098F0: netlink_query_family_id (repro.c:176)
> ==9335==    by 0x109A51: syz_genetlink_get_family_id (repro.c:211)
> ==9335==    by 0x10B476: execute_one (repro.c:2071)
> ==9335==    by 0x10B1A5: loop (repro.c:745)
> ==9335==    by 0x10B52F: main (repro.c:2088)
> ==9335==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> As of now, it seems unrelated to the root cause of the reported syzbot bug.
> 
> 
> At this point, zero-initializing the entire struct seems to work reliably,
> thought I'm still trying to get the full picture on this bug.

I think this might be a padding issue.

C struct literals are supposed to initialize the whole struct, but the
compiler folks in their infinite wisdom decided that didn't apply to
padding.

I think this needs to be raised with the KMSAN and compiler folks, as
inserting memsets all over the place would be a sad workaround.