dhcp: have DHCP library support multiple router entries in Router option (3) #11208

thom311 · 2018-12-19T09:13:07Z

extend the DHCP client to return a list of router options, not only the first one. All but the first router are still ignored by networkd, so there is little change in behaviour. The point is to extend the DHCP library.
don't drop localhost/null addresses from NTP/DNS options from the DHCP library. Move such filtering to the caller. This is related to filter out 127.x.x.x DNS servers when serving DHCP, and when requesting DHCP #4524 and commit d9ec2e6.
use inet_ntop() instead of inet_ntoa().

yuwata

Several coding style comments.

src/libsystemd-network/network-internal.c

src/basic/in-addr-util.c

src/network/networkd-link.c

thom311 · 2019-01-07T15:57:57Z

v3: renamed in4_addr_is_non_local() to in4_addr_is_suitable_for_dnsntp()

thom311 · 2019-01-15T08:57:49Z

@yuwata hi, is this blocked because of an upcoming 241 release or is anything else missing? (thanks!)

thom311 · 2019-02-15T08:42:32Z

v4: rebased to master (no changes)

poettering

Looks good. The size_t thing doesn#t really matter, that#s just my OCD speaking. I'd still like to see the rename of the new function take place though

poettering · 2019-02-15T11:50:02Z

src/network/networkd-link.c

