Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1519299 - Setting ipv4.method to "shared" forgets ipv4.dhcp-hostname
Summary: Setting ipv4.method to "shared" forgets ipv4.dhcp-hostname
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.5
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Francesco Giudici
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-11-30 14:57 UTC by Marius Vollmer
Modified: 2018-10-30 11:12 UTC (History)
9 users (show)

Fixed In Version: NetworkManager-1.12.0-0.1.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-10-30 11:11:02 UTC


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2018:3207 None None None 2018-10-30 11:12:20 UTC

Description Marius Vollmer 2017-11-30 14:57:29 UTC
Description of problem:

Setting ipv4.method of a connection to "shared" forgets a previously set ipv4.dhcp-hostname value.  Also, the ifcfg file isn't updated consistently.

Version-Release number of selected component (if applicable):
NetworkManager-1.10.0-1.el7.x86_64

How reproducible:
Always

Steps to Reproduce:

# nmcli con add type ethernet ifname foo0 connection.id CON
Connection 'CON' (a0be1b91-1396-4347-89f3-056aadc160a8) successfully added.
# nmcli -f ipv4.dhcp-hostname con show CON
ipv4.dhcp-hostname:                     --

# nmcli con mod CON ipv4.dhcp-hostname foo
# nmcli -f ipv4.dhcp-hostname con show CON
ipv4.dhcp-hostname:                     foo
# grep DHCP /etc/sysconfig/network-scripts/ifcfg-CON 
DHCP_HOSTNAME=foo

[ dhcp-hostname is set correctly now ]

# nmcli con mod CON ipv4.method shared
# nmcli -f ipv4.dhcp-hostname con show CON
ipv4.dhcp-hostname:                     --
# grep DHCP /etc/sysconfig/network-scripts/ifcfg-CON 
DHCP_HOSTNAME=foo

[ dhcp-hostname is reset inside NM but still in the ifcfg file ]

# nmcli con mod CON ipv4.method shared
# nmcli -f ipv4.dhcp-hostname con show CON
ipv4.dhcp-hostname:                     --
[root@localhost ~]# grep DHCP /etc/sysconfig/network-scripts/ifcfg-CON 
<empty>

[ now dhcp-hostname is gone from the ifcfg file as well. ]

Expected results:

The dhcp-hostname setting should stick around, as it used to.  If it is removed, the ifcfg file should be kept consistent.

Additional info:

This used to work with NetworkManager-1.10.0-0.1.git20171024.b16c853b.el7.x86_64

Comment 2 Francesco Giudici 2018-01-22 13:15:17 UTC
The describer behavior happens to all the ipv4.dhcp-* properties and some others (always ipv4 related) when method is changed to "shared":
ipv4.dhcp-client-id
ipv4.dhcp-timeout
ipv4.dhcp-send-hostname
ipv4.dhcp-hostname
ipv4.dhcp-fqdn
ipv4.ignore-auto-routes
ipv4.ignore-auto-dns
ipv4.may-fail
...

When a connection is read from a ifcfg file, if the method is "shared" the read is stopped before reading the affected properties.
This was not spotted before because a recent change makes each write being followed by a read back, updating the connection from the last read.
Before you would get the same effect only restarting NetworkManager or when reloading the connections with:
# nmcli connection reload

Pushed a patch on branch: fg/dhcp-hostname_persist-rh1519299

This will allow to read all the properties when ipv4.method == shared (so that both in memory and on-disk state will be preserved).
Please review.

Comment 3 Thomas Haller 2018-01-22 14:13:44 UTC
if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {

nm_streq()



+              g_free (gateway);
+              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
+                   return NULL

This may crash due to dangling pointer. Don't free gateway yet here.



Could you move the entire 
  if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
check outside the loop? There is no need to read the GATEWAY multiple times, and it's not immediately obvious, that the loop is guaranteed to run at least once (though actually it always will).

Comment 4 Francesco Giudici 2018-01-22 17:24:41 UTC
(In reply to Thomas Haller from comment #3)
> if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> 
> nm_streq()
> 
> 
> 
> +              g_free (gateway);
> +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> +                   return NULL
> 
> This may crash due to dangling pointer. Don't free gateway yet here.

Cannot spot it, probably I'm missing something... gateway is initialized at NULL at declaration; then only:
   if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
may fill it with a g_strdup().
At this point g_free() will be safe...

> 
> 
> 
> Could you move the entire 
>   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> check outside the loop? There is no need to read the GATEWAY multiple times,
> and it's not immediately obvious, that the loop is guaranteed to run at
> least once (though actually it always will).

Well, I forgot to put a "break;" there:
+               if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
+                       g_free (gateway);
+                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
+                               return NULL;
+                       gateway = g_strdup (nm_utils_inet4_ntop (a, inet_buf));
--->                    break;
+               }

This was the main idea, as the original code checked for "GATEWAY" only if an IP_ADDRESS was found.. using the loop, we would do that only if "addr" had value.


Anyway...
I realized that we may not want this at all. I would just remove that snippet, as the GATEWAY should be read by read_full_ip4_address():
       if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))

