Created attachment 968938 [details]
device: Disable kernel RA handling before configuring v6 addresses

(In reply to Lubomir Rintel from comment #1)
> Created attachment 968938 [details]
> device: Disable kernel RA handling before configuring v6 addresses

I think this needs a

  if (nm_device_uses_assumed_connection (self)) {
  }


otherwise, LGTM

Created attachment 968976 [details]
device: don't change IPv6 sysctls if there's an assumed connection

We can't filter out all nm_device_ipv6_sysctl_set() calls for assumed connections; if the user deactivates an NM-owned assumed connection, then we need to reset the sysctls back to their default values.

(In reply to Dan Winship from comment #4)
> We can't filter out all nm_device_ipv6_sysctl_set() calls for assumed
> connections; if the user deactivates an NM-owned assumed connection, then we
> need to reset the sysctls back to their default values.

gboolean
nm_device_ipv6_sysctl_set (NMDevice *self,
                           const char *property,
                           const char *value
                           gboolean skip_assumed)

?

(In reply to Thomas Haller from comment #5)
> (In reply to Dan Winship from comment #4)
> > We can't filter out all nm_device_ipv6_sysctl_set() calls for assumed
> > connections; if the user deactivates an NM-owned assumed connection, then we
> > need to reset the sysctls back to their default values.
> 
> gboolean
> nm_device_ipv6_sysctl_set (NMDevice *self,
>                            const char *property,
>                            const char *value
>                            gboolean skip_assumed)
> 
> ?

I'm not too sure. I think we'd need to pass skip_assumed through restore_ip6_properties() and set_disable_ipv6() and it might be less confusing to just add the conditionals wherever they are missing?


Pushed to lr/sysctls-rh1173087, but didn't give it much testing yet.

(In reply to Lubomir Rintel from comment #6)
> (In reply to Thomas Haller from comment #5)
> > (In reply to Dan Winship from comment #4)
> > > We can't filter out all nm_device_ipv6_sysctl_set() calls for assumed
> > > connections; if the user deactivates an NM-owned assumed connection, then we
> > > need to reset the sysctls back to their default values.
> > 
> > gboolean
> > nm_device_ipv6_sysctl_set (NMDevice *self,
> >                            const char *property,
> >                            const char *value
> >                            gboolean skip_assumed)
> > 
> > ?
> 
> I'm not too sure. I think we'd need to pass skip_assumed through
> restore_ip6_properties() and set_disable_ipv6() and it might be less
> confusing to just add the conditionals wherever they are missing?
> 
> 
> Pushed to lr/sysctls-rh1173087, but didn't give it much testing yet.

Indeed, restore_ip6_properties() etc. would need that parameter too.

I makes it more explicit that these functions touch the interface. And when using them you have to think for a second which behavior you want.


I like how it could enhance logging by saying:
  "At this point NM would set /sys/value=XYZ, but skip it for assumed connection. (Bonus output:) current actual value is /sys/value=ZYX"

Okay.

Pushed updated lr/sysctls-rh1173087, please have a look.

(In reply to Lubomir Rintel from comment #8)
> Okay.
> 
> Pushed updated lr/sysctls-rh1173087, please have a look.

The branch is based on nm-1-0 branch. It should be based on master.


Otherwise LGTM. I like the @skip_assumed parameter as is.

Well, it would definitely be a lot simpler to just leave nm_device_ipv6_sysctl_set() as is, and just add a check in restore_ip6_properties().

> I makes it more explicit that these functions touch the interface. And when
> using them you have to think for a second which behavior you want.

I think the name makes it pretty clear that it touches the interface. And you have to think about this *everywhere*...

All current uses of set_nm_ipv6ll(TRUE) and set_disable_ipv6("1") are already protected by assumed checks, so this branch is a NOP for those two right?

It also stops touching disable_ipv6 for ipv6.method=IGNORE, which is valid.

It also stops touching stuff in nm_device_cleanup(), which I think is a NOP because we no longer deactivate assumed connections at all, so all the cases that would call nm_device_cleanup() are no longer hit for assumed connections?

What it will do now is two things:

1) stop setting sysctls for IPv6 RA processing for assumed connections; wouldn't this cause NetworkManager and the kernel to fight for RA processing?  eg, if an assumed connection has accept_ra*=1, then NM will no longer stop kernel RA processing, but will still start userspace RA handling.  The kernel and NM will be listening for the same RAs and trying to do the same thing now, right?

2) stop clearing sysctls when the user explicitly does 'nmcli con down <assumed connection>'; which is also the case if you do "nmcli con up foobar; systemctl restart NetworkManager; nmcli con down foobar".  I think this should get changed so that NM always clears them out?  Since we dont' deactivate assumed connections anymore, the only way NM woudl clean up an assumed connection would be through explicit D-Bus requests to deactivate the connection or disconnect the device, in which case we would expect NM to clean up the stuff.

Actually, I think git master may already have the correct beahvior here?  AIUI the original bug report was that for 'manual' IPv6 configurations, accept_ra could still be left enabled.

From what I can see, nm_device_cleanup() will get called when the device moves to UNAVAILABLE state, which sets accept_ra=0.  Also, ip6_managed_setup() gets called at the same place, which sets the extra accept_ra_* sysctls to the right values.

Lubomir, does that look right to you, does this bug need any further work?  Could you re-test with NM from current RHEL7.1 and see if you can reproduce the issue still?

Created attachment 980581 [details]
[PATCH] device: avoid toggling disable_ipv6 unless activating a  connection that needs it

I believe that the last remaining case where our sysctl handling was not correct was that we were toggling disable_ipv6 for ipv6.method=ignore connection.

I tried to address that with the patch I'm attaching, though I'm really not too sure whether it's correct. I tried to explain what was I intending to do in the commit message. I'm a bit worried that I got too confused about the existing code and the bits that I'm removing actually weren't as useless as I thought.

I think the changes to _set_state_full() for UNAVAILABLE mode will result in interfaces that might stay in UNAVAILABLE for a long time, but are NM managed (like wifi interfaces where the supplicant might not be running or ethernet interfaces without a carrier) will get kernel IPv6 LL addresses now?  Removing the ip6_managed_setup (self); call (which turns on user IPv6LL) will leave them with kernel LL until they move to DISCONNECTED.

Also for the DISCONNECTED case, I think that will change sysctls for assumed connections where we don't want to touch stuff?

I'm closing this.

The original issue reported here is long fixed and there doesn't seem to be any more outstanding issue. We do still toggle disable_ipv6 for activation of method=ignore connections which wipes ipv6 configuration, but it's because when a device has ipv6 configuration the activation downs the assumed connection first. And that sounds sort of expected.