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

[John Baldwin]

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

From: John Baldwin <jhb at freebsd.org>
To: Meny Yossefi <menyy at mellanox.com>
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: Mon, 22 Jul 2013 11:40:08 -0400

 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 validate 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 gain
 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 added.
 
 > 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 ?
 
 If the user uses ifconfig to disable checksum offload and force software
 checksums 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; Meny Yossefi
 > 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 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 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
 > 
 > ===================================================================
 > 
 > --- en_tx.c           (revision 253470)
 > 
 > +++ en_tx.c        (working copy)
 > 
 > @@ -780,8 +780,12 @@ retry:
 > 
 >                tx_desc->ctrl.srcrb_flags = 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 |= 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 |=
 > 
 > +                                                 cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);
 > 
 > +                             if (mb->m_pkthdr.csum_flags & (CSUM_TCP|CSUM_UDP))
 > 
 > +                                             tx_desc->ctrl.srcrb_flags |=
 > 
 > +                                                 cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM);
 > 
 >                                priv->port_stats.tx_chksum_offload++;
 > 
 >                }
 > 
 > 
 > 
 > --
 > 
 > John Baldwin
 > 
 
 -- 
 John Baldwin