Page MenuHomeFreeBSD

netinet6: In tcp_lro_rx_ipv6(), check for IPv6 flow ID.
Needs ReviewPublic

Authored by nc on May 31 2020, 7:58 PM.

Details

Reviewers
bz
rrs
Group Reviewers
transport
Summary

netinet6: In tcp_lro_rx_ipv6(), check for IPv6 flow ID.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nc requested review of this revision.May 31 2020, 7:58 PM

In RFC 6437 there is no rule, that flow labels are immutable for a given TCP session. It only notes in section 6.1, that changing the flow label within a TCP stream might be suspicious.
IPv6 flow labels are designed as QoS/routing indicators (like the DCSP field), which (in theory) might vary during the life time of the TCP session.

Given, that LRO handling is on the recipient end of the session -- there are no routing, forwarding or QoS behind this point -- the flow label is irrelevant by definition.
In my eyes, throwing away packets which successfully reached the destination for potential QoS/routing issues, is unexpected.

If I'm wrong, the change can be passed.
If I'm right, please change the comment itself to reflect the reasoning.

This revision is now accepted and ready to land.Aug 25 2020, 1:32 PM
In D25085#560681, @lutz_donnerhacke.de wrote:

In my eyes, throwing away packets which successfully reached the destination for potential QoS/routing issues, is unexpected.

We don't throw away the packet. We just refuse to merge it with the packet in the LRO buffer. This causes us to send both packets up the stack separately, so that the flow label is properly preserved.

Thanks for approving this.

Could one of you please commit this?

bz requested changes to this revision.Aug 25 2020, 3:25 PM
bz added inline comments.
sys/netinet/tcp_lro.c
385–388

Aehm veto. The mbuf packet header flowid is the 4-tuple and has nothing to do with the IPv6 flow label, or am I wrong?

This revision now requires changes to proceed.Aug 25 2020, 3:25 PM

Good catch.

Would this updated patch work?

nc marked an inline comment as done.Aug 25 2020, 3:41 PM
sys/netinet/tcp_lro.c
385–388

ip6 is derived from m in tcp_lro_rx2() so you are comparing the identical memory here.

I think what needs to be done is to store the ip6_flow or see if we have the entire ip6 header stored in lc already. The code has changed quite a bit since I wrote these comments. lc->leip->ip6 might have that but we'd need to make sure it actually does and then comapre to ip6->ip6_flow or if the former is NULL (I'd assume) pass?

Thanks for the clarification.

Is this good?

nc marked an inline comment as done.Aug 25 2020, 4:27 PM
bz added inline comments.
sys/netinet/tcp_lro.c
385

I assume le is != NULL here given on return of this function we unconditionally write to it; the question is probably more le->le_ip6 != NULL.

At this point someone who knows the current code should then have a look over this again as I honestly have no idea about the code flow anymore. @rrs is probably the best candidate; @gallatin or @np are probably also people who might be able to review/test this.