diff mbox

[RFC] lro: ip fragment checking

Message ID 492ACEC4.3020702@de.ibm.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Jan-Bernd Themann Nov. 24, 2008, 3:56 p.m. UTC 
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(-)

Comments

Ben Hutchings Nov. 24, 2008, 4:51 p.m. UTC | #1 
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.
Jesse Brandeburg Nov. 24, 2008, 9:04 p.m. UTC | #2 
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.--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;