netdev - Re: [PATCH net 0/5] net: xdp: execute xdp_do_flush() before napi_complete

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ8uoz1SLe-AqW7Byd77g=_Z+oUy335+j18jRmZtdi+1FomCkw@mail.gmail.com>
Date:   Tue, 17 Jan 2023 11:40:57 +0100
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     magnus.karlsson@...el.com, bjorn@...nel.org, ast@...nel.org,
        daniel@...earbox.net, netdev@...r.kernel.org,
        jonathan.lemon@...il.com, maciej.fijalkowski@...el.com,
        kuba@...nel.org, toke@...hat.com, pabeni@...hat.com,
        davem@...emloft.net, aelior@...vell.com, manishc@...vell.com,
        horatiu.vultur@...rochip.com, UNGLinuxDriver@...rochip.com,
        jasowang@...hat.com, ioana.ciornei@....com, madalin.bucur@....com,
        bpf@...r.kernel.org
Subject: Re: [PATCH net 0/5] net: xdp: execute xdp_do_flush() before napi_complete_done()

On Tue, Jan 17, 2023 at 11:12 AM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Tue, Jan 17, 2023 at 10:25:28AM +0100, Magnus Karlsson wrote:
> > Make sure that xdp_do_flush() is always executed before
> > napi_complete_done(). This is important for two reasons. First, a
> > redirect to an XSKMAP assumes that a call to xdp_do_redirect() from
> > napi context X on CPU Y will be follwed by a xdp_do_flush() from the
> > same napi context and CPU. This is not guaranteed if the
> > napi_complete_done() is executed before xdp_do_flush(), as it tells
> > the napi logic that it is fine to schedule napi context X on another
> > CPU. Details from a production system triggering this bug using the
> > veth driver can be found in [1].
> >
> > The second reason is that the XDP_REDIRECT logic in itself relies on
> > being inside a single NAPI instance through to the xdp_do_flush() call
> > for RCU protection of all in-kernel data structures. Details can be
> > found in [2].
> >
> > The drivers have only been compile-tested since I do not own any of
> > the HW below. So if you are a manintainer, please make sure I did not
> > mess something up. This is a lousy excuse for virtio-net though, but
> > it should be much simpler for the vitio-net maintainers to test this,
> > than me trying to find test cases, validation suites, instantiating a
> > good setup, etc. Michael and Jason can likely do this in minutes.
>
> This kind of thing doesn't scale though. There are more contributors
> than maintainers. Also, I am not 100% sure what kind of XDP workload
> do I need to be a good test.

True. Is there a smoke test that could be run to check that normal
traffic is not affected? Just so we know that it works as expected.
Then we could move on to try out XDP_REDIRECT for virtio. Anyone out
there that knows of something existing that could be used for this?
Just note that reproducing the issue seems to be challenging as 10
systems running a production workload only experienced a single
failure per night due to this [1]. So I suggest we just go with
checking that existing functionality works as expected.

> >
> > Note that these were the drivers I found that violated the ordering by
> > running a simple script and manually checking the ones that came up as
> > potential offenders. But the script was not perfect in any way. There
> > might still be offenders out there, since the script can generate
> > false negatives.
> >
> > [1] https://lore.kernel.org/r/20221220185903.1105011-1-sbohrer@cloudflare.com
> > [2] https://lore.kernel.org/all/20210624160609.292325-1-toke@redhat.com/
> >
> > Thanks: Magnus
> >
> > Magnus Karlsson (5):
> >   qede: execute xdp_do_flush() before napi_complete_done()
> >   lan966x: execute xdp_do_flush() before napi_complete_done()
> >   virtio-net: execute xdp_do_flush() before napi_complete_done()
> >   dpaa_eth: execute xdp_do_flush() before napi_complete_done()
> >   dpaa2-eth: execute xdp_do_flush() before napi_complete_done()
> >
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c        | 6 +++---
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c      | 9 ++++++---
> >  drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 6 +++---
> >  drivers/net/ethernet/qlogic/qede/qede_fp.c            | 7 ++++---
> >  drivers/net/virtio_net.c                              | 6 +++---
> >  5 files changed, 19 insertions(+), 15 deletions(-)
> >
> >
> > base-commit: 87b93b678e95c7d93fe6a55b0e0fbda26d8c7760
> > --
> > 2.34.1
>