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

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]

Message-ID: <0d9c022b4b00c2ed0e0af9bdb14ad7e53703872f.camel@mellanox.com>
Date:   Wed, 28 Nov 2018 01:11:18 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>
Subject: Re: [Patch net] mlx5: fixup checksum for ethernet padding

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@...il.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@...lanox.com>
> > > Cc: Eric Dumazet <edumazet@...gle.com>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@...il.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.