lkml.org 
[lkml]   [2018]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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

    \
     
     \ /
      Last update: 2018-12-18 13:41    [W:4.388 / U:0.276 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site