GNOME Bugzilla – Bug 721767
NetworkManager does not add route for dhcp server when needed
Last modified: 2014-01-24 12:38:23 UTC
I'm filing this as an upstream version of redhat bug 983325 When using a NetworkManager with a dhcp server, a routing entry is not added for the server if the server is not within the assigned network mask. In this case, a host routing entry for the dhcp_server_identifier should be created, so that if the interface is not the default route, the dhcp server can be contacted. To clarify, this bug will only be visible if the interface is NOT the default route. The bug will cause the address on the interface to eventually expire, and cause the connection to reset (killing all open connections).
Created attachment 265638 [details] [review] add route for dhcp4-server if needed I've created a patch to add a host routing entry for the dhcp4 server if it's not on the local subnet. I've tested this on my server, and it works as expected on my Fedora rpm build.
(In reply to comment #1) > Created an attachment (id=265638) [details] [review] > add route for dhcp4-server if needed What is the purpose of the first hunk? Also, could you please describe the actual problem, specifically the reason why the DHCP server/relay which is typically in the same subnet, should be unreachable at all. Preferebly in a comment, so it doesn't get lost. The magic of adding various routes is already more complicated than necessary, it would be nice to have all new additions properly documented in comments.
(In reply to comment #1) > Created an attachment (id=265638) [details] [review] > add route for dhcp4-server if needed > > I've created a patch to add a host routing entry for the dhcp4 server if it's > not on the local subnet. I've tested this on my server, and it works as > expected on my Fedora rpm build. Just a minor side note: you could create the patch with $ git format-patch -n1 which contains the author and your commit message. We can then apply it conveniently with `git am`. Anyway, thank you for your effort!!
Created attachment 265805 [details] [review] Updated patch with comment, requested patch format The first hunk is to save the gwaddr in the second code path to retrieve it. I've updated the code with a comment referring to the RFC and the rationale for the route (and also when it's needed). In my case it's a dhcp server with Comcast in the US, but I didn't think that needed to be specified in the comment -- although the ISP is large enough to affect many users. Let me know if you need any more updates :)
Referring to your original bug report https://bugzilla.redhat.com/show_bug.cgi?id=983325#c0 ... I think that is_router_reachable() in /usr/sbin/dhclient-script does something different: it ensures, that there is a device route for all the routers. You want to have a route to the DHCP server. In your original report, you got the DHCP offer from 96.147.132.1, but then you want to set the route for 69.252.97.6 (you explain that the former "is actually the relay agent"...). But in another setup without new_dhcp_server_identifier, wouldn't we have to ensure, that there is a route to the IP where the DHCP offer came from? Also, you only add the route, if the interface has a gateway. Why this restriction? Wouldn't we want to add a device route to the DHCP server in this case?
(In reply to comment #3) > Just a minor side note: you could create the patch with > $ git format-patch -n1 > which contains the author and your commit message. We can then apply it > conveniently with `git am`. Isn't 'git show' easier and better?
(In reply to comment #5) > Referring to your original bug report > https://bugzilla.redhat.com/show_bug.cgi?id=983325#c0 ... > > I think that is_router_reachable() in /usr/sbin/dhclient-script does something > different: it ensures, that there is a device route for all the routers. You > want to have a route to the DHCP server. > > In your original report, you got the DHCP offer from 96.147.132.1, but then you > want to set the route for 69.252.97.6 (you explain that the former "is actually > the relay agent"...). Aha, I missed that. You should AFAIK never talk to the DHCP server directly and should always use the relay agent. Please check and CLOSE/WONTFIX.
(In reply to comment #6) > Isn't 'git show' easier and better? AFAIK the output of git-show cannot be applied as conveniently as doing git-format-patch + git-am -- in the latter case, you already have the commit ready, not just the patch applied. (In reply to comment #7) > Aha, I missed that. You should AFAIK never talk to the DHCP server directly and > should always use the relay agent. Please check and CLOSE/WONTFIX. In the original bug report, the relay-agent is also not on the same network. So, maybe, we need to add the route for the IP that sent the DHCPOFFER?
(In reply to comment #5) > I think that is_router_reachable() in /usr/sbin/dhclient-script does something > different: it ensures, that there is a device route for all the routers. You > want to have a route to the DHCP server. My comment there was noting how is_router_reachable is specifically checking if the router address is on a different subnet than the assigned one, and adding a route for it before pinging it in that case... > In your original report, you got the DHCP offer from 96.147.132.1, but then you > want to set the route for 69.252.97.6 (you explain that the former "is actually > the relay agent"...). But in another setup without new_dhcp_server_identifier, > wouldn't we have to ensure, that there is a route to the IP where the DHCP > offer came from? When server_identifier is present, dhclient unicasts directly to it; if none is supplied, then it broadcasts on the device (no route needed). I looked at the isc dhclient.c code (see state_bound) as I couldn't test this directly... my dhcpd always sends a server_identifier option, and I can't find how to suppress it (just change it :) > Also, you only add the route, if the interface has a gateway. Why this > restriction? Wouldn't we want to add a device route to the DHCP server in this > case? I suppose a device route could be used as a fallback... I'm just not sure how to create one using the NMPlatformIP4Route (would setting the gateway to zero do it?).
(In reply to comment #7) > Aha, I missed that. You should AFAIK never talk to the DHCP server directly and > should always use the relay agent. Please check and CLOSE/WONTFIX. As I referenced in the patch comment, rfc2132 says dhclient should use the server_identifier to unicast to the dhcp server when it's supplied, so dhclient is expected to talk to it directly (and it tries to :)
(In reply to comment #9) > I suppose a device route could be used as a fallback... I'm just not sure how > to create one using the NMPlatformIP4Route (would setting the gateway to zero > do it?). Yes, a device route has 0.0.0.0 as gateway. So, I think, you could just omit the check "if (gwaddr) {" ...
Created attachment 265843 [details] [review] Updated patch to add device route if no gateway. Also added a info log if the route is added... so it's clear where it's going to.
Created attachment 265845 [details] [review] style !fixup for previous patch (In reply to comment #12) I would suggest this !fixup for style/logging, but overall I think it's good. IMO we should reference the full bz URLs in the commit message: https://bugzilla.gnome.org/show_bug.cgi?id=721767 https://bugzilla.redhat.com/show_bug.cgi?id=983325
Style updates look fine. If it's ok to put links in the commit messages, it sounds like a good idea. Thanks for all the feedback... still getting my sea legs on the "gnome way" of things :)
BTW, should I submit an updated patch combining the changes and updating the commit message, or is the plan to submit both patches together?
(In reply to comment #15) > BTW, should I submit an updated patch combining the changes and updating the > commit message, or is the plan to submit both patches together? yes, if you agree, just take !fixup from comment 13, update the commit message and repost the patch :) Thank you!!
Created attachment 265882 [details] [review] Patch including fixups and updated commit message Updated patch. Thanks again for all the assistance!
ACK, looks good to me!
Pushed to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=31fe84e467732463eabc8f70c2a419008e7a227c (After adjusting commit message and fixed whitespace) Thanks!! Closing bug now.
For reference... 9.7. Server Identifier This option is used in DHCPOFFER and DHCPREQUEST messages, and may optionally be included in the DHCPACK and DHCPNAK messages. DHCP servers include this option in the DHCPOFFER in order to allow the client to distinguish between lease offers. DHCP clients use the contents of the 'server identifier' field as the destination address for any DHCP messages unicast to the DHCP server. DHCP clients also indicate which of several lease offers is being accepted by including this option in a DHCPREQUEST message. The identifier is the IP address of the selected server. The code for this option is 54, and its length is 4. This RFC unfortunately doesn't specify the behavior with regard to DHCP relay agent, though.