The only different behavior we could have is when in method == SHARED, we have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this case we will miss the GATEWAY.
Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we missed it before).

If we want to strictly follow behavior as it used to be for the above cases, I would be for moving the check of method == SHARED outside the loop, as suggested. But as I think we don't want, proposing the updated branch:
fg/dhcp-hostname_persist-rh1519299

Comment 5 Thomas Haller 2018-01-22 18:25:48 UTC
(In reply to Francesco Giudici from comment #4)
> (In reply to Thomas Haller from comment #3)
> > if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > 
> > nm_streq()
> > 
> > 
> > 
> > +              g_free (gateway);
> > +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> > +                   return NULL
> > 
> > This may crash due to dangling pointer. Don't free gateway yet here.
> 
> Cannot spot it, probably I'm missing something... gateway is initialized at
> NULL at declaration; then only:
>    if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> may fill it with a g_strdup().
> At this point g_free() will be safe...

during "return NULL", gateway will be freed, because it's declared with gs_free. Cannot leave a dangling pointer in this case, and dangling pointers are ugly in general.


> > Could you move the entire 
> >   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > check outside the loop? There is no need to read the GATEWAY multiple times,
> > and it's not immediately obvious, that the loop is guaranteed to run at
> > least once (though actually it always will).
> 
> Well, I forgot to put a "break;" there:
> +               if (!g_ascii_strcasecmp (method,
> NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> +                       g_free (gateway);
> +                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a,
> error))
> +                               return NULL;
> +                       gateway = g_strdup (nm_utils_inet4_ntop (a,
> inet_buf));
> --->                    break;
> +               }
> 
> This was the main idea, as the original code checked for "GATEWAY" only if
> an IP_ADDRESS was found.. using the loop, we would do that only if "addr"
> had value.
> 
> 
> Anyway...
> I realized that we may not want this at all. I would just remove that
> snippet, as the GATEWAY should be read by read_full_ip4_address():
>        if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> 
> The only different behavior we could have is when in method == SHARED, we
> have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this
> case we will miss the GATEWAY.
> Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we
> missed it before).
> 
> If we want to strictly follow behavior as it used to be for the above cases,
> I would be for moving the check of method == SHARED outside the loop, as
> suggested. But as I think we don't want, proposing the updated branch:
> fg/dhcp-hostname_persist-rh1519299

if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0)

Please let's use nm_streq() or NM_IN_STRSET()



Anyway, NMSettingIP4Config's verify() has code like 


»···} else if (   !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL)
»···           || !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)
    ....
»···»···if (nm_setting_ip_config_get_num_dns (s_ip) > 0) {
»···»···»···g_set_error (error,
»···»···»···             NM_CONNECTION_ERROR,
»···»···»···             NM_CONNECTION_ERROR_INVALID_PROPERTY,
»···»···»···             _("this property is not allowed for '%s=%s'"),
»···»···»···             NM_SETTING_IP_CONFIG_METHOD, method);
»···»···»···g_prefix_error (error, "%s.%s: ", NM_SETTING_IP4_CONFIG_SETTING_NAME, NM_SETTING_IP_CONFIG_DNS);
»···»···»···return FALSE;
»···»···}


that means, a shared connection with DNS servers is rejected as invalid. Hence, if a user has an ifcfg file on disc with method=shared *and* DNS servers, previously the DNS servers would have been silently ignored, now it is rejected with error.
That might be fine though, since writer always only writes connections that verify(). Hence, writer supposedly never wrote such a connection (which would need investigation and confirmation). So, at worst, the user edited such files by hand and they will stop working because we now declare them as invalid.
That's still kinda ugly. Can we avoid rejecting files that we accepted previously?

