netdev - Re: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for TCP traffic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200707311233.31227.ossthema@de.ibm.com>
Date:	Tue, 31 Jul 2007 12:33:29 +0200
From:	Jan-Bernd Themann <ossthema@...ibm.com>
To:	Andrew Gallatin <gallatin@...i.com>
Cc:	netdev <netdev@...r.kernel.org>,
	Christoph Raisch <raisch@...ibm.com>,
	Jan-Bernd Themann <themann@...ibm.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-ppc <linuxppc-dev@...abs.org>,
	Marcus Eder <meder@...ibm.com>,
	Thomas Klein <tklein@...ibm.com>,
	Stefan Roscher <stefan.roscher@...ibm.com>,
	David Miller <davem@...emloft.net>,
	Jeff Garzik <jeff@...zik.org>
Subject: Re: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for TCP traffic

Hi,

Thanks for finding these bugs! I'll post an updated version soon (2 patches
with no separate Kconfig patches, one LRO and one eHEA patch). See comments below.

Thanks,
Jan-Bernd

On Monday 30 July 2007 22:32, Andrew Gallatin wrote:
> I was working on testing the myri10ge patch, and I ran into a few
> problems.  I've attached a patch to inet_lro.c to fix some of them,
> and a patch to myri10ge.c to show how to use the page based
> interface.  Both patches are signed off by Andrew Gallatin
> <gallatin@...i.com>
> 
> First, the LRO_MAX_PG_HLEN is still a problem.  Minimally sized 60
> byte frames still cause problems in lro_gen_skb due to skb->len
> going negative.  Fixed in the attached patch.  It may be simpler
> to just drop LRO_MAX_PG_HLEN to ETH_ZLEN, but I'm not sure if
> that is enough.  Are there "smart" NICs which might chop off padding
> themselves?

I'd tend to stick to an explicit check as implemented in your patch
for now

> 
> Second, you still need to set skb->ip_summed = CHECKSUM_UNNECESSARY
> when modified packets are flushed, else the stack will see bad
> checksums for packets from CHECKSUM_COMPLETE drivers using the
> skb interface.  Fixed in the attached patch.

I thought about it... As we do update the TCP checksum for aggregated
packets we could add a second ip_summed field in the net_lro_mgr struct 
used for aggregated packets to support HW that does not have any checksum helper
functionality. These drivers could set this ip_summed field to CHECKSUM_NONE, 
and thus leave the checksum check to the stack. I'm not sure if these old devices benefit
a lot from LRO. So what do you think?

> 
> Fourth, I did some traffic sniffing to try to figure out what's going
> on above, and saw tcpdump complain about bad checksums.  Have you tried
> running tcpdump -s 65535 -vvv?  Have you also seen bad checksums?
> I seem to see this for both page- and skb-based versions of the driver.
> 

Hmmm, can't confirm that. For our skb-based version I see
correct checksums for aggregated packets and for the page-based version as well.
I used: (tcpdump -i ethX -s 0 -w dump.bin) in combination with ethereal.
Don't see problems as well with your tcpdump command.

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html