@@ -994,16 +994,17 @@ static int link_push_uplink_dns_to_dhcp_server(Link *link, sd_dhcp_server *s) {

        if (link->network->dhcp_use_dns && link->dhcp_lease) {
                const struct in_addr *da = NULL;
-                int n;
+                int j, n;


i figure this should be size_t actually, after all this is about memory...

in this case, the for loop goes over j < n, and n is returned by sd_dhcp_lease_get_dns().

For better or worse, sd_dhcp_lease_get_dns() returns an integer to leave room for error codes.

Mixing types here like doesn't seem preferable to me, is it?

poettering · 2019-02-15T11:50:23Z

src/network/networkd-manager.c

-static int ordered_set_put_in4_addrv(OrderedSet *s, const struct in_addr *addresses, unsigned n) {
+static int ordered_set_put_in4_addrv(OrderedSet *s,
+                                     const struct in_addr *addresses,
+                                     unsigned n,


poettering · 2019-02-15T11:52:01Z

src/basic/in-addr-util.h

@@ -30,6 +30,8 @@ int in_addr_is_link_local(int family, const union in_addr_union *u);
 bool in4_addr_is_localhost(const struct in_addr *a);
 int in_addr_is_localhost(int family, const union in_addr_union *u);

+bool in4_addr_is_suitable_for_dnsntp(const struct in_addr *a);


this functions sounds awfully specific for in-addr-util.h in basic/. Maybe move to network-internal.h instead?

or alternatively, maybe call this in4_addr_is_non_local() and leave it in basic/? After all, that's what this actually checks. by renaming it it would become generic enough to stay in in-addr-util.h I figure...

I like in4_addr_is_non_local() the best... will change back.

thom311 · 2019-02-15T12:44:00Z

v5:

rebased on master
renamed in4_addr_is_suitable_for_dnsntp() to in4_addr_is_non_local()
changed argument type unsigned n to size_t for ordered_set_put_in4_addrv().

poettering · 2019-02-15T12:55:07Z

Stellar! Thanks!

poettering · 2019-02-18T12:26:49Z

hmm, urks, some other commit broke this. could you please rebase?

…e_in_addrs() deserialize_in_addrs() allocates the buffer before trying to parse the IP address. Since a parsing error is silently ignored, the returned size might be zero. In such a case we shouldn't return any buffer. Anyway, there was no leak, because there are only two callers like r = deserialize_in_addrs(&lease->dns, dns); which both keep the unused buffer and later release it. Note that deserialize_in_addrs() doesn't free the pointer before reassigning the new output. The caller must take care to to pass "ret" with an allocated buffer that would be leaked when returning the result.

The Router DHCP option may contain a list of one or more routers ([1]). Extend the API of sd_dhcp_lease to return a list instead of only the first. Note that networkd still only uses the first router (if present). Aside from extending the internal API of the DHCP client, there is almost no change in behavior. The only visible difference in behavior is that the "ROUTER" variable in the lease file is now a list of addresses. Note how RFC 2132 does not define certain IP addresses as invalid for the router option. Still, previously sd_dhcp_lease_get_router() would never return a "0.0.0.0" address. In fact, the previous API could not differenciate whether no router option was present, whether it was invalid, or whether its first router was "0.0.0.0". No longer let the DHCP client library impose additional restrictions that are not part of RFC. Instead, the caller should handle this. The patch does that, and networkd only consideres the first router entry if it is not "0.0.0.0". [1] https://tools.ietf.org/html/rfc2132#section-3.5

The DHCP client should not pre-filter addresses beyond what RFC requires. If a client's user (like networkd) wishes to skip/filter certain addresses, it's their responsibility. The point of this is that the DHCP library does not hide/abstract information that might be relevant for certain users. For example, NetworkManager exposes DHCP options in its API. When doing that, the options should be close to the actual lease. This is related to commit d9ec2e6 (dhcp4: filter bogus DNS/NTP server addresses silently).

inet_ntop() is not documented to be thread-safe, so it should not be used in the DHCP library. Arguably, glibc uses a thread local buffer, so indeed there is no problem with a suitable libc. Anyway, just avoid it.

thom311 · 2019-02-18T12:37:54Z

v6: rebased on master and resolved merge-conflict in src/network/networkd-dhcp4.c (link_set_dhcp_routes()).

poettering · 2019-02-18T12:38:58Z

excellent! thank you very much!

thom311 changed the title ~~dhcp: have DHCP library support multiple router options~~ dhcp: have DHCP library support multiple router entries in Router option (3) Dec 19, 2018

yuwata added network dhcp labels Dec 20, 2018

yuwata reviewed Dec 25, 2018

View reviewed changes

thom311 force-pushed the dhcp-router-option-list branch from b569031 to 8eae1f8 Compare December 27, 2018 20:03

thom311 force-pushed the dhcp-router-option-list branch 2 times, most recently from 43900f8 to 2b3561a Compare January 7, 2019 15:57

thom311 force-pushed the dhcp-router-option-list branch from 2b3561a to 5dd46a9 Compare January 25, 2019 10:34

yuwata added this to the v242 milestone Feb 6, 2019

thom311 force-pushed the dhcp-router-option-list branch from 5dd46a9 to e88ae9e Compare February 15, 2019 08:42

poettering requested changes Feb 15, 2019

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 15, 2019

thom311 force-pushed the dhcp-router-option-list branch from e88ae9e to c965639 Compare February 15, 2019 12:42

poettering 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 Feb 15, 2019

poettering added needs-rebase and removed 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 labels Feb 18, 2019

thom311 added 4 commits February 18, 2019 13:32

network: avoid inet_ntoa() in favor of inet_ntop()

189255d

inet_ntop() is not documented to be thread-safe, so it should not be used in the DHCP library. Arguably, glibc uses a thread local buffer, so indeed there is no problem with a suitable libc. Anyway, just avoid it.

thom311 force-pushed the dhcp-router-option-list branch from c965639 to 189255d Compare February 18, 2019 12:37

poettering 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 Feb 18, 2019

poettering removed the needs-rebase label Feb 18, 2019

poettering merged commit c014a33 into systemd:master Feb 18, 2019

yuwata mentioned this pull request Feb 19, 2019

network: do not log wrong error cause #11762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dhcp: have DHCP library support multiple router entries in Router option (3) #11208

dhcp: have DHCP library support multiple router entries in Router option (3) #11208

thom311 commented Dec 19, 2018

yuwata left a comment

thom311 commented Jan 7, 2019

thom311 commented Jan 15, 2019

thom311 commented Feb 15, 2019

poettering left a comment

poettering Feb 15, 2019

thom311 Feb 15, 2019

poettering Feb 15, 2019

thom311 Feb 15, 2019

poettering Feb 15, 2019

poettering Feb 15, 2019

thom311 Feb 15, 2019

thom311 commented Feb 15, 2019

poettering commented Feb 15, 2019

poettering commented Feb 18, 2019

thom311 commented Feb 18, 2019

poettering commented Feb 18, 2019

dhcp: have DHCP library support multiple router entries in Router option (3) #11208

dhcp: have DHCP library support multiple router entries in Router option (3) #11208

Conversation

thom311 commented Dec 19, 2018

yuwata left a comment

Choose a reason for hiding this comment

thom311 commented Jan 7, 2019

thom311 commented Jan 15, 2019

thom311 commented Feb 15, 2019

poettering left a comment

Choose a reason for hiding this comment

poettering Feb 15, 2019

Choose a reason for hiding this comment

thom311 Feb 15, 2019

Choose a reason for hiding this comment

poettering Feb 15, 2019

Choose a reason for hiding this comment

thom311 Feb 15, 2019

Choose a reason for hiding this comment

poettering Feb 15, 2019

Choose a reason for hiding this comment

poettering Feb 15, 2019

Choose a reason for hiding this comment

thom311 Feb 15, 2019

Choose a reason for hiding this comment

thom311 commented Feb 15, 2019

poettering commented Feb 15, 2019

poettering commented Feb 18, 2019

thom311 commented Feb 18, 2019

poettering commented Feb 18, 2019