Messages in this thread | | | Date | Tue, 02 Dec 2008 10:36:22 -0500 | From | Andrew Gallatin <> | Subject | Re: [PATCH] lro: IP fragment checking |
| |
Ben Hutchings wrote: > On Tue, 2008-12-02 at 09:42 -0500, Andrew Gallatin wrote: >> Ben Hutchings wrote: >>> On Mon, 2008-12-01 at 19:02 -0500, Andrew Gallatin wrote: >>>> Ben Hutchings wrote: > [...] >>>>> 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.. > > 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?
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
| |