[Patch net] mlx5: fixup checksum for ethernet padding

[Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

When an ethernet frame is padded to meet the minimum ethernet frame
size, the padding octets are not covered by the hardware checksum.
Fortunately the padding octets are ususally zero's, which don't affect
checksum. However, we have a switch which pads non-zero octets, this
causes kernel hardware checksum fault repeatedly.

Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
skb checksum is forced to be CHECKSUM_NONE when padding is detected.
After it, we have to keep skb->csum updated, like what we do for FCS.

The logic is a bit complicated when dealing with both FCS and padding,
so I wrap it up in a helper function mlx5e_csum_padding().

I tested this patch with RXFCS on and off, it works fine without any
warning in both cases.

Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 16985ca3248d..93c18647ca74 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -732,6 +732,35 @@ static u8 get_ip_proto(struct sk_buff *skb, __be16 proto)
 					    ((struct ipv6hdr *)ip_p)->nexthdr;
 }
 
+static void mlx5e_csum_padding(struct sk_buff *skb, int network_depth,
+			       __be16 proto, bool has_fcs)
+{
+	u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
+	void *ip_p = skb->data + network_depth;
+	u32 pad_offset, pad_len;
+	void *pad;
+
+	if (likely(frame_len > ETH_ZLEN))
+		return;
+
+	if (proto == htons(ETH_P_IP)) {
+		struct iphdr *ipv4 = ip_p;
+
+		pad_offset =  network_depth + be16_to_cpu(ipv4->tot_len);
+	} else if (proto == htons(ETH_P_IPV6)) {
+		struct ipv6hdr *ipv6 = ip_p;
+
+		pad_offset = network_depth + sizeof(struct ipv6hdr) +
+			     be16_to_cpu(ipv6->payload_len);
+	}
+
+	pad = skb->data + pad_offset;
+	pad_len = frame_len - pad_offset;
+
+	skb->csum = csum_block_add(skb->csum, csum_partial(pad, pad_len, 0),
+				   pad_offset);
+}
+
 static inline void mlx5e_handle_csum(struct net_device *netdev,
 				     struct mlx5_cqe64 *cqe,
 				     struct mlx5e_rq *rq,
@@ -772,6 +801,13 @@ static inline void mlx5e_handle_csum(struct net_device *netdev,
 			skb->csum = csum_block_add(skb->csum,
 						   (__force __wsum)mlx5e_get_fcs(skb),
 						   skb->len - ETH_FCS_LEN);
+
+		/* CQE csum doesn't cover padding octets. And the padding is
+		 * appended prior to calculating and appending the FCS field.
+		 */
+		mlx5e_csum_padding(skb, network_depth, proto,
+				   !!(netdev->features & NETIF_F_RXFCS));
+
 		stats->csum_complete++;
 		return;
 	}
-- 
2.19.1

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

On Tue, Nov 27, 2018 at 3:21 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> +       if (proto == htons(ETH_P_IP)) {
> +               struct iphdr *ipv4 = ip_p;
> +
> +               pad_offset =  network_depth + be16_to_cpu(ipv4->tot_len);
> +       } else if (proto == htons(ETH_P_IPV6)) {
> +               struct ipv6hdr *ipv6 = ip_p;
> +
> +               pad_offset = network_depth + sizeof(struct ipv6hdr) +
> +                            be16_to_cpu(ipv6->payload_len);
> +       }
> +

Should return here for other protocols... Will fix this in v2.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Eric Dumazet]

