[PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets

[PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets

[Harini Katakam]

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@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
v2:
Add fixes tag and reviewed-by tag

 drivers/net/ethernet/cadence/macb_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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))
 				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) {
-- 
2.17.1

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

[Jakub Kicinski]

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@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.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) {

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

[Harini Katakam]

Hi Jakub,

On Thu, May 12, 2022 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> > Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.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?

Thanks for the review. The top level summary is that there CSUM
offload is enabled by
via NETIF_F_HW_CSUM (and universally in IP registers) and then
selectively disabled for
certain packets (using NOCRC bit in buffer descriptors) where the
application intentionally
performs CSUM and HW should not replace it, for ex. forwarding usecases.
I'm modifying this list of exceptions with this patch.

This was due to HW limitation (see
https://www.spinics.net/lists/netdev/msg505065.html).
Further to this, Claudiu added macb_pad_and_fcs support. Please see
comment starting
with "It was reported in" below:
https://lists.openwall.net/netdev/2018/10/30/76

Hope this helps.
I'll fix the nit and send another version.

Regards,
Harini

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

[Jakub Kicinski]

On Thu, 12 May 2022 12:26:15 +0530 Harini Katakam wrote:
> On Thu, May 12, 2022 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > 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@xilinx.com>
> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> > > Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.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?  
> 
> Thanks for the review. The top level summary is that there CSUM
> offload is enabled by
> via NETIF_F_HW_CSUM (and universally in IP registers) and then
> selectively disabled for
> certain packets (using NOCRC bit in buffer descriptors) where the
> application intentionally
> performs CSUM and HW should not replace it, for ex. forwarding usecases.
> I'm modifying this list of exceptions with this patch.
> 
> This was due to HW limitation (see
> https://www.spinics.net/lists/netdev/msg505065.html).
> Further to this, Claudiu added macb_pad_and_fcs support. Please see
> comment starting
> with "It was reported in" below:
> https://lists.openwall.net/netdev/2018/10/30/76
> 
> Hope this helps.
> I'll fix the nit and send another version.

So the NOCRC bit controls both ethernet and transport protocol
checksums? The CRC in the name is a little confusing.

Are you sure commit 403dc16796f5 ("cadence: force nonlinear buffers to
be cloned") does not fix the case you're trying to address?

RE: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets

[Harini Katakam]

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, May 12, 2022 9:15 PM
> To: Harini Katakam <harinik@xilinx.com>
> Cc: Harini Katakam <harinik@xilinx.com>; Nicolas Ferre
> <nicolas.ferre@microchip.com>; David Miller <davem@davemloft.net>;
> Claudiu Beznea <claudiu.beznea@microchip.com>; dumazet@google.com;
> Paolo Abeni <pabeni@redhat.com>; netdev <netdev@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Michal Simek
> <michals@xilinx.com>; Radhey Shyam Pandey <radheys@xilinx.com>
> Subject: Re: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented
> packets
> 
> On Thu, 12 May 2022 12:26:15 +0530 Harini Katakam wrote:
> > On Thu, May 12, 2022 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > 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@xilinx.com>
> > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > > > <radhey.shyam.pandey@xilinx.com>
> > > > Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.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?
> >
> > Thanks for the review. The top level summary is that there CSUM
> > offload is enabled by via NETIF_F_HW_CSUM (and universally in IP
> > registers) and then selectively disabled for certain packets (using
> > NOCRC bit in buffer descriptors) where the application intentionally
> > performs CSUM and HW should not replace it, for ex. forwarding usecases.
> > I'm modifying this list of exceptions with this patch.
> >
> > This was due to HW limitation (see
> > https://www.spinics.net/lists/netdev/msg505065.html).
> > Further to this, Claudiu added macb_pad_and_fcs support. Please see
> > comment starting with "It was reported in" below:
> > https://lists.openwall.net/netdev/2018/10/30/76
> >
> > Hope this helps.
> > I'll fix the nit and send another version.
> 
> So the NOCRC bit controls both ethernet and transport protocol checksums?
> The CRC in the name is a little confusing.

Yes

> 
> Are you sure commit 403dc16796f5 ("cadence: force nonlinear buffers to be
> cloned") does not fix the case you're trying to address?

Thanks for this pointer. Yes, this patch already addresses the bug on linear skb buffers
and solves the issue I was hitting.

Apologies for the inconvenience. Please ignore this patch.

I had to unfortunately work on an tightly coupled older kernel and app to reproduce this
particular issue and hence could only verify whether my patch *works* on 5.18, not whether
it was *needed*. I backported 403dc16796f5 ("cadence: force nonlinear buffers to be cloned")
and it works as expected.

Regards,
Harini