Re: [syzbot] WARNING in unsafe_follow_pfn

[Dmitry Vyukov]

On Thu, Apr 1, 2021 at 2:19 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Wed, Mar 31, 2021 at 07:29:22AM +0300, Dan Carpenter wrote:
> > On Tue, Mar 30, 2021 at 07:04:30PM +0200, Paolo Bonzini wrote:
> > > On 30/03/21 17:26, syzbot wrote:
> > > > Hello,
> > > >
> > > > syzbot found the following issue on:
> > > >
> > > > HEAD commit:    93129492 Add linux-next specific files for 20210326
> > > > git tree:       linux-next
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=169ab21ad00000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6f2f73285ea94c45
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=015dd7cdbbbc2c180c65
> > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=119b8d06d00000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=112e978ad00000
> > > >
> > > > The issue was bisected to:
> > > >
> > > > commit d40b9fdee6dc819d8fc35f70c345cbe0394cde4c
> > > > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > Date:   Tue Mar 16 15:33:01 2021 +0000
> > > >
> > > >      mm: Add unsafe_follow_pfn
> > > >
> > > > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=122d2016d00000
> > > > final oops:     https://syzkaller.appspot.com/x/report.txt?x=112d2016d00000
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=162d2016d00000
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > Reported-by: syzbot+015dd7cdbbbc2c180c65@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Fixes: d40b9fdee6dc ("mm: Add unsafe_follow_pfn")
> > >
> > > This is basically intentional because get_vaddr_frames is broken, isn't it?
> > > I think it needs to be ignored in syzkaller.
> >
> > What?
> >
> > The bisect is wrong (because it's blaming the commit which added the
> > warning instead of the commit which added the buggy caller) but the
> > warning is correct.
> >
> > Plus users are going to be seeing this as well.  According to the commit
> > message for 69bacee7f9ad ("mm: Add unsafe_follow_pfn") "Unfortunately
> > there's some users where this is not fixable (like v4l userptr of iomem
> > mappings)".  It sort of seems crazy to dump this giant splat and then
> > tell users to ignore it forever because it can't be fixed...  0_0
>
> I think the discussion conclusion was that this interface should not
> be used by userspace anymore, it is obsolete by some new interface?
>
> It should be protected by some kconfig and the kconfig should be
> turned off for syzkaller runs.

If this is not a kernel bug, then it must not use WARN_ON[_ONCE]. It
makes the kernel untestable for both automated systems and humans:

https://lwn.net/Articles/769365/

<quote>
Greg Kroah-Hartman raised the problem of core kernel API code that
will use WARN_ON_ONCE() to complain about bad usage; that will not
generate the desired result if WARN_ON_ONCE() is configured to crash
the machine. He was told that the code should just call pr_warn()
instead, and that the called function should return an error in such
situations. It was generally agreed that any WARN_ON() or
WARN_ON_ONCE() calls that can be triggered from user space need to be
fixed.
</quote>


https://lore.kernel.org/netdev/20210413085522.2caee809@xxxxxxxxxxxxxxxxxx/
From: Steven Rostedt
<quote>

I agree. WARN_ON(_ONCE) should be reserved for anomalies that should not
happen ever. Anything that the user could trigger, should not trigger a
WARN_ON.

A WARN_ON is perfectly fine for detecting an accounting error inside the
kernel. I have them scattered all over my code, but they should never be
hit, even if something in user space tries to hit it. (with an exception of
an interface I want to deprecate, where I want to know if it's still being
used ;-) Of course, that wouldn't help bots testing the code. And I haven't
done that in years)

Any anomaly that can be triggered by user space doing something it should
not be doing really needs a pr_warn().
</quote>

And if it's a kernel bug reachable from user-space, then I think this
code should be removed entirely, not just on all testing systems. Or
otherwise if we are not removing it for some reason, then it needs to
be fixed.