netdev - Re: [net 1/2] net/mlx5e: When RXFCS is set, add FCS data into checksum calculation

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

Message-ID: <1ba0c9cd-1852-1043-c8ea-ce375f7d4ee1@gmail.com>
Date:   Mon, 29 Oct 2018 20:29:04 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [net 1/2] net/mlx5e: When RXFCS is set, add FCS data into
 checksum calculation



On 05/24/2018 02:53 PM, Saeed Mahameed wrote:
> From: Eran Ben Elisha <eranbe@...lanox.com>
> 
> When RXFCS feature is enabled, the HW do not strip the FCS data,
> however it is not present in the checksum calculated by the HW.
> 
> Fix that by manually calculating the FCS checksum and adding it to the SKB
> checksum field.
> 
> Add helper function to find the FCS data for all SKB forms (linear,
> one fragment or more).
> 
> Fixes: 102722fc6832 ("net/mlx5e: Add support for RXFCS feature flag")
> Signed-off-by: Eran Ben Elisha <eranbe@...lanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 176645762e49..1ff0b0e93804 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -615,6 +615,45 @@ static inline bool is_last_ethertype_ip(struct sk_buff *skb, int *network_depth)
>  	return (ethertype == htons(ETH_P_IP) || ethertype == htons(ETH_P_IPV6));
>  }
>  
> +static __be32 mlx5e_get_fcs(struct sk_buff *skb)
> +{
> +	int last_frag_sz, bytes_in_prev, nr_frags;
> +	u8 *fcs_p1, *fcs_p2;
> +	skb_frag_t *last_frag;
> +	__be32 fcs_bytes;
> +
> +	if (!skb_is_nonlinear(skb))
> +		return *(__be32 *)(skb->data + skb->len - ETH_FCS_LEN);
> +
> +	nr_frags = skb_shinfo(skb)->nr_frags;
> +	last_frag = &skb_shinfo(skb)->frags[nr_frags - 1];
> +	last_frag_sz = skb_frag_size(last_frag);
> +
> +	/* If all FCS data is in last frag */
> +	if (last_frag_sz >= ETH_FCS_LEN)
> +		return *(__be32 *)(skb_frag_address(last_frag) +
> +				   last_frag_sz - ETH_FCS_LEN);
> +
> +	fcs_p2 = (u8 *)skb_frag_address(last_frag);
> +	bytes_in_prev = ETH_FCS_LEN - last_frag_sz;
> +
> +	/* Find where the other part of the FCS is - Linear or another frag */
> +	if (nr_frags == 1) {
> +		fcs_p1 = skb_tail_pointer(skb);
> +	} else {
> +		skb_frag_t *prev_frag = &skb_shinfo(skb)->frags[nr_frags - 2];
> +
> +		fcs_p1 = skb_frag_address(prev_frag) +
> +			    skb_frag_size(prev_frag);
> +	}
> +	fcs_p1 -= bytes_in_prev;
> +
> +	memcpy(&fcs_bytes, fcs_p1, bytes_in_prev);
> +	memcpy(((u8 *)&fcs_bytes) + bytes_in_prev, fcs_p2, last_frag_sz);
> +
> +	return fcs_bytes;
> +}
>

Oh well, this is so ugly, why isn't skb_header_pointer() used ?

Untested patch :

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 94224c22ecc310a87b6715051e335446f29bec03..11129e3a50d6f3b9a49861a99023541720bbcbe4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -713,43 +713,12 @@ static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff *skb)
        rq->stats->ecn_mark += !!rc;
 }
 
-static __be32 mlx5e_get_fcs(struct sk_buff *skb)
+static __be32 mlx5e_get_fcs(const struct sk_buff *skb)
 {
-       int last_frag_sz, bytes_in_prev, nr_frags;
-       u8 *fcs_p1, *fcs_p2;
-       skb_frag_t *last_frag;
        __be32 fcs_bytes;
 
-       if (!skb_is_nonlinear(skb))
-               return *(__be32 *)(skb->data + skb->len - ETH_FCS_LEN);
-
-       nr_frags = skb_shinfo(skb)->nr_frags;
-       last_frag = &skb_shinfo(skb)->frags[nr_frags - 1];
-       last_frag_sz = skb_frag_size(last_frag);
-
-       /* If all FCS data is in last frag */
-       if (last_frag_sz >= ETH_FCS_LEN)
-               return *(__be32 *)(skb_frag_address(last_frag) +
-                                  last_frag_sz - ETH_FCS_LEN);
-
-       fcs_p2 = (u8 *)skb_frag_address(last_frag);
-       bytes_in_prev = ETH_FCS_LEN - last_frag_sz;
-
-       /* Find where the other part of the FCS is - Linear or another frag */
-       if (nr_frags == 1) {
-               fcs_p1 = skb_tail_pointer(skb);
-       } else {
-               skb_frag_t *prev_frag = &skb_shinfo(skb)->frags[nr_frags - 2];
-
-               fcs_p1 = skb_frag_address(prev_frag) +
-                           skb_frag_size(prev_frag);
-       }
-       fcs_p1 -= bytes_in_prev;
-
-       memcpy(&fcs_bytes, fcs_p1, bytes_in_prev);
-       memcpy(((u8 *)&fcs_bytes) + bytes_in_prev, fcs_p2, last_frag_sz);
-
-       return fcs_bytes;
+       return *(__be32 *)skb_header_pointer(skb, skb->len - ETH_FCS_LEN,
+                                            ETH_FCS_LEN, &fcs_bytes);
 }