lro: IP fragment checking
Subsystems (2)
PATCH 1/1 lro: IP fragment checking
This patch prevents that ip fragmented TCP packets are considered vaild for aggregation
Regards,
Jan-Bernd
---
net/ipv4/inet_lro.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
net/ipv4/inet_lro.c +3 -0
@@ -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 ;
Signature · 1 line
1.5.5
David Miller
maintainerFrom: Jan-Bernd Themann <[email protected]>
Date: Mon, 01 Dec 2008 09:58:55 +0100
Quoted 2 lines
This patch prevents that ip fragmented TCP packets are considered vaild for aggregation
We discussed this last week.
The things feeding packets into the LRO machinery should make this check, and that could be a hardware LRO assist.
Quoted 2 lines · Jan-Bernd Themann
This patch prevents that ip fragmented TCP packets are considered vaild for aggregation
<...>
net/ipv4/inet_lro.c static int lro_tcp_ip_check(struct iphdr *iph, struct tcphdr *tcph, · line 69
Quoted 3 lines
net/ipv4/inet_lro.c +3
-0+if (iph->frag_off &IP_MF ) +return -1 ; +
I think there is an endian bug, and that you should also check IP_OFFSET. What about:
if (iph->frag_off & htons(IP_MF|IP_OFFSET))
As to whether or not to do it in the drivers/hardware or in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.
Drew
David Miller
maintainerFrom: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500
Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.
Then there is no point in the hardware doing the check, if we're going to check it anyways.
That's part of my point about why this check doesn't belong here.
Quoted 7 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.
Quoted 5 lines
Then there is no point in the hardware doing the check, if we're going to check it anyways.
That's part of my point about why this check doesn't belong here.
What hardware does an explicit check for fragmentation? In most cases, aren't we just relying on the hardware checksum to be wrong on fragmented packets? That works 99.999% of the time, but the TCP checksum is pretty weak, and it is possible to have a fragmented packet where the first fragment has the same checksum as the entire packet.
I'd rather have a fragmentation check at the LRO layer to remove any ambiguity. But if you still object, I'll at least have to submit a patch which adds an explicit check in myri10ge.
Drew
Quoted 8 lines · Andrew Gallatin
Quoted 7 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.
Quoted 7 lines
Quoted 5 lines
Then there is no point in the hardware doing the check, if we're going to check it anyways.
That's part of my point about why this check doesn't belong here.
What hardware does an explicit check for fragmentation?
Any that implements TCP/UDP checksumming properly.
Quoted 5 lines
In most cases, aren't we just relying on the hardware checksum to be wrong on fragmented packets? That works 99.999% of the time, but the TCP checksum is pretty weak, and it is possible to have a fragmented packet where the first fragment has the same checksum as the entire packet.
[...]
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
Ben.
Signature · 3 lines
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.
Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Andrew Gallatin
Quoted 6 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Then there is no point in the hardware doing the check, if we're going to check it anyways.
That's part of my point about why this check doesn't belong here.
What hardware does an explicit check for fragmentation?
Any that implements TCP/UDP checksumming properly.
How many do?
Quoted 10 lines
Quoted 5 lines
In most cases, aren't we just relying on the hardware checksum to be wrong on fragmented packets? That works 99.999% of the time, but the TCP checksum is pretty weak, and it is possible to have a fragmented packet where the first fragment has the same checksum as the entire packet.
[...]
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
We do partial checksums.
Drew
Quoted 12 lines · Andrew Gallatin
Quoted 11 lines · Ben Hutchings
Quoted 10 lines · Andrew Gallatin
Quoted 9 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.Then there is no point in the hardware doing the check, if we're going to check it anyways.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
That's part of my point about why this check doesn't belong here.
What hardware does an explicit check for fragmentation?
Any that implements TCP/UDP checksumming properly.
How many do?
Good question. ;-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
In most cases, aren't we just relying on the hardware checksum to be wrong on fragmented packets? That works 99.999% of the time, but the TCP checksum is pretty weak, and it is possible to have a fragmented packet where the first fragment has the same checksum as the entire packet.
Quoted 7 lines
Quoted 5 lines
[...]
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
We do partial checksums.
So you should check for IP fragmentation in your get_frag_header() along with all the other checks you've got to do.
Ben.
Signature · 3 lines
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.
Quoted 28 lines · Ben Hutchings
Quoted 9 lines · Andrew Gallatin
Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Andrew Gallatin
Quoted 6 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 12:50:15 -0500Quoted 3 lines
As to whether or not to do it in the
drivers/hardwareor in the LRO code, I favor doing it in the LRO code just so that it is not missed in some driver.Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
Then there is no point in the hardware doing the check, if we're going to check it anyways.
That's part of my point about why this check doesn't belong here.
What hardware does an explicit check for fragmentation?
Any that implements TCP/UDP checksumming properly.
How many do?
Good question. ;-)
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
In most cases, aren't we just relying on the hardware checksum to be wrong on fragmented packets? That works 99.999% of the time, but the TCP checksum is pretty weak, and it is possible to have a fragmented packet where the first fragment has the same checksum as the entire packet.
[...]
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
We do partial checksums.
So you should check for IP fragmentation in your
get_frag_header()along with all the other checks you've got to do.
Indeed, and that is the patch I intend to submit if the fragment check in inet_lro is rejected. I still think the check belongs in the inet lro code though, and I'm worried it is being rejected for the wrong reasons..
Drew
Quoted 3 lines · Andrew Gallatin
Quoted 2 lines · Ben Hutchings
Quoted 1 line · Andrew Gallatin
Ben Hutchings wrote:
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
We do partial checksums.
Quoted 7 lines
Quoted 2 lines
So you should check for IP fragmentation in your
get_frag_header()along with all the other checks you've got to do.Indeed, and that is the patch I intend to submit if the fragment check in
inet_lrois rejected. I still think the check belongs in the inet lro code though, and I'm worried it is being rejected for the wrong reasons..
There's a wide variety of capabilities of different hardware:
1. No checksum offload. Probably not worth using LRO.
2. Full-checksum generation. Driver passes packets to inet_lro; get_frag_header() or get_skb_header() parses packets to check that they are TCP/IPv4 and to validate the checksum. inet_lro does further checks.
3. L4 packet parsing and checksum validation. Driver passes TCP/IPv4 packets to inet_lro. inet_lro does further checks.
4. Hardware/firmware LRO. inet_lro not needed.
You seem to be proposing that a check that is only needed in case (2) should also be applied in case (3). Maybe it would make more sense to define a generic implementation of get_frag_header() for full-checksum devices, if that's possible?
Ben.
Signature · 3 lines
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.
Quoted 26 lines · Ben Hutchings
Quoted 3 lines · Andrew Gallatin
Quoted 2 lines · Ben Hutchings
Quoted 1 line · Andrew Gallatin
Ben Hutchings wrote:
[...]
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
If your hardware/firmware wrongly claims to be able to verify the TCP/UDP checksum for an IP fragment, it seems to me you should deal with that in your driver or fix the firmware.
We do partial checksums.
So you should check for IP fragmentation in your
get_frag_header()along with all the other checks you've got to do.Indeed, and that is the patch I intend to submit if the fragment check in
inet_lrois rejected. I still think the check belongs in the inet lro code though, and I'm worried it is being rejected for the wrong reasons..There's a wide variety of capabilities of different hardware:
1. No checksum offload. Probably not worth using LRO.
2. Full-checksum generation. Driver passes packets toinet_lro;get_frag_header()orget_skb_header()parses packets to check that they are TCP/IPv4 and to validate the checksum.inet_lrodoes further checks.
3. L4 packet parsing and checksum validation. Driver passes TCP/IPv4 packets toinet_lro.inet_lrodoes further checks.
4. Hardware/firmware LRO.inet_lronot needed.
Quoted 4 lines
You seem to be proposing that a check that is only needed in case (2) should also be applied in case (3). Maybe it would make more sense to define a generic implementation of
get_frag_header()for full-checksum devices, if that's possible?
Or maybe a generic lro_check_header() that can be called from everybody's get_frag_header()/get_skb_header(). I guess what bothers me is the division of checks between the get_*_header() routine and lro_tcp_ip_checks() and the inevitable code duplication in the get_*_header routines.
I still don't understand why an unneeded check for fragmentation in case (3) is any more objectionable than the existing tcp flags checks in lro_tcp_ip_check(), many of which are surely not needed in case (3) either.
Drew
David Miller
maintainerFrom: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 16:53:14 -0500
Quoted 1 line
What hardware does an explicit check for fragmentation?
If the packet is fragmented, the chip shouldn't even be looking at the ports to make LRO deferral decisions let alone pass it down as a LRO'able frame.
Quoted 8 lines · David Miller
From: Andrew Gallatin <[email protected]>
Date: Mon, 01 Dec 2008 16:53:14 -0500Quoted 1 line
What hardware does an explicit check for fragmentation?
If the packet is fragmented, the chip shouldn't even be looking at the ports to make LRO deferral decisions let alone pass it down as a LRO'able frame.
You seem to be assuming that all consumers of the Linux LRO interface have support for LRO in hardware. Many (most?) do not. The way software LRO works in these drivers is the driver just sends all packets through lro_receive_skb() when LRO is configured and enabled. The LRO layer then filters out things which are not eligible for LRO. See, for example, igb_receive_skb().
If you're arguing that the hardware should have checked for fragmentation, then many (most?) things in lro_tcp_ip_check() should be removed. They are all basic sanity checks for packets that would not be considered for LRO by a hardware implementation, but are required by a software implementation where all packets are through the LRO code.
Drew