Re: [PATCH bpf-next v5 8/8] selftests: net: add test for XDP_PASS skb checksum invalidation

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3751 bytes --]

On Jul 16, Stanislav Fomichev wrote:
> On 07/15, Lorenzo Bianconi wrote:
> > Add a test that verifies skb->ip_summed is set to CHECKSUM_NONE
> > when a device running in XDP mode creates an skb from a xdp_buff
> > if the attached ebpf program returns an XDP_PASS.
> > The test attaches an XDP program returning XDP_PASS, and a TC
> > ingress program that runs the bpf_skb_rx_checksum() kfunc to
> > inspect the resulting skb. After XDP_PASS the driver must invalidate
> > any previously computed hardware RX checksum since XDP may have
> > modified the packet data.
> > The BPF program counts packets per checksum type in a map, and the
> > test runner verifies that after sending traffic the CHECKSUM_NONE
> > counter is non-zero while CHECKSUM_UNNECESSARY and CHECKSUM_COMPLETE
> > counters are zero.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  Documentation/networking/xdp-rx-metadata.rst       |  5 ++
> >  .../selftests/drivers/net/hw/xdp_metadata.py       | 55 +++++++++++++++-
> >  .../selftests/net/lib/skb_metadata_csum.bpf.c      | 73 ++++++++++++++++++++++
> >  3 files changed, 132 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> > index 93918b3769a3..7434ac98242a 100644
> > --- a/Documentation/networking/xdp-rx-metadata.rst
> > +++ b/Documentation/networking/xdp-rx-metadata.rst
> > @@ -90,6 +90,11 @@ conversion, and the XDP metadata is not used by the kernel when building
> >  ``skbs``. However, TC-BPF programs can access the XDP metadata area using
> >  the ``data_meta`` pointer.
> 
> [..]
> 
> > +If a driver is running in XDP mode, any existing hardware RX checksum
> > +(``CHECKSUM_UNNECESSARY`` or ``CHECKSUM_COMPLETE``) must be invalidated
> > +by setting ``skb->ip_summed`` to ``CHECKSUM_NONE`` before passing the
> > +skb to the kernel, since XDP may have modified the packet data.
> > +
> >  In the future, we'd like to support a case where an XDP program
> >  can override some of the metadata used for building ``skbs``.
> 
> Sorry for keeping nitpicking on this, but I'm still not convinced that
> it is what we currently do. From my previous reply:

no worries :)
My current take-away from the previous discussion is we just need to document
what would be the driver expected behaviour adding a kselftest for it (without
modifying any driver).

> 
> > > Looking at a few drivers:
> > > - bnxt (bnxt_rx_pkt) does UNNECESSARY - ok
> > > - mlx5 (mlx5e_handle_csum) does UNNECESSARY and skips COMPLETE if there is
> > >   bpf prog attached
> > > - fbnic (fbnic_rx_csum) - can do COMPLETE even with xdp attached?
> > > - gve (gve_rx) - can do COMPLETE even with xdp attached?
> 
> (although for gve I might be wrong, there is also gve_rx_skb_csum that only
> does UNNECESSARY).
> 
> I'd wait for Jakub to chime in, but it feels like we should just document
> what we currently do as a recommended approach: for the drivers
> that support COMPLETE, do not report it when the bpf program is attached.
> Both NONE and UNNECESSARY are ok.

I am not completely sure the UNNECESSARY case is different from the COMPLETE
one. What are we supposed to do if the driver reports UNNECESSARY and the ebpf
program modifies some fields covered by the rx-checksum?

> 
> Also, did you run this test on real HW? NIPA now has HW tests, maybe it
> makes sense to route this series via net-next to get the real coverage?

What about splitting this series and have two different series:
- bpf-next: add xdp rx kfunc and related selftest
- net-next: add kselftest for the driver expected behaviour.

What do you think?

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]