Re: WARNING in __rcu_read_unlock

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 18 Dec 2018 09:49:17 +0100
Dmitry Vyukov <dvyukov@google.com> wrote:

> On Tue, Dec 18, 2018 at 12:18 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Mon, 17 Dec 2018 16:53:36 +0100
> > Dmitry Vyukov <dvyukov@google.com> wrote:
> >  
> > > On Mon, Dec 17, 2018 at 4:24 PM Stefano Brivio <sbrivio@redhat.com> wrote:  
> > > >
> > > > On Mon, 17 Dec 2018 06:57:35 -0800
> > > > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > >  
> > > > > Might be cause by commit b8a51b38e4d4dec3e379d52c0fe1a66827f7cf1e
> > > > > fou, fou6: ICMP error handlers for FoU and GUE  
> > > >
> > > > This:
> > > >
> > > > diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
> > > > index 0d0ad19ecb87..20a6de26d146 100644
> > > > --- a/net/ipv4/fou.c
> > > > +++ b/net/ipv4/fou.c
> > > > @@ -1008,6 +1008,9 @@ static int gue_err_proto_handler(int proto, struct sk_buff *skb, u32 info)
> > > >  {
> > > >         const struct net_protocol *ipprot = rcu_dereference(inet_protos[proto]);
> > > >
> > > > +       if (ipprot == IPPROTO_UDP)
> > > > +               return -EINVAL;
> > > > +
> > > >         if (ipprot && ipprot->err_handler) {
> > > >                 if (!ipprot->err_handler(skb, info))
> > > >                         return 0;
> > > >
> > > > should fix the issue, but I still have to run tests and make sure we
> > > > don't hit similar cases.  
> > >
> > > Please don't forget to add a regression test for it too ;)  
> >
> > Where would you suggest to add this? The only selftest that goes  
> 
> I dunno. But there must be some place for such tests, right?

Not as far as I know. The selftests checking this path, by design, only
use supported configurations, they don't forge packets.

Maybe it would be nice to have a semi-automated way to isolate and
describe/name specific conditions found by syzbot via fuzzing and turn
those into tests that are then repeated periodically. I'm not sure how
that would look like, but I think it's still more maintainable than a
pile of C reproducers with forged packets in selftests/net.

Eric, Cong, Xin, as you also recently fixed a nice deal of similar cases
reported by syzbot, what do you think? Did you ever feel the need to
turn a syzbot reproducer into a regression test case?

> > through this path currently is net/pmtu.sh, but as configuration of an
> > actual UDP-in-GUE tunnel is currently not supported, I would really
> > need to forge that specific packet, so that doesn't seem to be a good
> > fit.
> >
> > Won't syzbot add this to some list of reproducers that are checked in
> > the future?  
> 
> It won't. Also fuzzing is complementary to testing, not a replacement:

Indeed, but that doesn't mean we need to limit the potential of fuzzing
because "it's not testing". It can be used to check for regressions,
too, especially in these cases.

> https://twitter.com/dvyukov/status/1074719682962358272

Now, I'm extremely thankful for the work you're doing and especially
for finding this subtle condition with syzbot, but this is quite
inaccurate. To be exposed to this issue, the user would need to
have the fou module loaded (it won't autoload), which is used quite
rarely, and, on top of that, have a UDP tunnel configured. It wouldn't
have been the kind of "evil packet crashes the internet" scenario you
were dreaming of ;)

-- 
Stefano