From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [Patch net v2] mlx5: fixup checksum for short ethernet frame padding Date: Wed, 28 Nov 2018 07:00:46 -0800 Message-ID: References: <20181128061013.3885-1-xiyou.wangcong@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: netdev , Saeed Mahameed To: Cong Wang Return-path: Received: from mail-io1-f66.google.com ([209.85.166.66]:40351 "EHLO mail-io1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728251AbeK2CC4 (ORCPT ); Wed, 28 Nov 2018 21:02:56 -0500 Received: by mail-io1-f66.google.com with SMTP id n9so20195084ioh.7 for ; Wed, 28 Nov 2018 07:00:58 -0800 (PST) In-Reply-To: <20181128061013.3885-1-xiyou.wangcong@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Nov 27, 2018 at 10:10 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 need 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 > Cc: Eric Dumazet > Signed-off-by: Cong Wang > --- > .../net/ethernet/mellanox/mlx5/core/en_rx.c | 39 +++++++++++++++++++ > 1 file changed, 39 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..9b6bd2b51556 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > @@ -732,6 +732,37 @@ 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; > + u32 pad_offset, pad_len; > + void *pad, *ip_p; > + > + /* We only handle short frames here, and we know they are linear. */ > + if (likely(frame_len > ETH_ZLEN)) > + return; > + > + ip_p = skb->data + network_depth; > + if (proto == htons(ETH_P_IP)) { > + struct iphdr *ipv4 = ip_p; > + > + pad_offset = network_depth + be16_to_cpu(ipv4->tot_len); > + } else { /* Either IPv4 or IPv6 here. */ > + struct ipv6hdr *ipv6 = ip_p; > + > + pad_offset = network_depth + sizeof(struct ipv6hdr) + > + be16_to_cpu(ipv6->payload_len); > + } > + pad_len = frame_len - pad_offset; > + if (pad_len == 0) > + return; Nice packet of death alert. pad_len can be 0xFFFFFF67 here, if frame_len is smaller than pad_offset. Really I suggest you set ip_summed to CHECKSUM_NONE, then remove the initial test ( if (likely(frame_len > ETH_ZLEN)) ...) Until the firmware is fixed. Otherwise frames with a wrong checksum and some non zero padding could potentially be seen as correct frames. (Probability of 1/65536) Do not focus on your immediate problem (small packets being padded by a non malicious entity) > + > + pad = skb->data + 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 +803,14 @@ 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 in short ethernet > + * frames. And the pad field 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 >