Comment 6 Francesco Giudici 2018-01-23 09:23:08 UTC
(In reply to Thomas Haller from comment #5)
> (In reply to Francesco Giudici from comment #4)
> > (In reply to Thomas Haller from comment #3)
> > > if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > > 
> > > nm_streq()
> > > 
> > > 
> > > 
> > > +              g_free (gateway);
> > > +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> > > +                   return NULL
> > > 
> > > This may crash due to dangling pointer. Don't free gateway yet here.
> > 
> > Cannot spot it, probably I'm missing something... gateway is initialized at
> > NULL at declaration; then only:
> >    if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> > may fill it with a g_strdup().
> > At this point g_free() will be safe...
> 
> during "return NULL", gateway will be freed, because it's declared with
> gs_free. Cannot leave a dangling pointer in this case, and dangling pointers
> are ugly in general.

Oh, sure... I missed that gateway is declared gs_free... thanks.

> 
> 
> > > Could you move the entire 
> > >   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > > check outside the loop? There is no need to read the GATEWAY multiple times,
> > > and it's not immediately obvious, that the loop is guaranteed to run at
> > > least once (though actually it always will).
> > 
> > Well, I forgot to put a "break;" there:
> > +               if (!g_ascii_strcasecmp (method,
> > NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > +                       g_free (gateway);
> > +                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a,
> > error))
> > +                               return NULL;
> > +                       gateway = g_strdup (nm_utils_inet4_ntop (a,
> > inet_buf));
> > --->                    break;
> > +               }
> > 
> > This was the main idea, as the original code checked for "GATEWAY" only if
> > an IP_ADDRESS was found.. using the loop, we would do that only if "addr"
> > had value.
> > 
> > 
> > Anyway...
> > I realized that we may not want this at all. I would just remove that
> > snippet, as the GATEWAY should be read by read_full_ip4_address():
> >        if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> > 
> > The only different behavior we could have is when in method == SHARED, we
> > have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this
> > case we will miss the GATEWAY.
> > Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we
> > missed it before).
> > 
> > If we want to strictly follow behavior as it used to be for the above cases,
> > I would be for moving the check of method == SHARED outside the loop, as
> > suggested. But as I think we don't want, proposing the updated branch:
> > fg/dhcp-hostname_persist-rh1519299
> 
> if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0)
> 
> Please let's use nm_streq() or NM_IN_STRSET()
> 
> 
> 
> Anyway, NMSettingIP4Config's verify() has code like 
> 
> 
> »···} else if (   !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL)
> »···           || !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)
>     ....
> »···»···if (nm_setting_ip_config_get_num_dns (s_ip) > 0) {
> »···»···»···g_set_error (error,
> »···»···»···             NM_CONNECTION_ERROR,
> »···»···»···             NM_CONNECTION_ERROR_INVALID_PROPERTY,
> »···»···»···             _("this property is not allowed for '%s=%s'"),
> »···»···»···             NM_SETTING_IP_CONFIG_METHOD, method);
> »···»···»···g_prefix_error (error, "%s.%s: ",
> NM_SETTING_IP4_CONFIG_SETTING_NAME, NM_SETTING_IP_CONFIG_DNS);
> »···»···»···return FALSE;
> »···»···}
> 
> 
> that means, a shared connection with DNS servers is rejected as invalid.
> Hence, if a user has an ifcfg file on disc with method=shared *and* DNS
> servers, previously the DNS servers would have been silently ignored, now it
> is rejected with error.
> That might be fine though, since writer always only writes connections that
> verify(). Hence, writer supposedly never wrote such a connection (which
> would need investigation and confirmation). So, at worst, the user edited
> such files by hand and they will stop working because we now declare them as
> invalid.
> That's still kinda ugly. Can we avoid rejecting files that we accepted
> previously?

