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