Re: WARNING in ion_ioctl

[Dmitry Vyukov]

On Thu, Jan 4, 2018 at 3:24 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> > On Thu, Jan 04, 2018 at 05:57:01AM -0800, syzbot wrote:
>> >> Hello,
>> >>
>> >> syzkaller hit the following crash on
>> >> 71ee203389f7cb1c1927eab22b95baa01405791c
>> >> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
>> >> compiler: gcc (GCC) 7.1.1 20170620
>> >> .config is attached
>> >> Raw console output is attached.
>> >> C reproducer is attached
>> >> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
>> >> for information about syzkaller reproducers
>> >>
>> >>
>> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> >> Reported-by: syzbot+fa2d5f63ee5904a0115a@xxxxxxxxxxxxxxxxxxxxxxxxx
>> >> It will help syzbot understand when the bug is fixed. See footer for
>> >> details.
>> >> If you forward the report, please keep this part and the footer.
>> >>
>> >> audit: type=1400 audit(1514734723.062:7): avc:  denied  { map } for
>> >> pid=3502 comm="syzkaller809746" path="/root/syzkaller809746698" dev="sda1"
>> >> ino=16481 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
>> >> tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1
>> >> WARNING: CPU: 0 PID: 3502 at drivers/staging/android/ion/ion-ioctl.c:73
>> >> ion_ioctl+0x2db/0x380 drivers/staging/android/ion/ion-ioctl.c:73
>> >> Kernel panic - not syncing: panic_on_warn set ...
>> >
>> > This is to be expected when you pass in a crappy ion ioctl structure.
>> >
>> > So don't do that :)
>> >
>> > Yeah, it's a harsh warning, but I think the userspace developers like it
>> > to ensure they got their implementation correct.
>> >
>> > After the warning is thrown, all keeps working just fine.
>>
>> Hi Greg,
>>
>> Or, don't do WARNINGs on EINVAL and do pr_warn instead, as useful but
>> also enables automated kernel testing with non-tainted reports, which
>> is kinda a useful property.
>
> Sure, that would be the sane thing to do, but this is staging android
> code, the exact opposite of "sane" at the moment :)

If it's so bad that it's not even ready for testing and has lots of
know bugs, then I will just go and disable it on syzbot. Just say.
But then it will never become production-ready until we re-enable
testing and go through this cleaning process later.

But I would expect that if the code is in mainline, even if in
staging, we should productionize it as quickly as possible, in
particular using fuzzing as guidance.
And in the end pr_warn can even provide a more informative error
message as compared to the WARNING with cpu number, process id,
register values and the stack trace with is always the same for the
ioctl.