Re: general protection fault in usb_find_alt_setting

[Dmitry Vyukov]

On Sun, Sep 23, 2018 at 11:15 PM, Vladis Dronov <vdronov@xxxxxxxxxx> wrote:
> Hello, Dmitry,
>
> Thank you for the reply. I probably do not properly understand how
> syzcaller works then. Can you please, have a look at my reasoning.
>
> The bug:
>
> https://syzkaller.appspot.com/bug?id=4b88ff5aa6aa88f9283a45cc62f16e55b0722131
> (Reported-by: syzbot+c99ecc8a2c68eb7e06cf2f652e60d63d6fbe2f31@xxxxxxxxxxxxxxxxxxxxxxxxx,
> "[upstream] general protection fault in usb_find_alt_setting")
>
> was not fixed. it was closed as invalid, so, afaiu, all the work has stopped for it.
>
> So syzbot did not wait until the fixing commit reached all tested trees, and the
> crash was not spotted again _after_ that.
>
> Then I look at the bug:
>
> https://syzkaller.appspot.com/bug?id=a0ec6260a1d37288a4508250fe30a5604ceec666
> (Reported-by: syzbot+19c3aaef85a89d451eac@xxxxxxxxxxxxxxxxxxxxxxxxx,
> "[upstream] general protection fault in usb_find_alt_setting (2)")
>
> And I see the crash happens at the same place _and_ at the same code:
>
> (bug id=a0ec6260a1d3)
> RIP: 0010:usb_find_alt_setting+0x38/0x310 drivers/usb/core/usb.c:231
> Code: ... fd 48 8d 7b 04 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 86 02 00 00
> (bug id=4b88ff5aa6aa)
> Code: ... fd 48 8d 7b 04 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 a1 02 00
> RIP: usb_find_alt_setting+0x38/0x310 drivers/usb/core/usb.c:231 RSP: ffff88005893f610
>
> This makes me be sure these are the same bug (dup) which are fixed by the same
> commit "USB: handle NULL config in usb_find_alt_setting()".
>
> As I'm kinda a perfectionist, I would like to mark (bug id=4b88ff5aa6aa) as
> fixed by this commit and not closed as invalid.


My bad: I did not check status of the first version of the bug:
https://syzkaller.appspot.com/bug?id=4b88ff5aa6aa88f9283a45cc62f16e55b0722131

The thing is that "fixed" and "invalid" are considered terminal states
for a bug. syzbot ignores all commands for them. This was done to
avoid races between re-opening an old bug and syzbot creating a new
version of the same bug at the same time. And also to simplify overall
process: bugs moving monotonically to terminal states. So at the
moment it's unfortunately not possible to go back and say that this
was a dup rather than invalid, like one would do in a real bug
tracker.
As a rule of thumb: things still on dashboard need some action, things
that are already not on the dashboard do not need any attention.

The reason for closing the first one was noted here:
https://groups.google.com/d/msg/syzkaller-bugs/Iqf80RqXK14/SSYd1aAtAQAJ
Taking into account the huge number of bugs, it can make more sense to
clean up the dashboard from old, most likely irrelevant bugs to
increase chances of fixing other bugs. In this case it was a wrong
decision, probably a part of a sweeping clean up. But syzbot created a
new bug for this since it started happening again, so in the end
everything worked as intended.

I understand this all can look somewhat confusing initially. The
system built as an attempt to chew hundreds of bugs per month with
limited human resources.

But we need perfectionists for lots of the open bugs on the dashboard!
https://syzkaller.appspot.com#upstream


> ----- Original Message -----
>> From: "Dmitry Vyukov" <dvyukov@xxxxxxxxxx>
>> On Sun, Sep 23, 2018 at 11:11 AM, Vladis Dronov <vdronov@xxxxxxxxxx> wrote:
>> > #syz fix: USB: handle NULL config in usb_find_alt_setting()
>> > #syz dup: general protection fault in usb_find_alt_setting (2)
>>
>> Same here.
>> syzbot process designed in such way that it will not open second
>> version of the bug (2) for the same bug. syzbot waits until the fixing
>> commit reaches all tested tree and only then closes a bug. If the
>> crash is spotted again _after_ that, then syzbot creates second
>> version of the bug (2). But at that point it has to be a different bug
>> requiring a different fix.
>> So this should not be a dup, and should not fixed with the same commit
>> as the first version.