| 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
| ||
|
Message-ID: <6693bcdb-b5dc-2f5b-41fa-9a9bba909dc7@intel.com>
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
Powered by blists - more mailing lists