linux-kernel - Re: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets

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:   Wed, 11 May 2022 15:40:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Harini Katakam <harini.katakam@...inx.com>
Cc:     <nicolas.ferre@...rochip.com>, <davem@...emloft.net>,
        <claudiu.beznea@...rochip.com>, <dumazet@...gle.com>,
        <pabeni@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <michal.simek@...inx.com>,
        <harinikatakamlinux@...il.com>, <radhey.shyam.pandey@...inx.com>
Subject: Re: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented
 packets

On Tue, 10 May 2022 21:58:09 +0530 Harini Katakam wrote:
> data_len in skbuff represents bytes resident in fragment lists or
> unmapped page buffers. For such packets, when data_len is non-zero,
> skb_put cannot be used - this will throw a kernel bug. Hence do not
> use macb_pad_and_fcs for such fragments.
> 
> Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
> Signed-off-by: Harini Katakam <harini.katakam@...inx.com>
> Signed-off-by: Michal Simek <michal.simek@...inx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea@...rochip.com>

I'm confused. When do we *have to* compute the FCS?

This commit seems to indicate that we can't put the FCS so it's okay to
ask the HW to do it. But that's backwards. We should ask the HW to
compute the FCS whenever possible, to save the CPU cycles.

Is there an unstated HW limitation here?

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6434e74c04f1..0b03305ad6a0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1995,7 +1995,8 @@ static unsigned int macb_tx_map(struct macb *bp,
>  			ctrl |= MACB_BF(TX_LSO, lso_ctrl);
>  			ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
>  			if ((bp->dev->features & NETIF_F_HW_CSUM) &&
> -			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl)
> +			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl &&
> +			    (skb->data_len == 0))

nit: unnecessary parenthesis

>  				ctrl |= MACB_BIT(TX_NOCRC);
>  		} else
>  			/* Only set MSS/MFS on payload descriptors
> @@ -2091,9 +2092,11 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  	struct sk_buff *nskb;
>  	u32 fcs;
>  
> +	/* Not available for GSO and fragments */
>  	if (!(ndev->features & NETIF_F_HW_CSUM) ||
>  	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
> -	    skb_shinfo(*skb)->gso_size)	/* Not available for GSO */
> +	    skb_shinfo(*skb)->gso_size ||
> +	    ((*skb)->data_len > 0))
>  		return 0;
>  
>  	if (padlen <= 0) {