Re: RFC: chasing all idr_remove() misses

[Alexandre Ferrieux <alexandre.ferrieux@gmail.com>]

On 18/11/2024 03:47, Matthew Wilcox wrote:
> On Sun, Nov 17, 2024 at 04:07:43PM +0100, Alexandre Ferrieux wrote:
>> On 16/11/2024 20:43, Dan Carpenter wrote:
>> >
>> > On Sat, Nov 16, 2024 at 06:45:37PM +0100, Alexandre Ferrieux wrote:
>> >>
>> >>   73af53d82076 net: sched: cls_u32: Fix u32's systematic failure to free IDR
>> >>                entries for hnodes.
>> >>
>> >> So, unless we have reasons to think cls_u32 was the only place where two ID
>> >> encodings might lend themselves to confusion, I'm wondering if it wouldn't
>> >> make sense to chase the issue more systematically
>> >>
>> > Matthew Wilcox maintains IDR so it's really up to him.  I don't think adding
>> > a WARN_ON_ONCE() in idr_remove() for NULL returns is a bad idea but we could
>> > hide it behind a #if DEBUG_IDR or something and try run syzkaller on it first.
>> 
>> Thanks Dan. Now, I'm not familiar with the syzbot feeding process, can you help
>> me out ? Is the next step to:
>> 
>>  (a) invent the new debug flag (e.g. "DEBUG_IDR") then post a patch using it;
>> wait for its acceptation and merge ; then contact syzbot operators to add it in
>> (some of) its builds
>> 
>>  (b) use some existing wide-range debug flag so that syzbot will automagically
>> test IDR once the patch is merged
>> 
>>  (c) wait for you or Matthew to handle all of this
> 
> IDR is deprecated.  I'm not excited about patches that make it better.
> IDR users should be converted to use the XArray API.

Well, the problem is more about chasing a bad usage pattern than an API's
limitations. Indeed, doing the same grep for Xarray shows that, out of 434
(naked) call sites of xa_erase() in the kernel, 307 fail to check the result.

So, independently from the IDR->XArray transition (which may still take some
time), it looks like there's a Damocles sword over a possibly huge number of
areas, and that would handily be grabbed by a syzbot scan, with a ridiculous
time investment: just add #ifdef ... WARN_ON_ONCE() ...#endif on the "return
NULL" path of both idr_remove() and xa_erase().

Again, I'm not asking to do this on production kernels. All I'm asking is the
detailed procedure to let the "established" fuzzers exercise that, as using
syzkaller locally on my limited hardware is not an serious option.