kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets
Meny Yossefi
menyy at mellanox.com
Wed Jul 24 13:20: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: Wed, 24 Jul 2013 13:14:53 +0000
--_000_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Hi John,
Looks good.
-Meny
From: Meny Yossefi
Sent: Tuesday, July 23, 2013 5:01 PM
To: 'John Baldwin'
Cc: 'bug-followup at freebsd.org'
Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment=
ed packets
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<mailto: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_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_
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";}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@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"MsoNormal"><span style=3D"color:#1F497D">Hi John, <o:p></o:p></=
span></p>
<p class=3D"MsoNormal"><span style=3D"color:#1F497D"><o:p> </o:p></spa=
n></p>
<p class=3D"MsoNormal"><span style=3D"color:#1F497D">Looks good. <o:p></o:p=
></span></p>
<p class=3D"MsoNormal"><span style=3D"color:#1F497D"><o:p> </o:p></spa=
n></p>
<div>
<p class=3D"MsoNormal"><span style=3D"color:#1F497D">-Meny <o:p></o:p></spa=
n></p>
</div>
<p class=3D"MsoNormal"><span style=3D"color:#1F497D"><o:p> </o:p></spa=
n></p>
<div>
<div style=3D"border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in =
0in 0in">
<p class=3D"MsoNormal"><b><span style=3D"font-size:10.0pt;font-family:"=
;Tahoma","sans-serif"">From:</span></b><span style=3D"font-s=
ize:10.0pt;font-family:"Tahoma","sans-serif""> Meny Yos=
sefi
<br>
<b>Sent:</b> Tuesday, July 23, 2013 5:01 PM<br>
<b>To:</b> 'John Baldwin'<br>
<b>Cc:</b> 'bug-followup at freebsd.org'<br>
<b>Subject:</b> RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for f=
ragmented packets<o:p></o:p></span></p>
</div>
</div>
<p class=3D"MsoNormal"><o:p> </o:p></p>
<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 [<a href=3D"mailto:jhb at freebsd.org">mailto:jhb at freebsd.o=
rg</a>] <br>
Sent: Monday, July 22, 2013 7:04 PM<br>
To: Meny Yossefi<br>
Cc: <a href=3D"mailto:bug-followup at freebsd.org">bug-followup at freebsd.org</a=
><br>
Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment=
ed packets<o:p></o:p></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;  =
; 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;  =
; 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->=
;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_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_--
More information about the freebsd-net
mailing list