On 11/27/2018 03:21 PM, Cong Wang wrote:
> When an ethernet frame is padded to meet the minimum ethernet frame
> size, the padding octets are not covered by the hardware checksum.
> Fortunately the padding octets are ususally zero's, which don't affect
> checksum. However, we have a switch which pads non-zero octets, this
> causes kernel hardware checksum fault repeatedly.
> 
> Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
> skb checksum is forced to be CHECKSUM_NONE when padding is detected.
> After it, we have to keep skb->csum updated, like what we do for FCS.
> 
> The logic is a bit complicated when dealing with both FCS and padding,
> so I wrap it up in a helper function mlx5e_csum_padding().
> 
> I tested this patch with RXFCS on and off, it works fine without any
> warning in both cases.
> 
> Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 16985ca3248d..93c18647ca74 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -732,6 +732,35 @@ static u8 get_ip_proto(struct sk_buff *skb, __be16 proto)
>  					    ((struct ipv6hdr *)ip_p)->nexthdr;
>  }
>  
> +static void mlx5e_csum_padding(struct sk_buff *skb, int network_depth,
> +			       __be16 proto, bool has_fcs)
> +{
> +	u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
> +	void *ip_p = skb->data + network_depth;
> +	u32 pad_offset, pad_len;
> +	void *pad;
> +
> +	if (likely(frame_len > ETH_ZLEN))
> +		return;
> +


But the padding might be added on normal packets (say 1000 bytes + 3 bytes of padding) ?

The bug here is that mlx5 csum only includes the data in IP frame.

I would simply force skb->ip_summed to CHECKSUM_NONE if any padding is detected.

Otherwise, your patch needs more work when multiple frags are used (ie num_frags > 1 )

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 11/27/2018 03:21 PM, Cong Wang wrote:
> > When an ethernet frame is padded to meet the minimum ethernet frame
> > size, the padding octets are not covered by the hardware checksum.
> > Fortunately the padding octets are ususally zero's, which don't affect
> > checksum. However, we have a switch which pads non-zero octets, this
> > causes kernel hardware checksum fault repeatedly.
> >
> > Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
> > skb checksum is forced to be CHECKSUM_NONE when padding is detected.
> > After it, we have to keep skb->csum updated, like what we do for FCS.
> >
> > The logic is a bit complicated when dealing with both FCS and padding,
> > so I wrap it up in a helper function mlx5e_csum_padding().
> >
> > I tested this patch with RXFCS on and off, it works fine without any
> > warning in both cases.
> >
> > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 36 +++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > index 16985ca3248d..93c18647ca74 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > @@ -732,6 +732,35 @@ static u8 get_ip_proto(struct sk_buff *skb, __be16 proto)
> >                                           ((struct ipv6hdr *)ip_p)->nexthdr;
> >  }
> >
> > +static void mlx5e_csum_padding(struct sk_buff *skb, int network_depth,
> > +                            __be16 proto, bool has_fcs)
> > +{
> > +     u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
> > +     void *ip_p = skb->data + network_depth;
> > +     u32 pad_offset, pad_len;
> > +     void *pad;
> > +
> > +     if (likely(frame_len > ETH_ZLEN))
> > +             return;
> > +
>
>
> But the padding might be added on normal packets (say 1000 bytes + 3 bytes of padding) ?

I never see other padding cases than ETH_ZLEN. Does ethernet standard
require padding for other cases? I only read the section "3.2.8 Pad field" in
the standard.

>
> The bug here is that mlx5 csum only includes the data in IP frame.
>
> I would simply force skb->ip_summed to CHECKSUM_NONE if any padding is detected.
>
> Otherwise, your patch needs more work when multiple frags are used (ie num_frags > 1 )

You mean using skb_header_pointer()? Yeah if we need to handle cases
larger than ETH_ZLEN.

Thanks.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Saeed Mahameed]

