netdev - Re: [PATCH net-next v2 1/2] ipv6: introduce and uses route look hints for list input

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 19 Nov 2019 09:10:29 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Edward Cree <ecree@...arflare.com>
Subject: Re: [PATCH net-next v2 1/2] ipv6: introduce and uses route look hints
 for list input

On Mon, Nov 18, 2019 at 4:59 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Mon, 2019-11-18 at 15:29 -0500, Willem de Bruijn wrote:
> > On Mon, Nov 18, 2019 at 6:03 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > > When doing RX batch packet processing, we currently always repeat
> > > the route lookup for each ingress packet. If policy routing is
> > > configured, and IPV6_SUBTREES is disabled at build time, we
> > > know that packets with the same destination address will use
> > > the same dst.
> > >
> > > This change tries to avoid per packet route lookup caching
> > > the destination address of the latest successful lookup, and
> > > reusing it for the next packet when the above conditions are
> > > in place. Ingress traffic for most servers should fit.
> > >
> > > The measured performance delta under UDP flood vs a recvmmsg
> > > receiver is as follow:
> > >
> > > vanilla         patched         delta
> > > Kpps            Kpps            %
> > > 1431            1664            +14
> >
> > Since IPv4 speed-up is almost half and code considerably more complex,
> > maybe only do IPv6?
>
> uhmm... I would avoid that kind of assimmetry, and I would not look
> down on a 8% speedup, if possible.

Okay, that's fair.

> > > @@ -104,9 +127,18 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
> > >                 skb = l3mdev_ip6_rcv(skb);
> > >                 if (!skb)
> > >                         continue;
> > > -               ip6_rcv_finish_core(net, sk, skb);
> > > +               ip6_rcv_finish_core(net, sk, skb, hint);
> > >                 dst = skb_dst(skb);
> > >                 if (curr_dst != dst) {
> > > +                       if (ip6_can_cache_route_hint(net)) {
> > > +                               _hint.refdst = skb->_skb_refdst;
> > > +                               memcpy(&_hint.daddr, &ipv6_hdr(skb)->daddr,
> > > +                                      sizeof(_hint.daddr));
> > > +                               hint = &_hint;
> > > +                       } else {
> > > +                               hint = NULL;
> > > +                       }
> >
> > not needed. ip6_can_cache_route_hit is the same for all iterations of
> > the loop (indeed, compile time static), so if false, hint is never
> > set.
>
> I think this is needed, instead: if CONFIG_MULTIPLE_TABLES=y,
> fib6_has_custom_rules can change at runtime - from 'false' to 'true'.
> If we don't reset 'hint', we could end-up with use-after-free.

Uhm, of course, this is not compile time static at all. I clearly
missed a part.

But such a config change does not expect instantaneous effect on
packets in flight, like those in the recv rcu critical section? In
which case it should be safe to treat all skbs in the list the same.

I would need to read that code more closely to be certain, and the
current solution errs on the side of caution, so is definitely fine as
is, of course.