network: Fix IPv6 PreferredSource routes #11375

daxtens · 2019-01-10T03:06:40Z

If an IPv6 route is added with a source address that is still
tentative, the kernel will refuse to install it.

Previously, once we sent the messages to the kernel to add the
addresses, we would immediately proceed to add the routes. The
addresses would usually still be tentative at this point, so
adding static IPv6 routes was broken - see issue #5882.

Now, only begin to configure routes once the addresses are ready,
by restructuring the state machine, and tracking when addresses are
ready, not just added.

Fixes: #5882

We're about to need it to be later in the file for the next commit. Moving it now means that when we change it in the next commit, it's not intermingled with the move. No functional change intended. Signed-off-by: Daniel Axtens <dja@axtens.net>

yuwata · 2019-01-13T17:50:58Z

Please provide a testcase for the issue in test/test-network/.

yuwata · 2019-01-13T17:51:07Z

cc @ssahani

ssahani · 2019-01-14T11:51:20Z

overall looks ok Please provide the test case so that it helps us track the issue.

daxtens · 2019-01-14T14:36:25Z

Hi,

Thanks for the feedback - I have added a new commit with a test case.

Regards,
Daniel

src/network/networkd-link.c

yuwata · 2019-01-15T02:54:00Z

I confirm that this actually fixes the provided test case, but unfortunately, breaks other route or routing policy related tests.