My aim was to be coherent in dealing with properties unused in current ipv4.method: write them all. Not happy to be incoherent here, saving just a part of them... but we don't want to break running config on updates, agreed.
Also moved back in place the NM_SETTING_IP4_CONFIG_METHOD_DISABLED check, skipping the dhcp properties from reading (we don't write them in first place as instead we do for the SHARED method).

Updated branch fg/dhcp-hostname_persist-rh1519299

Comment 7 Thomas Haller 2018-01-23 09:39:16 UTC
(In reply to Francesco Giudici from comment #6)
> (In reply to Thomas Haller from comment #5)

> > That's still kinda ugly. Can we avoid rejecting files that we accepted
> > previously?
> 
> My aim was to be coherent in dealing with properties unused in current
> ipv4.method: write them all. Not happy to be incoherent here, saving just a
> part of them... but we don't want to break running config on updates, agreed.
> Also moved back in place the NM_SETTING_IP4_CONFIG_METHOD_DISABLED check,
> skipping the dhcp properties from reading (we don't write them in first
> place as instead we do for the SHARED method).
> 
> Updated branch fg/dhcp-hostname_persist-rh1519299

nm-setting-ip4-config.c:verify() only rejects dns and dns-searches for "shared" method. All other settings are valid as far as libnm is concerned (regardless of whether NM actually makes use of them with "shared" method or not).

Properties like NM_SETTING_IP_CONFIG_DNS_PRIORITY are not forbidden by libnm, so a user can set them together with shared method, and then ifcfg-rh reader will fail to read the same setting back. One question is whether (in this example) DNS-PRIORITY makes sense or nm_connection_normalize() should get rid of it. But as far as ifcfg-reader/writer are concerned, they should read and write every possible libnm configuration (of a verified, normalized connection).

So, better not shortcut everything, just skip DNS and DNS-SEARCHES.

Comment 8 Francesco Giudici 2018-02-26 17:49:17 UTC
(In reply to Thomas Haller from comment #7)
> (In reply to Francesco Giudici from comment #6)
> [...]
> 
> nm-setting-ip4-config.c:verify() only rejects dns and dns-searches for
> "shared" method. All other settings are valid as far as libnm is concerned
> (regardless of whether NM actually makes use of them with "shared" method or
> not).
> 
> Properties like NM_SETTING_IP_CONFIG_DNS_PRIORITY are not forbidden by
> libnm, so a user can set them together with shared method, and then ifcfg-rh
> reader will fail to read the same setting back. One question is whether (in
> this example) DNS-PRIORITY makes sense or nm_connection_normalize() should
> get rid of it. But as far as ifcfg-reader/writer are concerned, they should
> read and write every possible libnm configuration (of a verified, normalized
> connection).
> 
> So, better not shortcut everything, just skip DNS and DNS-SEARCHES.

Well, I'm not convinced 100%: I think we should enforce writing all the config... yes, some mangled configuration files would then be dropped. But, hey, they are wrong after all, also if we did not detected them in the past (we just silently ignored the values there). Skipping "dns" and "dns-search" but not "dns-priority" looks pretty weird to me.
Anyway, as this is an unlikely corner case, let's stay on the "safe" side, and avoid reading "dns" and "dns-searches" only.

Branch fg/dhcp-hostname_persist-rh1519299 updated and rebased.

Comment 9 Thomas Haller 2018-02-26 17:58:51 UTC
The point is, nm_connection_verifiy() is the guideline what constitutes a valid connection and what not. Reader/Writer must persist and load every valid connection.

If you dislike the behaviour of verify(), then fix it in verify() Since this is already existing API, just tightening up verify() could break existing connections. For that, there is nm_connection_normalize(), to silently rectify a connection. If you don't like that either, then the issue cannot be fixed, and a connection can have unused/inconsistent values.

If you dislike the inconsistency, you could also relax verify() to allow DNS and DNS_SEARCH properties. Then it's more relaxed, but at least consistently so.


> Branch fg/dhcp-hostname_persist-rh1519299 updated and rebased.

lgtm

Comment 10 Francesco Giudici 2018-02-28 10:29:42 UTC
Yeah, I would like to get rid of the inconsistency... but don't relax the checks on verify, instead tighten them... without breaking existing configurations.
Yes, I basically want to eat my cake and have it, too :-)

So, ok, let us be on the safe side preserving working connections (and current checks).

> lgtm
thanks!

Patch merged on master:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ff1884a219

Comment 15 errata-xmlrpc 2018-10-30 11:11:02 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:3207


Note You need to log in before you can comment on or make changes to this bug.