On Tue, 2018-11-27 at 16:07 -0800, Cong Wang wrote:
> On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
> > 
> > 
> > On 11/27/2018 03:21 PM, Cong Wang wrote:
> > > When an ethernet frame is padded to meet the minimum ethernet
> > > frame
> > > size, the padding octets are not covered by the hardware
> > > checksum.
> > > Fortunately the padding octets are ususally zero's, which don't
> > > affect
> > > checksum. However, we have a switch which pads non-zero octets,
> > > this
> > > causes kernel hardware checksum fault repeatedly.
> > > 
> > > Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and
> > > CHECKSUM_COMPLETE are friends"),
> > > skb checksum is forced to be CHECKSUM_NONE when padding is
> > > detected.
> > > After it, we have to keep skb->csum updated, like what we do for
> > > FCS.
> > > 
> > > The logic is a bit complicated when dealing with both FCS and
> > > padding,
> > > so I wrap it up in a helper function mlx5e_csum_padding().
> > > 
> > > I tested this patch with RXFCS on and off, it works fine without
> > > any
> > > warning in both cases.
> > > 
> > > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and
> > > CHECKSUM_COMPLETE are friends"),
> > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > ---
> > >  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 36
> > > +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > index 16985ca3248d..93c18647ca74 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > @@ -732,6 +732,35 @@ static u8 get_ip_proto(struct sk_buff *skb,
> > > __be16 proto)
> > >                                           ((struct ipv6hdr
> > > *)ip_p)->nexthdr;
> > >  }
> > > 
> > > +static void mlx5e_csum_padding(struct sk_buff *skb, int
> > > network_depth,
> > > +                            __be16 proto, bool has_fcs)
> > > +{
> > > +     u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb-
> > > >len;
> > > +     void *ip_p = skb->data + network_depth;
> > > +     u32 pad_offset, pad_len;
> > > +     void *pad;
> > > +
> > > +     if (likely(frame_len > ETH_ZLEN))
> > > +             return;
> > > +
> > 
> > But the padding might be added on normal packets (say 1000 bytes +
> > 3 bytes of padding) ?
> 
> I never see other padding cases than ETH_ZLEN. Does ethernet standard
> require padding for other cases? I only read the section "3.2.8 Pad
> field" in
> the standard.
> 

hmm, so we are going to parse each and every packets ? to find if there
is an end  padding ? that is costly (packet rate wise)!

if the ETH_ZLEN check is good enough for now, let's stick with that.
We are currently exploring our options if we can W/A this in HW/FW and
avoid the extra driver overhead.

> > The bug here is that mlx5 csum only includes the data in IP frame.
> > 
> > I would simply force skb->ip_summed to CHECKSUM_NONE if any padding
> > is detected.
> > 

Totally agree, let's just skip csum completes for very small packets 
( < ETH_ZLEN ).

> > Otherwise, your patch needs more work when multiple frags are used
> > (ie num_frags > 1 )
> 
> You mean using skb_header_pointer()? Yeah if we need to handle cases
> larger than ETH_ZLEN.
> 
> Thanks.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Eric Dumazet]

On 11/27/2018 04:07 PM, Cong Wang wrote:
> On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:

>>
>> But the padding might be added on normal packets (say 1000 bytes + 3 bytes of padding) ?
> 
> I never see other padding cases than ETH_ZLEN. Does ethernet standard
> require padding for other cases? I only read the section "3.2.8 Pad field" in
> the standard.
> 

Padding can be done by senders, eg using AF_PACKET,
added at the tail of a regular IP/IP6 frame of 1000 or 6000 bytes.

Note that mlx5 will presumably set CHECKSUM_UNNECESSARY for standard protocols,
so if you want to reproduce the issue, you might need to find an IP frame that
mlx5 is not able to checksum validate.

Maybe a non-first fragment will do the trick.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

On Tue, Nov 27, 2018 at 5:11 PM Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Tue, 2018-11-27 at 16:07 -0800, Cong Wang wrote:
> > On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com>
> > wrote:
> > > The bug here is that mlx5 csum only includes the data in IP frame.
> > >
> > > I would simply force skb->ip_summed to CHECKSUM_NONE if any padding
> > > is detected.
> > >
>
> Totally agree, let's just skip csum completes for very small packets
> ( < ETH_ZLEN ).

That is my first idea in my mind, but you know we still have to parse IP header
to detect packets < ETH_ZLEN. Essentially same work, only saves a few adds
in csum_add(). ;)

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

On Tue, Nov 27, 2018 at 5:25 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 11/27/2018 04:07 PM, Cong Wang wrote:
> > On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> >>
> >> But the padding might be added on normal packets (say 1000 bytes + 3 bytes of padding) ?
> >
> > I never see other padding cases than ETH_ZLEN. Does ethernet standard
> > require padding for other cases? I only read the section "3.2.8 Pad field" in
> > the standard.
> >
>
> Padding can be done by senders, eg using AF_PACKET,
> added at the tail of a regular IP/IP6 frame of 1000 or 6000 bytes.