======================================================================
FAIL: test_ip_route (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 688, in test_ip_route
    self.assertRegex(output, 'static')
AssertionError: Regex didn't match: 'static' not found in '192.168.0.0/24 proto kernel scope link src 192.168.0.15'

======================================================================
FAIL: test_ip_route_blackhole_unreachable_prohibit (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 710, in test_ip_route_blackhole_unreachable_prohibit
    self.assertRegex(output, 'blackhole')
AssertionError: Regex didn't match: 'blackhole' not found in 'default via 192.168.43.1 dev wlp1s0 proto dhcp src 192.168.43.187 metric 1024 \n192.168.43.0/24 dev wlp1s0 proto kernel scope link src 192.168.43.187 \n192.168.43.1 dev wlp1s0 proto dhcp scope link src 192.168.43.187 metric 1024'

======================================================================
FAIL: test_ip_route_gateway (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 737, in test_ip_route_gateway
    self.assertRegex(output, 'default')
AssertionError: Regex didn't match: 'default' not found in ''

======================================================================
FAIL: test_ip_route_gateway_on_link (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 759, in test_ip_route_gateway_on_link
    self.assertRegex(output, 'default')
AssertionError: Regex didn't match: 'default' not found in ''

======================================================================
FAIL: test_ip_route_tcp_window (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 726, in test_ip_route_tcp_window
    self.assertRegex(output, 'initcwnd 20')
AssertionError: Regex didn't match: 'initcwnd 20' not found in 'default via 192.168.43.1 dev wlp1s0 proto dhcp src 192.168.43.187 metric 1024 \n192.168.43.0/24 dev wlp1s0 proto kernel scope link src 192.168.43.187 \n192.168.43.1 dev wlp1s0 proto dhcp scope link src 192.168.43.187 metric 1024'

======================================================================
FAIL: test_routing_policy_rule (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 612, in test_routing_policy_rule
    self.assertRegex(output, '111')
AssertionError: Regex didn't match: '111' not found in '0:\tfrom all lookup local \n1000:\tfrom all lookup [l3mdev-table] \n32766:\tfrom all lookup main \n32767:\tfrom all lookup default'

======================================================================
FAIL: test_routing_policy_rule_invert (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 646, in test_routing_policy_rule_invert
    self.assertRegex(output, '111')
AssertionError: Regex didn't match: '111' not found in '0:\tfrom all lookup local \n1000:\tfrom all lookup [l3mdev-table] \n32766:\tfrom all lookup main \n32767:\tfrom all lookup default'

======================================================================
FAIL: test_routing_policy_rule_port_range (__main__.NetworkdNetWorkTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./systemd-networkd-tests.py", line 629, in test_routing_policy_rule_port_range
    self.assertRegex(output, '111')
AssertionError: Regex didn't match: '111' not found in '0:\tfrom all lookup local \n1000:\tfrom all lookup [l3mdev-table] \n32766:\tfrom all lookup main \n32767:\tfrom all lookup default'

----------------------------------------------------------------------
Ran 67 tests in 815.683s

FAILED (failures=8, expected failures=1)

daxtens · 2019-01-15T03:11:22Z

OK, I will look in to that. I did see some failures in the test suite, but I thought they were localised to my machine because some of the tests were failing before the commit as well, and the CI tests passed. I will investigate further and revise the patches.

yuwata · 2019-01-15T03:54:06Z

If I understand correctly, when RA is enabled but no RA exists, then it takes NDISC_TIMEOUT_NO_RA_USEC = NDISC_ROUTER_SOLICITATION_INTERVAL * NDISC_MAX_ROUTER_SOLICITATIONS = 12sec to set ndisc_configured flag. But, in the test script, networkd only runs ~5sec. So, within the duration, link_check_ready() does not start to set route or routing policy.

Is it necessary to wait for RA to be configured?

yuwata · 2019-01-15T04:02:42Z

Before this PR, even if IPv6AcceptRA=no is not specified, route or routing policy are set soon. Of course, the link does not become 'configured' state in networkctl at that time. But, I guess almost all functionalities are actually configured. But this PR blocks 12 seconds...

yuwata · 2019-01-15T04:26:44Z

Yeah, by changing the below sleep time to 15sec, test_ip_route passes:

systemd/test/test-network/systemd-networkd-tests.py

Line 143 in 1c8e48f

time.sleep(5)

daxtens · 2019-01-15T13:51:37Z

Yes, I think your analysis is right. I think it might be possible to reorder the tests in link_check_ready() so as to get the routes installed earlier; I'll have a go at that and let you know how I go.

yuwata · 2019-01-15T21:17:28Z

@ssahani Do you have any comments?

If an IPv6 route is added with a source address that is still tentative, the kernel will refuse to install it. Previously, once we sent the messages to the kernel to add the addresses, we would immediately proceed to add the routes. The addresses would usually still be tentative at this point, so adding static IPv6 routes was broken - see issue systemd#5882. Now, only begin to configure routes once the addresses are ready, by restructuring the state machine, and tracking when addresses are ready, not just added. Fixes: systemd#5882 Signed-off-by: Daniel Axtens <dja@axtens.net>

The test is a bit messy because it must be done on a device that enforces a tentative state for IPv6 addresses, and it appears that the dummy device does not. So we use a bond instead. Signed-off-by: Daniel Axtens <dja@axtens.net>

daxtens · 2019-01-16T02:11:35Z

So I've re-ordered link_check_ready() so that the slow link-local addressing checks are done towards the end. As far as I can see, nothing that is configured earlier can sensibly depend on the link local addresses, and the tests seem to pass within a reasonable time period now.

yuwata

LGTM.

yuwata added the network label Jan 10, 2019

yuwata requested changes Jan 14, 2019

View reviewed changes

src/network/networkd-link.c Outdated Show resolved Hide resolved

daxtens force-pushed the issue5882 branch from 345e2de to d06b51a Compare January 14, 2019 23:53

daxtens added 2 commits January 16, 2019 12:54

tests: Add test for IPv6 source routing

20ca06a

The test is a bit messy because it must be done on a device that enforces a tentative state for IPv6 addresses, and it appears that the dummy device does not. So we use a bond instead. Signed-off-by: Daniel Axtens <dja@axtens.net>

daxtens force-pushed the issue5882 branch from d06b51a to 20ca06a Compare January 16, 2019 01:59

yuwata approved these changes Jan 16, 2019

View reviewed changes

yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 16, 2019

yuwata changed the title ~~#5882: Fix IPv6 PreferredSource routes~~ network: Fix IPv6 PreferredSource routes Jan 16, 2019

poettering merged commit a8ea283 into systemd:master Jan 17, 2019

Uh oh!

network: Fix IPv6 PreferredSource routes #11375

network: Fix IPv6 PreferredSource routes #11375

Uh oh!

Conversation

daxtens commented Jan 10, 2019

Uh oh!

yuwata commented Jan 13, 2019

Uh oh!

yuwata commented Jan 13, 2019

Uh oh!

ssahani commented Jan 14, 2019

Uh oh!

daxtens commented Jan 14, 2019

Uh oh!

Uh oh!

yuwata commented Jan 15, 2019

Uh oh!

daxtens commented Jan 15, 2019

Uh oh!

yuwata commented Jan 15, 2019

Uh oh!

yuwata commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuwata commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxtens commented Jan 15, 2019

Uh oh!

yuwata commented Jan 15, 2019

Uh oh!

daxtens commented Jan 16, 2019

Uh oh!

yuwata left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

yuwata commented Jan 15, 2019 •

edited

Loading

yuwata commented Jan 15, 2019 •

edited

Loading