From owner-freebsd-net@FreeBSD.ORG Mon Jul 22 14:20:02 2013 Return-Path: Delivered-To: freebsd-net@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 115B8161 for ; Mon, 22 Jul 2013 14:20:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id E70052111 for ; Mon, 22 Jul 2013 14:20:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id r6MEK1CE094831 for ; Mon, 22 Jul 2013 14:20:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r6MEK1YE094830; Mon, 22 Jul 2013 14:20:01 GMT (envelope-from gnats) Date: Mon, 22 Jul 2013 14:20:01 GMT Message-Id: <201307221420.r6MEK1YE094830@freefall.freebsd.org> To: freebsd-net@FreeBSD.org Cc: From: Meny Yossefi Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: Meny Yossefi List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Jul 2013 14:20:02 -0000 The following reply was made to PR kern/180430; it has been noted by GNATS. From: Meny Yossefi To: John Baldwin , "bug-followup@freebsd.org" Cc: Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets Date: Mon, 22 Jul 2013 14:11:51 +0000 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi John, The problem is that the HW will only calculate csum for parts of the payloa= d, one fragment at a time, while the receiver side, in our case the tcp/ip stack, will expect to valid= ate the packet's payload as a whole. I agree with the change you offered, though one thing bothers me. This change will add two conditions to the send flow which will probably ha= ve an effect on performance. Maybe 'likely' can be useful here ? BTW, I'm not entirely sure, but I think the CSUM_IP flag is always set, so = maybe the first condition is not necessary. What do you think ? -Meny -----Original Message----- From: John Baldwin [mailto:jhb@freebsd.org] Sent: Friday, July 19, 2013 6:29 PM To: bug-followup@freebsd.org; Meny Yossefi Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets Oops, my previous reply didn't make it to the PR itself. Is the problem that the hardware checksum overwrites arbitrary data in the = packet (at the location where the UDP header would be)? Also, I've looked at other drivers, and they all look at the CSUM_* flags t= o determine the right settings. IP fragments will not have CSUM_UDP or CSU= M_TCP set, so I think the more correct fix is this: Index: en_tx.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- en_tx.c (revision 253470) +++ en_tx.c (working copy) @@ -780,8 +780,12 @@ retry: tx_desc->ctrl.srcrb_flags =3D cpu_to_be32(MLX4_WQE_CTRL_CQ_U= PDATE | = MLX4_WQE_CTRL_SOLICITED); if (mb->m_pkthdr.csum_flags & (CSUM_IP|CSUM_TCP|CSUM_UDP)) { - tx_desc->ctrl.srcrb_flags |=3D cpu_to_be32(M= LX4_WQE_CTRL_IP_CSUM | - = MLX4_WQE_CTRL_TCP_UDP_CSUM); + if (mb->m_pkthdr.csum_flags & CSUM_IP) + tx_desc->ctrl.srcrb_flags |= =3D + cpu_to_be32(MLX4_WQE_CTRL= _IP_CSUM); + if (mb->m_pkthdr.csum_flags & (CSUM_TCP|CSUM_= UDP)) + tx_desc->ctrl.srcrb_flags |= =3D + cpu_to_be32(MLX4_WQE_CTRL= _TCP_UDP_CSUM); priv->port_stats.tx_chksum_offload++; } -- John Baldwin --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi John,

 

The problem is that= the HW will only calculate csum for parts of the payload, one fragment at = a time,

while the receiver = side, in our case the tcp/ip stack, will expect to validate the packet̵= 7;s payload as a whole.

 

I agree with the ch= ange you offered, though one thing bothers me.

This change will ad= d two conditions to the send flow which will probably have an effect on per= formance.

Maybe ‘likely= ’ can be useful here ?

 

BTW, I’m not = entirely sure, but I think the CSUM_IP flag is always set, so maybe the fir= st condition is not necessary.

What do you think ?=

 

-Meny

 

 

-----Original Message-----
From: John Baldwin [mailto:jhb@freebsd.org]
Sent: Friday, July 19, 2013 6:29 PM
To: bug-followup@freebsd.org; Meny Yossefi
Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets

 

Oops, my previous reply didn't make it to the PR = itself.

 

Is the problem that the hardware checksum overwri= tes arbitrary data in the packet (at the location where the UDP header woul= d be)?

 

Also, I've looked at other drivers, and they all = look at the CSUM_* flags to determine the right settings.  IP fragment= s will not have CSUM_UDP or CSUM_TCP set, so I think the more correct fix i= s this:

 

Index: en_tx.c

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D

--- en_tx.c      &n= bsp;    (revision 253470)

+++ en_tx.c    &n= bsp;   (working copy)

@@ -780,8 +780,12 @@ retry:

        &= nbsp;      tx_desc->ctrl.srcrb_flags =3D cpu_to= _be32(MLX4_WQE_CTRL_CQ_UPDATE |

        &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;           MLX4_WQE_= CTRL_SOLICITED);

        &= nbsp;      if (mb->m_pkthdr.csum_flags & (C= SUM_IP|CSUM_TCP|CSUM_UDP)) {

-        =             &nb= sp;         tx_desc->ctrl.srcrb_= flags |=3D cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |

-        =             &nb= sp;            =             &nb= sp;            =             &nb= sp;            =             &nb= sp;            =   MLX4_WQE_CTRL_TCP_UDP_CSUM);

+       &n= bsp;            = ;         if (mb->m_pkthdr.csum_= flags & CSUM_IP)

+       &n= bsp;            = ;            &n= bsp;            tx_d= esc->ctrl.srcrb_flags |=3D

+       &n= bsp;            = ;            &n= bsp;            &nbs= p;   cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);

+       &n= bsp;            = ;         if (mb->m_pkthdr.csum_= flags & (CSUM_TCP|CSUM_UDP))

+       &n= bsp;            = ;            &n= bsp;            tx_d= esc->ctrl.srcrb_flags |=3D

+       &n= bsp;            = ;            &n= bsp;            &nbs= p;   cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM);

        &= nbsp;           &nbs= p;          priv->port_stat= s.tx_chksum_offload++;

        &= nbsp;      }

 

--

John Baldwin

--_000_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_--