On Tue, 25 Sep 2018, Vladis Dronov wrote:
> > > What about adding a WARN_ON()? It doesn't crash the kernel and it will
> > > be detected and reported by syzbot.
>
> Yes, that would be a great solution.
>
> > Sure, we could do that. But would be the point?
>
> We know when usb_find_alt_setting() callers do smth weird and go fix them.
>
> > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is
> > no more of a bug than calling kfree() with a NULL pointer.
>
> Yes, exactly.
>
> > You wouldn't want to put a WARN_ON in kfree(), would you?
>
> Honestly, in the ideal world I would, again, to be aware when some code does
> something weird so we know about it. But this world is this world, it needs
> more performance to the throne of performance.
But is it really worthwhile? In terms of catching bugs, this would
help in only one situation: when the programmer thinks the argument
should always be non-NULL because a NULL argument indicates a bug.
Such situations seem to be relatively rare, and we can handle them by
inserting a WARN_ON() at the call site if need be.
So it's a choice between:
1. Putting a single test for NULL in the function being called,
together with WARN_ON() at a small number of call sites, or
2. Putting a WARN_ON() (or allowing a crash) in the function being
called, together with tests for NULL at a potentially large
number of call sites.
1 has two advantages over 2. First, it involves adding less code
overall. Second, it doesn't require the programmer to remember to add
special code (a test or a WARN_ON) in situation where it doesn't
matter -- presumably the majority of them.
Now consider the case at hand: the call to usb_find_alt_setting() from
check_ctrlrecip(). In this case ps->dev->actconfig being NULL doesn't
indicate an error or a bug; it merely indicates that the user is trying
to send a control request to a device which happens to be unconfigured,
which is a perfectly valid thing to do. Therefore it shouldn't require
any special handling at the call site.
Alan Stern
