GNOME Bugzilla – Bug 773571
dhcp fail because of duplicate DHCP request options are generated in dhclient config file
Last modified: 2016-11-08 13:03:27 UTC
Created attachment 338581 [details] DHCP DISCOVER by 'dhclient eth0' Linux: Ubuntu 14.04 to 16.04 Dhcp server: Juniper SRX with static route(option 121) in configuration file. We moved to new office and using a new Juniper SRX build-in dhcp server to replace old linux dhcp server. Juniper SRX are configured to push static route(rfc3442-classless-static-routes, option 121) to dhcpclient when send dhcp ACK. All Ubuntu linux can not obtain IP via dhcp using network-manager. Try issuing command line 'dhclient eth0' in terminal, the client can obtain IP from Juniper SRX.
Created attachment 338582 [details] DHCP DISCOVER by network-manager
Normal DHCP process with dhclient client Juniper RRX dhcp server DHCP DISCOVER ------> <-------- DHCP Offer DHCP REQUEST --------> <-------- DHCP ACK network-manager client Juniper RRX dhcp server DHCP DISCOVER ------> DHCP DISCOVER(retry) ---> DHCP DISCOVER(retry) ---> No offer DHCP REQUEST --------> <-------- DHCP NACK
See attached picture 'DHCP DISCOVER by network-manager' and 'DHCP DISCOVER by dhclient eth0', We found the DHCP DISCOVER requests send by network-manager contains duplicated options: 121, 42 The duplicated option 121 cause Juniper SRX does not replay DHCP offer and ACK. /etc/dhcp/dhclient.conf in ubuntu already contains request options 121 as bellow: option rfc3442-classless-static-routes code 121 = array of unsigned integer 8; request subnet-mask, broadcast-address, time-offset, routers, domain-name, domain-name-servers, domain-search, host-name, dhcp6.name-servers, dhcp6.domain-search, netbios-name-servers, netbios-scope, interface-mtu, rfc3442-classless-static-routes, ntp-servers, dhcp6.fqdn, dhcp6.sntp-servers; But network-manager add 'also request rfc3442-classless-static-routes' to generated dhclient config file unconditionally, this cause dhclient send duplicates DHCP options. I don't know if DHCP request with duplicated options are legal, but network-manager add 'also request' DHCP options without check options already exists seems a bit rude. This but can be fixed by 3 way: 1st. move rfc3442-classless-static-routes in /etc/dhcp/dhclient.conf from 'request' to 'also request'. NetworkManager doesn't generate duplicated 'also request' options. (This should be fixed by Ubuntu) 2nd. Filter out duplication options defined in config file by dhclient itself. 3rd. Do not generated 'also request' options which already exists in 'request'. I made a patch in the 3rd way.
Created attachment 338585 [details] [review] patch
The patch looks good to me, thanks!. Can you add the missing space after function name where it's missing in few places? +static gboolean +grab_request_optioins(GPtrArray *store, const char* line) And also fix the spelling of 'options'. Unless I'm reading the code wrong, I think there is a pre-existing problem in the way the "request" statement is parsed, as we should clear the option list every time instead of appending to it; but this can also be fixed in a following patch. The handling of "also request" instead seems correct.
Created attachment 338608 [details] [review] patch fixed typo and coding style. Upload new patch. fixed typo and coding style.
(In reply to trulyliu from comment #6) > Created attachment 338608 [details] [review] [review] > patch fixed typo and coding style. > > Upload new patch. fixed typo and coding style. Thanks. I've pushed the patch and another additional commit to branch bg/dhclient-dup-reqlist-bgo773571
grab_request_options() will skip over an element "-;" of "&foo;bar" anyway, the code has many problems with parsing unexpected data. E.g. send dhcp-client-identifier; will not be recognized, but send dhcp-client-identifierbogus; will be accepted wrongly. That should be fixed. But I digress, it's not a new problem of this patch. ACK to the patch.
Both the patch and commit in bg/dhclient-dup-reqlist-bgo773571 looks fine to me. Just a minor warn: in case of out of order "also request" and "request" (also request section before request one) or multiple "request" sections seems to me that we will discard the "also request" options (or the options in all the "request" section but the last). But I assume this is a correct behavior.
(In reply to Francesco Giudici from comment #9) > Both the patch and commit in bg/dhclient-dup-reqlist-bgo773571 looks fine to > me. > > Just a minor warn: in case of out of order "also request" and "request" > (also request section before request one) or multiple "request" sections > seems to me that we will discard the "also request" options (or the options > in all the "request" section but the last). But I assume this is a correct > behavior. Yeah, that is the intended behavior. Merged the branch to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bbc28a2f97967a75b2e9ae17fd0f36170410738a https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2049e97d9e4325b3e7416b1552df837000276d6e Thanks for the contribution!