Page MenuHomeFreeBSD

Fix broken IPv6 scope ID checks in outgoing direction
ClosedPublic

Authored by hselasky on Jan 7 2019, 12:33 PM.

Details

Summary

Fix loopback traffic on link local IPv6 addresses.

The problem is the loopback interface can only receive packets with a single scope ID, namely the scope ID of the loopback interface. To mitigate this packets which use the scope ID are appearing as received by the real network interface, see "origifp" in the patch. The current code would drop packets which are designated for loopback which use a link-local scope ID in the destination address or source address, because they won't match the lo0's scope ID. To fix this restore the network interface pointer from the scope ID in the destination address for the problematic cases. See comments added in patch for a more detailed description.

MFC after: 1 week
Sponsored by: Mellanox Technologies

Test Plan

Run iperf on IPv6 link local addresses and for VLAN ones too.

iperf -s -V
iperf -V -c xxxxx%iface

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hselasky created this revision.Jan 7 2019, 12:33 PM
bz added a subscriber: bz.Jan 7 2019, 1:53 PM

Can you please fold some of the problem description of "why?" this change is needed in the proposed commit message; having some more information available when scrolling through source code management system logs is extremely helpful.

sys/netinet6/ip6_output.c
575 ↗(On Diff #52622)

Comments start upper case and end in punctuation; this can also be folded into a single line now?

590 ↗(On Diff #52622)

Check ... ID.

638 ↗(On Diff #52622)

So this is an empty block? I am also slightly confused by the "code above/below".
I think doing some of the "common code" upfront with error handing such as sa6_recoverscope() would make the if() blocks easier to read; you could then negate this block and make it the "else" case? Might also help putting comments in better places?

hselasky added a reviewer: bz.Jan 7 2019, 2:07 PM
hselasky marked 3 inline comments as done.Jan 7 2019, 2:26 PM
hselasky added inline comments.
sys/netinet6/ip6_output.c
638 ↗(On Diff #52622)

I'll invert the block.

hselasky marked an inline comment as done.Jan 7 2019, 2:34 PM
hselasky edited the summary of this revision. (Show Details)
hselasky updated this revision to Diff 52626.

Address comments by bz@ .

bz accepted this revision.Jan 7 2019, 3:52 PM

I like this version better than the one before. Thanks! For as much as I can say it looks OK. I haven't tested it.

This revision is now accepted and ready to land.Jan 7 2019, 3:52 PM
ae added a comment.Jan 7 2019, 6:14 PM

The current code would drop packets which are designated for loopback which use a link-local scope ID in the destination address or source address, because they won't match the lo0's scope ID.

So, why you think that this is wrong and shouldn't happen?

Would it be possible to include firewalls in the test plan?
For example, ensure that ipfw still accepts the packet using something like 'allow ip6 from any to any via lo0' rule.

In D18769#400616, @ae wrote:

The current code would drop packets which are designated for loopback which use a link-local scope ID in the destination address or source address, because they won't match the lo0's scope ID.

So, why you think that this is wrong and shouldn't happen?

Because then TCP loopback on non-lo0 link-local addresses won't work.

Would it be possible to include firewalls in the test plan?
For example, ensure that ipfw still accepts the packet using something like 'allow ip6 from any to any via lo0' rule.

This patch doesn't affect ::1 traffic.

Can you suggest kernel options of IPFW and a test script?

ae added a comment.Jan 9 2019, 11:03 AM

So, why you think that this is wrong and shouldn't happen?

Because then TCP loopback on non-lo0 link-local addresses won't work.

Can you show an example? I just tested and it works:

% nc -6 -l fe80::e6a7:a0ff:fe8e:16bf%lagg0 1234
TEST
% nc -6 fe80::e6a7:a0ff:fe8e:16bf%lagg0 1234
TEST
In D18769#400969, @ae wrote:

So, why you think that this is wrong and shouldn't happen?

Because then TCP loopback on non-lo0 link-local addresses won't work.

Can you show an example? I just tested and it works:

% nc -6 -l fe80::e6a7:a0ff:fe8e:16bf%lagg0 1234
TEST
% nc -6 fe80::e6a7:a0ff:fe8e:16bf%lagg0 1234
TEST

You need to send traffic in both directions and more than one TCP packet.

Try this:

iperf -s -V
iperf -V -c fe80::e6a7:a0ff:fe8e:16bf%lagg0 -t 16 -i 1

ae added a comment.Jan 9 2019, 12:45 PM

Ah, yes, I remembered, this problem was introduced with route caching.

This revision was automatically updated to reflect the committed changes.