I tried the trafgen script you provided, it doesn't trigger any checksum fault.
So, it is probably a different case.

>
> Note that mlx5 will presumably set CHECKSUM_UNNECESSARY for standard protocols,
> so if you want to reproduce the issue, you might need to find an IP frame that
> mlx5 is not able to checksum validate.

This warning is 100% reproducible with a TCP RST packet (no data)
here, after I find the right switch which pads non-zero's. This is
also how I verified this patch.

Thanks.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Eric Dumazet]

On Tue, Nov 27, 2018 at 5:42 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, Nov 27, 2018 at 5:25 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > On 11/27/2018 04:07 PM, Cong Wang wrote:
> > > On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > >>
> > >> But the padding might be added on normal packets (say 1000 bytes + 3 bytes of padding) ?
> > >
> > > I never see other padding cases than ETH_ZLEN. Does ethernet standard
> > > require padding for other cases? I only read the section "3.2.8 Pad field" in
> > > the standard.
> > >
> >
> > Padding can be done by senders, eg using AF_PACKET,
> > added at the tail of a regular IP/IP6 frame of 1000 or 6000 bytes.
>
>
> I tried the trafgen script you provided, it doesn't trigger any checksum fault.
> So, it is probably a different case.

As I said, you need to cook a packet that the NIC is not able to L4
checksum-verify.

Maybe an encapsulated packet could do the trick, but only Mellanox
experts can say.

>
> >
> > Note that mlx5 will presumably set CHECKSUM_UNNECESSARY for standard protocols,
> > so if you want to reproduce the issue, you might need to find an IP frame that
> > mlx5 is not able to checksum validate.
>
> This warning is 100% reproducible with a TCP RST packet (no data)
> here, after I find the right switch which pads non-zero's. This is
> also how I verified this patch.

Sure this patch handles short frames, but I am saying the issue is
more generic than that.

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[Cong Wang]

On Tue, Nov 27, 2018 at 5:49 PM Eric Dumazet <edumazet@google.com> wrote:
>
>
> Sure this patch handles short frames, but I am saying the issue is
> more generic than that.

Yeah. Let's fix one problem in one patch.

I will clarify that I am only fixing short frames in v2, as I need to update
it anyway.

Thanks!

Re: [Patch net] mlx5: fixup checksum for ethernet padding

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Hi Cong,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cong-Wang/mlx5-fixup-checksum-for-ethernet-padding/20181130-014928
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=ia64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/mellanox/mlx5/core/en_rx.c: In function 'mlx5e_csum_padding':
>> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c:740:6: warning: 'pad_offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
     u32 pad_offset, pad_len;
         ^~~~~~~~~~

vim +/pad_offset +740 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c

   734	
   735	static void mlx5e_csum_padding(struct sk_buff *skb, int network_depth,
   736				       __be16 proto, bool has_fcs)
   737	{
   738		u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
   739		void *ip_p = skb->data + network_depth;
 > 740		u32 pad_offset, pad_len;
   741		void *pad;
   742	
   743		if (likely(frame_len > ETH_ZLEN))
   744			return;
   745	
   746		if (proto == htons(ETH_P_IP)) {
   747			struct iphdr *ipv4 = ip_p;
   748	
   749			pad_offset =  network_depth + be16_to_cpu(ipv4->tot_len);
   750		} else if (proto == htons(ETH_P_IPV6)) {
   751			struct ipv6hdr *ipv6 = ip_p;
   752	
   753			pad_offset = network_depth + sizeof(struct ipv6hdr) +
   754				     be16_to_cpu(ipv6->payload_len);
   755		}
   756	
   757		pad = skb->data + pad_offset;
   758		pad_len = frame_len - pad_offset;
   759	
   760		skb->csum = csum_block_add(skb->csum, csum_partial(pad, pad_len, 0),
   761					   pad_offset);
   762	}
   763	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52363 bytes --]