netdev - Re: [PATCH RESEND bpf-next 03/15] ice: make RX checksum checking code more reusable

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]
Date: Mon, 22 May 2023 17:51:37 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>
CC: <bpf@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
	"Andrii Nakryiko" <andrii@...nel.org>, Jakub Kicinski <kuba@...nel.org>,
	"Martin KaFai Lau" <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
	Yonghong Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, KP
 Singh <kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Anatoly Burakov <anatoly.burakov@...el.com>, Jesper Dangaard Brouer
	<brouer@...hat.com>, Alexander Lobakin <alexandr.lobakin@...el.com>, "Magnus
 Karlsson" <magnus.karlsson@...il.com>, Maryam Tahhan <mtahhan@...hat.com>,
	<xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 03/15] ice: make RX checksum checking code
 more reusable

From: Larysa Zaremba <larysa.zaremba@...el.com>
Date: Fri, 12 May 2023 17:25:55 +0200

> Previously, we only needed RX checksum flags in skb path,
> hence all related code was written with skb in mind.
> But with the addition of XDP hints via kfuncs to the ice driver,
> the same logic will be needed in .xmo_() callbacks.
> 
> Put generic process of determining checksum status into
> a separate function.
> 
> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 71 ++++++++++++-------
>  1 file changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> index 1aab79dc8915..6a4fd3f3fc0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> @@ -104,17 +104,17 @@ ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring,
>  }
>  
>  /**
> - * ice_rx_csum - Indicate in skb if checksum is good
> - * @ring: the ring we care about
> - * @skb: skb currently being received and modified
> + * ice_rx_csum_checked - Indicates, whether hardware has checked the checksum

%CHECKSUM_UNNECESSARY means that the csum is correct / frame is not
damaged. So "checked" is not enough I'd say, it's "verified" at least.
OTOH that's too long already, I'd go with classic "csum_ok" :D

>   * @rx_desc: the receive descriptor
>   * @ptype: the packet type decoded by hardware
> + * @csum_lvl_dst: address to put checksum level into
> + * @ring: ring for error stats, can be NULL
>   *
> - * skb->protocol must be set before this function is called
> + * Returns true, if hardware has checked the checksum.
>   */
> -static void
> -ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb,
> -	    union ice_32b_rx_flex_desc *rx_desc, u16 ptype)
> +static bool
> +ice_rx_csum_checked(union ice_32b_rx_flex_desc *rx_desc, u16 ptype,

(also const, but I guess you'll do that either way after the previous
 mails)

> +		    u8 *csum_lvl_dst, struct ice_rx_ring *ring)
>  {
>  	struct ice_rx_ptype_decoded decoded;
>  	u16 rx_status0, rx_status1;

[...]

> +/**
> + * ice_rx_csum_into_skb - Indicate in skb if checksum is good
> + * @ring: the ring we care about
> + * @skb: skb currently being received and modified
> + * @rx_desc: the receive descriptor
> + * @ptype: the packet type decoded by hardware
> + */
> +static void
> +ice_rx_csum_into_skb(struct ice_rx_ring *ring, struct sk_buff *skb,
> +		     union ice_32b_rx_flex_desc *rx_desc, u16 ptype)
> +{
> +	u8 csum_level = 0;

I'm not a fan of variables shorter than u32 on the stack. And since it
gets passed by a reference, I'm not sure the compiler will inline it =\

> +
> +	/* Start with CHECKSUM_NONE and by default csum_level = 0 */
> +	skb->ip_summed = CHECKSUM_NONE;
> +	skb_checksum_none_assert(skb);

Can we also remove this? Neither of these makes sense. ::ip_summed is
always zeroed after the memset() in __build_skb_around() (somewhere
there), while the assertion checks for `skb->ip_summed ==
CHECKSUM_NONE`, i.e. it's *always* true here (set and check :D). It's
some ancient pathetic rituals copied over and over again from e100
centuries or so...

...and BTW the comment is misleading, because the code doesn't zero
::csum_level as they claim :D

> +
> +	/* check if Rx checksum is enabled */
> +	if (!(ring->netdev->features & NETIF_F_RXCSUM))
> +		return;
> +
> +	if (!ice_rx_csum_checked(rx_desc, ptype, &csum_level, ring))
> +		return;
> +
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->csum_level = csum_level;

Since csum_level is useless when ip_summed is set to NONE, what do you
think about making the function return -1, 0, or 1 without writing
anything by reference?

	int csum_level;

	csum_level = ice_rx_csum_ok(rx_desc, ptype, ring);
	if (csum_level < 0)
		return;

	skb->ip_summed = CHECKSUM_UNNECESSARY;
	skb->csum_level = csum_level;

I'm not saying it's better (might be a bit at codegen), just proposing.

>  }
>  
>  /**
> @@ -232,7 +253,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  	/* modifies the skb - consumes the enet header */
>  	skb->protocol = eth_type_trans(skb, rx_ring->netdev);
>  
> -	ice_rx_csum(rx_ring, skb, rx_desc, ptype);
> +	ice_rx_csum_into_skb(rx_ring, skb, rx_desc, ptype);
>  
>  	if (rx_ring->ptp_rx)
>  		ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb);

Thanks,
Olek