Bug 1515829

Detected by CI test @pppoe_over_vlan

"assertion failed: (ifindex > 0)"

#4  0x000055df21c924b6 in nm_ip6_config_capture
#5  0x000055df21d023f3 in act_stage3_ip6_config_start
#6  0x000055df21d15578 in nm_device_activate_stage3_ip6_start
#7  0x000055df21d18c69 in activate_stage3_ip_config_start
...

Please review branch bg/ppp-rh1515829.

»···if (priv->ifindex <= 0) {
»···»···priv->ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, priv->ip_iface);
»···»···if (priv->ifindex <= 0) {

there is a race here. Could you at least retry once with nm_platform_process_events()? Maybe the netlink message about the interface is already pending.


>> device: ppp: rename fields

device/trivial: ...


+    if (priv->iface)
+         g_assert_cmpstr (priv->iface, ==, iface);

iface comes from untrusted, you cannot assert against it.



+         if (renamed)
+              nm_manager_remove_device (nm_manager_get (), iface, NM_DEVICE_TYPE_PPP);
+
+         /* Once the device gets an ifindex, start with IP configuration */
+         nm_device_activate_schedule_stage3_ip_config_start (device);


if it got renamed, you shouldn't start stage3, should you?

Created attachment 1370038 [details]
[PATCH 1/2] ppp: introduce SetInterface pppd plugin D-Bus method

Created attachment 1370040 [details]
[PATCH 2/2] ppp/trivial: rename field

(In reply to Thomas Haller from comment #3)
> »···if (priv->ifindex <= 0) {
> »···»···priv->ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET,
> priv->ip_iface);
> »···»···if (priv->ifindex <= 0) {
> 
> there is a race here. Could you at least retry once with
> nm_platform_process_events()? Maybe the netlink message about the interface
> is already pending.

Fixed.

> iface comes from untrusted, you cannot assert against it.

> if it got renamed, you shouldn't start stage3, should you?

Solved in different ways.

SetInterface uses the interface name. could we send the ifindex instead?

The ID of a link is the ifindex (ignoring the fact, that a ifindex might be reused). Contrary to an ifname, the ifindex cannot change.

Yes, inside the ppp plugin, there is only ifname available. Which I would consider a bug on it's own. You can immediately resolve the ifindex with if_nametoindex().
Of course, that is still racy with with possibility that the interface might be renamed or disappeared already. But it minimizes the time for the race by resolving the name as early as possible.

If you are unable to resolve the ifname (e.g. because the interface is already gone), then send ifindex "0" to NM to indicate that something went horribly wrong and NMDevice fails activation.


That also means, you need something like nm_device_set_ip_ifindex(), which does not exist yet. But I think that would be the right API, not set_ip_ifname(). In nm_device_set_ip_ifindex() you would instead need to lookup the ifname (e.g. by consulting the platform cache, possibly in conjunction with process_events). That might fail too, but you can handle that the same as receiving ifindex 0 and fail activation too.

Pushed branch bg/ppp-set-ifindex-bgo1515829.

(In reply to Beniamino Galvani from comment #8)
> Pushed branch bg/ppp-set-ifindex-bgo1515829.

looks mostly good. How about the two fixups?

(In reply to Thomas Haller from comment #9)
> (In reply to Beniamino Galvani from comment #8)
> > Pushed branch bg/ppp-set-ifindex-bgo1515829.
> 
> looks mostly good. How about the two fixups?

Squashed, thanks.

Branch merged to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3d4652fc6e21028068f3b59b8e95b8d03da3105e

Created attachment 1391902 [details]
[PATCH nm-1-10] ppp: don't start IPv6 configuration on the device

Patch for nm-1-10 branch.

(In reply to Beniamino Galvani from comment #11)
> Created attachment 1391902 [details]
> [PATCH nm-1-10] ppp: don't start IPv6 configuration on the device
> 
> Patch for nm-1-10 branch.

lgtm

Applied to branch nm-1-10:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-10&id=258f4fc76961f564c5d63d1eaf5246b21c2d0ce0

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:0778