Messages in this thread | | | Date | Tue, 18 Dec 2018 13:40:24 +0100 | From | Stefano Brivio <> | Subject | Re: WARNING in __rcu_read_unlock |
| |
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
| |