netdev - Re: [PATCH bpf-next v3 13/21] ice: Implement checksum hint

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: <CAADnVQKF3j-_qLM4MWkJKK=ZyPuWrLnmGfgf9BC4zm-4=1qSfw@mail.gmail.com>
Date: Thu, 20 Jul 2023 08:14:52 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: "Zaremba, Larysa" <larysa.zaremba@...el.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>, "ast@...nel.org" <ast@...nel.org>, 
	"daniel@...earbox.net" <daniel@...earbox.net>, "andrii@...nel.org" <andrii@...nel.org>, 
	"martin.lau@...ux.dev" <martin.lau@...ux.dev>, "song@...nel.org" <song@...nel.org>, "yhs@...com" <yhs@...com>, 
	"john.fastabend@...il.com" <john.fastabend@...il.com>, "kpsingh@...nel.org" <kpsingh@...nel.org>, 
	"sdf@...gle.com" <sdf@...gle.com>, "haoluo@...gle.com" <haoluo@...gle.com>, 
	"jolsa@...nel.org" <jolsa@...nel.org>, David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	Willem de Bruijn <willemb@...gle.com>, "Brouer, Jesper" <brouer@...hat.com>, 
	"Burakov, Anatoly" <anatoly.burakov@...el.com>, 
	"Lobakin, Aleksander" <aleksander.lobakin@...el.com>, Magnus Karlsson <magnus.karlsson@...il.com>, 
	"Tahhan, Maryam" <mtahhan@...hat.com>, 
	"xdp-hints@...-project.net" <xdp-hints@...-project.net>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 13/21] ice: Implement checksum hint

On Thu, Jul 20, 2023 at 2:47 AM Zaremba, Larysa
<larysa.zaremba@...el.com> wrote:
>
> On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote:
> > Alexei Starovoitov wrote:
> > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote:
> > > > Implement .xmo_rx_csum callback to allow XDP code to determine,
> > > > whether HW has validated any checksums.
> > > >
> > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > > > index 54685d0747aa..6647a7e55ac8 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci,
> > > >   return 0;
> > > >  }
> > > >
> > > > +/**
> > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum
> > > > + * @ctx: XDP buff pointer
> > > > + * @csum_status: destination address
> > > > + * @csum_info: destination address
> > > > + *
> > > > + * Copy HW checksum level (if was checked) to the destination address.
> > > > + */
> > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx,
> > > > +                    enum xdp_csum_status *csum_status,
> > > > +                    union xdp_csum_info *csum_info)
> > > > +{
> > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx;
> > > > + const union ice_32b_rx_flex_desc *eop_desc;
> > > > + enum ice_rx_csum_status status;
> > > > + u16 ptype;
> > > > +
> > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc;
> > > > + ptype = ice_get_ptype(eop_desc);
> > > > +
> > > > + status = ice_get_rx_csum_status(eop_desc, ptype);
> > > > + if (status & ICE_RX_CSUM_NONE)
> > > > +         return -ENODATA;
> > > > +
> > > > + *csum_status = ice_rx_csum_lvl(status) + 1;
> > > > + return 0;
> > > > +}
> > >
> > > and xdp_csum_info from previous patch left uninitialized?
> > > What was the point adding it then?
> >
> > I suppose this driver only returns CHECKSUM_NONE or
> > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir.
> >
>
> Yes, correct, current ice HW cannot produce complete checksum,
> so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise,
> nothing to initialize csum_info with in either case.
>
> xdp_csum_info is initialized in veth implementation though, but only
> csum_start/offset, so complete XDP checksum has no users in this patchset.
> Is this a problem?
>
> In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone
> has agreed, csum hint kfunc should give more comprehensive output.

csum kfunc supposed to be generic.
If for ICE it fills in one argument and for veth another then the whole
idea of generic api is not working.