kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets

Tue Jul 23 14:10:01 UTC 2013

The following reply was made to PR kern/180430; it has been noted by GNATS.

From: Meny Yossefi <menyy at mellanox.com>
To: John Baldwin <jhb at freebsd.org>
Cc: "bug-followup at freebsd.org" <bug-followup at freebsd.org>
Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for
 fragmented packets
Date: Tue, 23 Jul 2013 14:01:23 +0000

 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_
 Content-Type: text/plain; charset="us-ascii"
 Content-Transfer-Encoding: quoted-printable

 Let's stick with the FreeBSD standards (without the likely/unlikely)

 Let me just double check the CSUM flags work as expected.

 I'll get back to you as soon as I'm done.

 -Meny

 -----Original Message-----
 From: John Baldwin [mailto:jhb at freebsd.org]
 Sent: Monday, July 22, 2013 7:04 PM
 To: Meny Yossefi
 Cc: bug-followup at freebsd.org
 Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment=
 ed packets

 On Monday, July 22, 2013 10:11:51 am Meny Yossefi wrote:

 > 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 val=
 idate the packet's payload as a whole.

 Ok.

 > 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 =
 have an effect on performance.

 >

 > Maybe 'likely' can be useful here ?

 FreeBSD tends to not use likely/unlikely unless there is a demonstrable gai=
 n via benchmark measurements.  However, if the OFED code regularly uses it =
 and you feel this is a case where you would normally use it, it can be adde=
 d.

 > BTW, I'm not entirely sure, but I think the CSUM_IP flag is always set, s=
 o maybe the first condition is not necessary.

 >

 > What do you think ?

 If the user uses ifconfig to disable checksum offload and force software ch=
 ecksums the flag will not be set.

 > -Meny

 >

 >

 >

 >

 >

 > -----Original Message-----

 > From: John Baldwin [mailto:jhb at freebsd.org]

 > Sent: Friday, July 19, 2013 6:29 PM

 > To: bug-followup at freebsd.org<mailto:bug-followup at freebsd.org>; Meny Yosse=
 fi

 > Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for

 > fragmented packets

 >

 >

 >

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

 >

 >

 >

 > Is the problem that the hardware checksum overwrites arbitrary data in th=
 e 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 to determine the right settings.  IP fragments will not have

 > CSUM_UDP or

 CSUM_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_UPDATE |

 >

 >

 > 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=
 (MLX4_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

 >

 --

 John Baldwin

 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_
 Content-Type: text/html; charset="us-ascii"
 Content-Transfer-Encoding: quoted-printable

 <html xmlns:v=3D"urn:schemas-microsoft-com:vml" xmlns:o=3D"urn:schemas-micr=
 osoft-com:office:office" xmlns:w=3D"urn:schemas-microsoft-com:office:word" =
 xmlns:m=3D"http://schemas.microsoft.com/office/2004/12/omml" xmlns=3D"http:=
 //www.w3.org/TR/REC-html40">
 <head>
 <meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Dus-ascii"=
 >
 <meta name=3D"Generator" content=3D"Microsoft Word 14 (filtered medium)">
 <style><!--
 /* Font Definitions */
 @font-face
 	{font-family:"Cambria Math";
 	panose-1:2 4 5 3 5 4 6 3 2 4;}
 @font-face
 	{font-family:Calibri;
 	panose-1:2 15 5 2 2 2 4 3 2 4;}
 @font-face
 	{font-family:Tahoma;
 	panose-1:2 11 6 4 3 5 4 4 2 4;}
 /* Style Definitions */
 p.MsoNormal, li.MsoNormal, div.MsoNormal
 	{margin:0in;
 	margin-bottom:.0001pt;
 	font-size:11.0pt;
 	font-family:"Calibri","sans-serif";}
 a:link, span.MsoHyperlink
 	{mso-style-priority:99;
 	color:blue;
 	text-decoration:underline;}
 a:visited, span.MsoHyperlinkFollowed
 	{mso-style-priority:99;
 	color:purple;
 	text-decoration:underline;}
 p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
 	{mso-style-priority:99;
 	mso-style-link:"Plain Text Char";
 	margin:0in;
 	margin-bottom:.0001pt;
 	font-size:11.0pt;
 	font-family:"Calibri","sans-serif";}
 p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
 	{mso-style-priority:99;
 	mso-style-link:"Balloon Text Char";
 	margin:0in;
 	margin-bottom:.0001pt;
 	font-size:8.0pt;
 	font-family:"Tahoma","sans-serif";}
 span.PlainTextChar
 	{mso-style-name:"Plain Text Char";
 	mso-style-priority:99;
 	mso-style-link:"Plain Text";
 	font-family:"Calibri","sans-serif";}
 span.BalloonTextChar
 	{mso-style-name:"Balloon Text Char";
 	mso-style-priority:99;
 	mso-style-link:"Balloon Text";
 	font-family:"Tahoma","sans-serif";}
 .MsoChpDefault
 	{mso-style-type:export-only;
 	font-family:"Calibri","sans-serif";}
 @page WordSection1
 	{size:8.5in 11.0in;
 	margin:1.0in 1.0in 1.0in 1.0in;}
 div.WordSection1
 	{page:WordSection1;}
 --></style><!--[if gte mso 9]><xml>
 <o:shapedefaults v:ext=3D"edit" spidmax=3D"1026" />
 </xml><![endif]--><!--[if gte mso 9]><xml>
 <o:shapelayout v:ext=3D"edit">
 <o:idmap v:ext=3D"edit" data=3D"1" />
 </o:shapelayout></xml><![endif]-->
 </head>
 <body lang=3D"EN-US" link=3D"blue" vlink=3D"purple">
 <div class=3D"WordSection1">
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">Let's stick with th=
 e FreeBSD standards (without the likely/unlikely)<o:p></o:p></span></p>
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D"><o:p> </o:p></=
 span></p>
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">Let me just double =
 check the CSUM flags work as expected.<o:p></o:p></span></p>
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">I’ll get back=
  to you as soon as I’m done.<o:p></o:p></span></p>
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D"><o:p> </o:p></=
 span></p>
 <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">-Meny <o:p></o:p></=
 span></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">-----Original Message-----<br>
 From: John Baldwin [mailto:jhb at freebsd.org] <br>
 Sent: Monday, July 22, 2013 7:04 PM<br>
 To: Meny Yossefi<br>
 Cc: bug-followup at freebsd.org<br>
 Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment=
 ed packets</p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">On Monday, July 22, 2013 10:11:51 am Meny Yossefi=
  wrote:<o:p></o:p></p>
 <p class=3D"MsoPlainText">> Hi John,<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> The problem is that the HW will only calcula=
 te csum for parts of the
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> payload, one fragment at a time,<o:p></o:p><=
 /p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> while the receiver side, in our case the tcp=
 /ip stack, will expect to validate the packet's payload as a whole.<o:p></o=
 :p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">Ok.<o:p></o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">> I agree with the change you offered, though =
 one thing bothers me.<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> This change will add two conditions to the s=
 end flow which will probably have an effect on performance.<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> Maybe 'likely' can be useful here ?<o:p></o:=
 p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">FreeBSD tends to not use likely/unlikely unless t=
 here is a demonstrable gain via benchmark measurements.  However, if t=
 he OFED code regularly uses it and you feel this is a case where you would =
 normally use it, it can be added.<o:p></o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">> BTW, I'm not entirely sure, but I think the =
 CSUM_IP flag is always set, so maybe the first condition is not necessary.<=
 o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> What do you think ?<o:p></o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">If the user uses ifconfig to disable checksum off=
 load and force software checksums the flag will not be set.<o:p></o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">> -Meny<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> -----Original Message-----<o:p></o:p></p>
 <p class=3D"MsoPlainText">> From: John Baldwin [<a href=3D"mailto:jhb at fr=
 eebsd.org"><span style=3D"color:windowtext;text-decoration:none">mailto:jhb=
 @freebsd.org</span></a>]<o:p></o:p></p>
 <p class=3D"MsoPlainText">> Sent: Friday, July 19, 2013 6:29 PM<o:p></o:=
 p></p>
 <p class=3D"MsoPlainText">> To: <a href=3D"mailto:bug-followup at freebsd.o=
 rg"><span style=3D"color:windowtext;text-decoration:none">bug-followup at free=
 bsd.org</span></a>; Meny Yossefi<o:p></o:p></p>
 <p class=3D"MsoPlainText">> Subject: Re: kern/180430: [ofed] [patch] Bad=
  UDP checksum calc for
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> fragmented packets<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> Oops, my previous reply didn't make it to th=
 e PR itself.<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> Is the problem that the hardware checksum ov=
 erwrites arbitrary data in the packet (at the location where the UDP header=
  would be)?<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> Also, I've looked at other drivers, and they=
  all look at the CSUM_*
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> flags to determine the right settings. =
  IP fragments will not have
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> CSUM_UDP or<o:p></o:p></p>
 <p class=3D"MsoPlainText">CSUM_TCP set, so I think the more correct fix is =
 this:<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> Index: en_tx.c<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> =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<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> --- en_tx.c     &nb=
 sp;     (revision 253470)<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +++ en_tx.c   &nb=
 sp;    (working copy)<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> @@ -780,8 +780,12 @@ retry:<o:p></o:p></=
 p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">>       &nb=
 sp;        tx_desc->ctrl.srcrb_flags =
 =3D <o:p></o:p></p>
 <p class=3D"MsoPlainText">> cpu_to_be32(MLX4_WQE_CTRL_CQ_UPDATE |<o:p></=
 o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">>       &nb=
 sp;            =
             &nb=
 sp;            =
             &nb=
 sp;            =
             &nb=
 sp;            
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> MLX4_WQE_CTRL_SOLICITED);<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">>       &nb=
 sp;        if (mb->m_pkthdr.csum_flag=
 s & <o:p></o:p></p>
 <p class=3D"MsoPlainText">> (CSUM_IP|CSUM_TCP|CSUM_UDP)) {<o:p></o:p></p=
 >
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> -       &=
 nbsp;           &nbs=
 p;          tx_desc->ctrl.s=
 rcrb_flags |=3D cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> -       &=
 nbsp;           &nbs=
 p;            &=
 nbsp;           &nbs=
 p;            &=
 nbsp;           &nbs=
 p;            &=
 nbsp;           &nbs=
 p;            &=
 nbsp;  MLX4_WQE_CTRL_TCP_UDP_CSUM);<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +       &n=
 bsp;           &nbsp=
 ;         if (mb->m_pkthdr.=
 csum_flags & CSUM_IP)<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +      &nb=
 sp;            =
             &nb=
 sp;            =
  <o:p></o:p></p>
 <p class=3D"MsoPlainText">> + tx_desc->ctrl.srcrb_flags |=3D<o:p>=
 </o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +      &nb=
 sp;            =
             &nb=
 sp;            =
      <o:p>
 </o:p></p>
 <p class=3D"MsoPlainText">> + cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);<o:=
 p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +      &nb=
 sp;            &nbsp=
 ;         if (mb->m_pkthdr.=
 csum_flags &
 <o:p></o:p></p>
 <p class=3D"MsoPlainText">> + (CSUM_TCP|CSUM_UDP))<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +      &nb=
 sp;            =
             &nb=
 sp;            =
  <o:p></o:p></p>
 <p class=3D"MsoPlainText">> + tx_desc->ctrl.srcrb_flags |=3D<o:p>=
 </o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> +      &nb=
 sp;            =
             &nb=
 sp;            =
      <o:p>
 </o:p></p>
 <p class=3D"MsoPlainText">> + cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM=
 );<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">>       &nb=
 sp;            =
             priv-&gt=
 ;port_stats.tx_chksum_offload++;<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">>       &nb=
 sp;        }<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> --<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText">> John Baldwin<o:p></o:p></p>
 <p class=3D"MsoPlainText">> <o:p></o:p></p>
 <p class=3D"MsoPlainText"><o:p> </o:p></p>
 <p class=3D"MsoPlainText">--<o:p></o:p></p>
 <p class=3D"MsoPlainText">John Baldwin<o:p></o:p></p>
 </div>
 </body>
 </html>

 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_--