After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772880 - [review] cleanup handling of permanent MAC address for devices that don't have one [th/preserve-fake-perm-hwaddr-bgo772880]
[review] cleanup handling of permanent MAC address for devices that don't hav...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-10-13 17:06 UTC by Thomas Haller
Modified: 2016-10-28 15:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2016-10-13 17:06:37 UTC 
please review
Comment 1 Dan Williams 2016-10-15 16:45:30 UTC 
Seems OK to me.... but we should test what happens with udev rules that assign a stable MAC address to the device like we've been recommending users do for years.  Just to make sure that the udev-assigned MAC is considered the permanent one (even though it's fake) and not whatever random address the kernel assigned on hotplug.  I'm pretty sure the platform code only exposes the device once all the rules have been applied, but we should check that just in case.
Comment 2 Thomas Haller 2016-10-15 17:52:05 UTC 
the primary usecase would be containers, where there is no udev.


Note that this only takes effect

  - for devices that have no permanent MAC address (veth)
  - after restart of NetworkManager, if NetworkManager in the previous
    run already saw the device.


But the issue you raise that udev rules change the MAC address is independent of this change. Platform exposes the device as soon as it sees it in netlink. But it still has an "initalized" flag set to false -- unless running in a container, where there is no udev.

Indeed, that would trigger platform_link_added() and nm_device_realize_start(), which then calls nm_device_update_permanent_hw_address() and freezes the device to the possibly wrong MAC address.
That should probably be fixed...
Comment 3 Thomas Haller 2016-10-24 11:11:15 UTC 
ok, added a follow up commit that aims to wait a bit more for UDEV to settle...

It's however not clear that this will work in every case, because we might need a permanent MAC address earlier. That is, every access to nm_device_get_permanent_hw_addr() *needs* a MAC address now, it cannot wait for UDEV anymore.
Comment 4 Beniamino Galvani 2016-10-25 20:30:44 UTC 
> core: persist the fake permanent hardware address to the device's statefile

+               dev_state = nm_config_device_state_load (nm_config_get (), priv->ifindex);
+               if (dev_state) {
+                       if (   dev_state

Remove the outer if.

> device: delay capturing permanent MAC address until UDEV is settled

+       /* the user is advised to configure stable MAC addresses for software devices via
+        * UDEV. Thus, check whether the link is fully initalized. */

s/initalized/initialized/

The rest LGTM.
Comment 5 Beniamino Galvani 2016-10-28 09:35:43 UTC 
New branch still looks fine to me.