[RFC PATCH] lro: ip fragment checking

[RFC PATCH] lro: ip fragment checking

[Jan-Bernd Themann]

Currently there is no checking in the LRO receive path whether
TCP packets are ip fragmented. We should not consider
those packets for aggregation.
I'm not sure if this checking is actually required. Does anyone
know if it is possible to get fragmented TCP packets without
the tcp stack changing the MSS size?
This patch introduces explicit checking. Any objections?

Regards,

Jan-Bernd

---



 net/ipv4/inet_lro.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
index cfd034a..1f9159d 100644
--- a/net/ipv4/inet_lro.c
+++ b/net/ipv4/inet_lro.c
@@ -64,6 +64,9 @@ static int lro_tcp_ip_check(struct iphdr *iph, struct tcphdr *tcph,
 	if (iph->ihl != IPH_LEN_WO_OPTIONS)
 		return -1;
 
+	if (iph->frag_off & IP_MF)
+		return -1;
+
 	if (tcph->cwr || tcph->ece || tcph->urg || !tcph->ack
 	    || tcph->rst || tcph->syn || tcph->fin)
 		return -1;
-- 
1.5.5

Re: [RFC PATCH] lro: ip fragment checking

[Ben Hutchings]

On Mon, 2008-11-24 at 16:56 +0100, Jan-Bernd Themann wrote:
> Currently there is no checking in the LRO receive path whether
> TCP packets are ip fragmented. We should not consider
> those packets for aggregation.
> I'm not sure if this checking is actually required. Does anyone
> know if it is possible to get fragmented TCP packets without
> the tcp stack changing the MSS size?
> This patch introduces explicit checking. Any objections?

LRO depends on the hardware performing TCP checksum offload, and the TCP
checksum cannot be verified for IP fragments in isolation.  So I think
drivers should not be passing fragments into inet_lro or should reject
them in its get_frag_header() or get_skb_header() method.  Certainly sfc
doesn't pass fragments into inet_lro because they have not been
checksummed.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH] lro: ip fragment checking

[Brandeburg, Jesse]

Ben Hutchings wrote:
> On Mon, 2008-11-24 at 16:56 +0100, Jan-Bernd Themann wrote:
>> Currently there is no checking in the LRO receive path whether
>> TCP packets are ip fragmented. We should not consider
>> those packets for aggregation.
>> I'm not sure if this checking is actually required. Does anyone
>> know if it is possible to get fragmented TCP packets without
>> the tcp stack changing the MSS size?
>> This patch introduces explicit checking. Any objections?
> 
> LRO depends on the hardware performing TCP checksum offload, and the
> TCP checksum cannot be verified for IP fragments in isolation.  So I
> think drivers should not be passing fragments into inet_lro or should
> reject them in its get_frag_header() or get_skb_header() method. 
> Certainly sfc doesn't pass fragments into inet_lro because they have
> not been checksummed.

The fragments, definitely will not have checksums offloaded, but
what about the first packet in the chain?  I haven't verified in
ixgbe or igb whether or not it could try and aggregate a packet
with MF set if it was the first fragment in a series of IP fragments.