Skip to content

networkd: don't enable dhcpv6 on l3 devices#17608

Merged
keszybz merged 1 commit intosystemd:masterfrom
Mic92:dhcpv6-fixes
Nov 23, 2020
Merged

networkd: don't enable dhcpv6 on l3 devices#17608
keszybz merged 1 commit intosystemd:masterfrom
Mic92:dhcpv6-fixes

Conversation

@Mic92
Copy link
Copy Markdown
Contributor

@Mic92 Mic92 commented Nov 14, 2020

Devices with multicast but without mac addresses i.e. tun devices
are not getting setuped correctly:

$ ip tuntap add mode tun dev tun0
$ ip addr show tun0
16: tun0: <NO-CARRIER,POINTOPOINT,MULTICAST,NOARP,UP> mtu 1500 qdisc fq_codel state DOWN group default qlen 500
link/none
$ cat /etc/systemd/network/tun0.network
[Match]
Name = tun0

[Network]
Address=192.168.1.1/32
$ ./systemd-networkd
tun0: DHCP6 CLIENT: Failed to set identifier: Invalid argument
tun0: Failed

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 14, 2020

I also tried to come up with a test for this. However I don't have a fedora VM right now to test it:

diff --git a/test/test-network/conf/25-tun.network b/test/test-network/conf/25-tun.network
new file mode 100644
index 0000000000..bbfba362b3
--- /dev/null
+++ b/test/test-network/conf/25-tun.network
@@ -0,0 +1,5 @@
+[Match]
+Name=tun99
+
+[Network]
+Address=fe80::1
diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py
index 1062f93e55..82d50f3f2f 100755
--- a/test/test-network/systemd-networkd-tests.py
+++ b/test/test-network/systemd-networkd-tests.py
@@ -1160,15 +1160,20 @@ def test_veth(self):
 
     def test_tun(self):
         copy_unit_to_networkd_unit_path('25-tun.netdev')
+        copy_unit_to_networkd_unit_path('25-tun.network')
         start_networkd()
 
-        self.wait_online(['tun99:off'], setup_state='unmanaged')
+        self.wait_online(['tun99:routable'], setup_state='managed')
 
         output = check_output('ip -d link show tun99')
         print(output)
         # Old ip command does not support IFF_ flags
         self.assertRegex(output, 'tun (type tun pi on vnet_hdr on multi_queue|addrgenmode) ')
 
+        output = check_output('ip -6 addr show tun99')
+        print(output)
+        self.assertRegex(output, 'inet6 fe80::1')
+
     def test_tap(self):
         copy_unit_to_networkd_unit_path('25-tap.netdev')
         start_networkd()

@Mic92 Mic92 changed the title networkd: don't enable ipv6 on l3 devices networkd: don't enable dhcpv6 on l3 devices Nov 14, 2020
Copy link
Copy Markdown
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think link_ipv6ll_enabled() should be updated for such devices, instead of adding an extra condition in dhcp6_configure().

@yuwata yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 16, 2020
@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 16, 2020

I am not sure link_ipv6ll_enabled is right point to decide this. IPv6 link local addresses only need multi-cast for duplicate address detection not necessary a l2 network. Tinc vpn for example supports multicast in layer3 mode.

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 16, 2020

Right now I consider issue a blocker for the next release because it will break VPN setups.

@yuwata yuwata added this to the v247 milestone Nov 16, 2020
@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 16, 2020

I am not sure link_ipv6ll_enabled is right point to decide this. IPv6 link local addresses only need multi-cast for duplicate address detection not necessary a l2 network. Tinc vpn for example supports multicast in layer3 mode.

Or, then link_dhcp6_enabled() (which is an alias of link_dhcp_enabled()) and link_ipv6_accept_ra_enabled() should be updated?

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 16, 2020

I think, we should also update link_ipv4ll_enabled(), as IPv4LL also requires MAC address.

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 18, 2020

I cannot use link_dhcp6_enabled either because there is link_ipv6_accept_ra_enabled which is true for my interface. So it would still break the configuration. I don't know why it also checks for link_ipv6_accept_ra_enabled?

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 18, 2020

I cannot use link_dhcp6_enabled either because there is link_ipv6_accept_ra_enabled which is true for my interface. So it would still break the configuration. I don't know why it also checks for link_ipv6_accept_ra_enabled?

As, RA may request to start dhcp6 client. So, please not only link_dhcp6_enabled() but also link_ipv6_accept_ra_enabled() should be updated.

BTW, I noticed that link_dhcp6_enabled() does not call link_ipv6ll_enabled(), But IPv6LL is required in link_acquire_ipv6_conf()... We may need whole logic/conditions of configuring/starting dhcp6 client and ndisc.

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 18, 2020

And, half of the list of interface kind in link_ipv6ll_enabled() are interfaces which do not have MAC address. Maybe, we should add link_has_mac_address() or something and use it at appropriate places.

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 18, 2020

Would this not break multi-cast network interfaces that support router advertisement but don't have MAC addresses? cc @andir

UPDATE ok. let's go this route, but longterm it would be good not to depend on having ethernet for dhcpv6/router advertisment.

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 18, 2020

Actually ndisc_configure can handle cases with no ipv6 addresses.

@Mic92
Copy link
Copy Markdown
Contributor Author

Mic92 commented Nov 19, 2020

@yuwata it now defaults to DUID_TYPE_EN for devices without mac addresses. If you like this solution I will also update the documentation accordingly.

@Mic92 Mic92 force-pushed the dhcpv6-fixes branch 2 times, most recently from 9ee237f to f80d043 Compare November 20, 2020 20:42
Copy link
Copy Markdown
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several minor coding issues.

Devices with multicast but without mac addresses i.e. tun devices
are not getting setuped correctly:

$ ip tuntap add mode tun dev tun0
$ ip addr show tun0
16: tun0: <NO-CARRIER,POINTOPOINT,MULTICAST,NOARP,UP> mtu 1500 qdisc fq_codel state DOWN group default qlen 500
    link/none
$ cat /etc/systemd/network/tun0.network
[Match]
Name = tun0

[Network]
Address=192.168.1.1/32
$ ./systemd-networkd
tun0: DHCP6 CLIENT: Failed to set identifier: Invalid argument
tun0: Failed
@yuwata
Copy link
Copy Markdown
Member

yuwata commented Nov 23, 2020

Thanks. LGTM.

@yuwata yuwata added 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 and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Nov 23, 2020
Copy link
Copy Markdown
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bionic-amd64 failed in TEST-50-DISSECT.
semaphoreci timed out.

I'll go ahead and merge this without waiting for the straggling tests, since this isn't tested in any way by our tests, so if some pass because compilation works, there isn't much point in waiting for the others.

return &link->network->duid;
else if (link->hw_addr.length == 0 &&
(link->manager->duid.type == DUID_TYPE_LLT ||
link->manager->duid.type == DUID_TYPE_LL))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IN_SET() would be nicer here, but it doesn't matter much.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #17692.

@keszybz keszybz merged commit 1d370b2 into systemd:master Nov 23, 2020
@Mic92 Mic92 deleted the dhcpv6-fixes branch November 23, 2020 14:06
@flokli flokli mentioned this pull request Nov 26, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 network

Development

Successfully merging this pull request may close these issues.

3 participants