(In reply to comment #0)
> (The IGNORE state was intended as a backwards compatible state that would make
> NM just ignore anything to do with IPv6 and not turn it off, since the kernel
> can provide an address given a router.

More specifically, it was put in because we couldn't make AUTO the default given the way activation worked at that time (it would have made activation take 45 seconds on networks with no IPv6 support). But we couldn't make DISABLED the default either, because that would suddenly break people who had gotten IPv6 support working behind NM's back in a previous release.

> The IGNORE state should be deprecated
> and eventually removed in the future, and replaced with AUTO.)

I think it's the future already.

I think that link-local addresses are not an option for slaves. They definitely created problems with bridge slaves and I actually thought that newere kernels don't perform link-local configuration nor global configuration on bridge slaves. Not sure about bond slaves.

AFAIK the method configuration shouldn't be misused for things like that unless you know what you're doing. It should be as simple as “slaves should never be autoconfigured” and that kernel should enforces this for kernel-internal configuration and NetworkManager should be enforcing this for external configuration.

(In reply to comment #2)
> AFAIK the method configuration shouldn't be misused for things like that unless
> you know what you're doing. It should be as simple as “slaves should never be
> autoconfigured”

but I think the way to do this that is consistent with the rest of NM is to say that a slave connection defaults to ipv4.method=NONE and ipv6.method=NONE, and it fails to validate if the user has explicitly set them to something else.

OK. We can revisit it if it happens to cause any problems. That was not a strong opinion.

The only reason this is happening is (I think) because we're setting the accept_ra for slaves.  We can obviously hack around that now without adding DISABLED, but we might as well just add DISABLED and be done with it.  IGNORE should be mapped to AUTO I think.

Created attachment 245591 [details] [review]
libnm-util: recognize "disabled" as an IPv6 config method

as a preliminary, let's commit this to nm-0-9-8 before 0.9.8.2 goes out...

Created attachment 245592 [details] [review]
libnm-util: recognize "disabled" as an IPv6 config method

um... better yet, let's commit this version that isn't completely broken

Hmm, that would change the setting though, so that if you then saved the connection, you'd end up with "ignore" when you switched back to 0.9.10.  Would it be better to just check both "disabled" and "ignroe" in the code and do the same thing?

Hm... actually, this only affects the keyfile plugin, since none of the others record the literal value of the method property in the file. So maybe we can just ignore the problem, since it basically will only affect the current developers, and we're all using Fedora where the keyfile plugin doesn't get used for much (except VPNs, which mostly don't support IPv6 anyway).

(It's possible to accept "disabled" and treat it as "ignore" everywhere, but it's a bigger patch that way, so more chance of messing things up...)

You're right, I just pushed the patch to 098.

oh, ok. By "ignore the problem" I meant "ignore the problem this patch was trying to solve", not "ignore the problem that you pointed out with my patch". But I'm fine ignoring either problem!

(In reply to comment #7)
> Created an attachment (id=245592) [details] [review]
> libnm-util: recognize "disabled" as an IPv6 config method
> 
> um... better yet, let's commit this version that isn't completely broken

Boo, except it was still totally broken because it wants == 0 :(  Fixed in 098.

I see that network manager reports to GNetworkMonitor that the network is available, even it isn't, because I have WiFi disabled (with a hardware switch) and I just unplugged the cable. It seems it's related to this bug report, after my investigation, because the GNetworkMonitor received both IPv4 and IPv6 addresses (or how you call it at gnetworkmonitornetlink.c:read_netlink_messages()), but I have setup the IPv6 as "Ignore" in NetworkManager preferences. The NetworkManager status icon correctly shows there is no network connection, thought, it's only the GNetworkMonitor reporting wrong status for network-available. Changing the "Ignore" to "Automatic" fixes this behaviour, but that's not what I want to do, I want to keep the "Ignore", in a meaning of "Disable", from my point of view.
This is with:
   NetworkManager-0.9.8.2-4.fc19.x86_64
   glib2-2.36.3-2.fc19.x86_64

Created attachment 248369 [details]
simple test program

Compile this with the command at the first line of the file, and run it. It listens for network changes. The outcome should be "available: 1" when network cable is plugged in, and "available: 0" when network cable is not plugged in, with WiFi disabled and IPv6 set to "Ignore" on the Ethernet connection.

(In reply to comment #13)
> I see that network manager reports to GNetworkMonitor that the network is
> available

GNetworkMonitor doesn't talk to NetworkManager, it talks to the kernel directly.

The problem still seems to be in NetworkManager to me, or related to the way it configures the interface, because that's the only thing I change, and where I change it (in NetworkManager), and the whole thing either works or not.

*** Bug 704014 has been marked as a duplicate of this bug. ***

I'm afraid this bugreport got too messy, mixing three different things:

1) Dan Williams reported that the kernel sometimes configures slave devices and later added that it's because we set accept_ra on slaves. The good news is that with userspace router discovery support (nm-rdisc) accept_ra should be always zero for nm-managed devices.

2) We want to have DISABLED method for IPv6.

3) We don't want to have IGNORE method for IPv6.

4) A probably totally unrelated problem reported by Milan Crha that should be reported for GNetworkMonitor and further investigated. Even if there is any relation to NetworkManager, it should be explicitly stated in a separate bug report.

(In reply to comment #18)
> I'm afraid this bugreport got too messy, mixing three different things:
> 
> 1) Dan Williams reported that the kernel sometimes configures slave devices and
> later added that it's because we set accept_ra on slaves. The good news is that
> with userspace router discovery support (nm-rdisc) accept_ra should be always
> zero for nm-managed devices.

If dcbw is right about accept_ra, it will be easily solved by always setting accept_ra to zero (already required by all IPv6 methods except 'ignore' which will be removed) and/or not touching accept_ra for enslaved devices. This is incidentally also covered in bug 682932:

> Disabling the IPv4/IPv6 protocol itself on a specific device or for the whole
> system is IMO out of scope of NetworkManager. AFAIK the only IPv6 sysctl that
> should be used by NetworkManager is accept_ra nad that one should be always set
> to zero for managed devices. It should *not* be resored to its orinal value
> while the device is still configured. To be more precise, the only time we
> should restore accept_ra is *just after* the device is being set down by
> NetworkManager and it should *always* be set to zero when NetworkManager sets
> the device up or takes it into management.

AFAIK this is the *only* think that should be fixed to close this particularl bug report, unless dcbw was wrong of course.

> 2) We want to have DISABLED method for IPv6.

This is now included in bug 682932. Removing from summary.

> 3) We don't want to have IGNORE method for IPv6.

This is now included in bug 682932.

> 4) A probably totally unrelated problem reported by Milan Crha that should be
> reported for GNetworkMonitor and further investigated. Even if there is any
> relation to NetworkManager, it should be explicitly stated in a separate bug
> report.

This is up to Milan to report properly.

(In reply to comment #19)
> > 4) A probably totally unrelated problem reported by Milan Crha that should be
> > reported for GNetworkMonitor and further investigated. Even if there is any
> > relation to NetworkManager, it should be explicitly stated in a separate bug
> > report.
> 
> This is up to Milan to report properly.

Yup, I will wait for a result on this, just in case any fix here will influence behaviour of the test.c.

(In reply to comment #20)
> (In reply to comment #19)
> > > 4) A probably totally unrelated problem reported by Milan Crha that should be
> > > reported for GNetworkMonitor and further investigated. Even if there is any
> > > relation to NetworkManager, it should be explicitly stated in a separate bug
> > > report.
> > 
> > This is up to Milan to report properly.
> 
> Yup, I will wait for a result on this, just in case any fix here will influence
> behaviour of the test.c.

After IRC discussion with Milan, I came to a conclusion that Milan should test 'link-local' instead of 'ignore' and his usability concerns would be sorted out by removing 'ignore' and renaming 'link-local' to 'disabled' as I proposed in:

https://bugzilla.gnome.org/show_bug.cgi?id=682932#c2

Therefore the only concern for this bug report is:

(In reply to comment #19)
> If dcbw is right about accept_ra, it will be easily solved by always setting
> accept_ra to zero (already required by all IPv6 methods except 'ignore' which
> will be removed) and/or not touching accept_ra for enslaved devices. This is
> incidentally also covered in bug 682932:
> 
> > Disabling the IPv4/IPv6 protocol itself on a specific device or for the whole
> > system is IMO out of scope of NetworkManager. AFAIK the only IPv6 sysctl that
> > should be used by NetworkManager is accept_ra nad that one should be always set
> > to zero for managed devices. It should *not* be resored to its orinal value
> > while the device is still configured. To be more precise, the only time we
> > should restore accept_ra is *just after* the device is being set down by
> > NetworkManager and it should *always* be set to zero when NetworkManager sets
> > the device up or takes it into management.
> 
> AFAIK this is the *only* think that should be fixed to close this particularl
> bug report, unless dcbw was wrong of course.

Changing the summary accordingly. It's also closely related to nm-rdisc now that we touch accept_ra (1) to disable kernel's autoconfiguration at all times and (2) perform the autoconf in userspace.

I'm reverting this back to its previous summary, because adding an IPv6 "disabled" method is important for multiple reasons, and is more important than completely reorganizing IP config methods as in bug 682932.

I've pushed the danw/ipv6disable branch (on top of danw/ipwarning for bug 708875)

Though i should also mention that that branch isn't tested yet, since I don't want to bother testing it if we're *not* going to go with the simpler path first...

(In reply to comment #22)
> I'm reverting this back to its previous summary, because adding an IPv6
> "disabled" method is important for multiple reasons, and is more important than
> completely reorganizing IP config methods as in bug 682932.

Sure.

The only concern is that we more or less agreed that setting sysctl for disabled ipv6 is a separate action from enabling/disabling autoconfiguration. It remains a question whether it's useful to extend the ipv6.method API now that we want to split method into separate options and use method as a fallback.

Also a rationale why it's so important to have a different value for method than method=link-local in the short term would be handy.

> I've pushed the danw/ipv6disable branch (on top of danw/ipwarning for bug
> 708875)

Can't access fdo right now, will look at that later.

(In reply to comment #24)
> > I've pushed the danw/ipv6disable branch (on top of danw/ipwarning for bug
> > 708875)
> 
> Can't access fdo right now, will look at that later.

Now it works...

nm-setting-connection.c hunk seems to treat IGNORE and DISABLED the same (and distinct from the others), while the docs say it's like AUTO.

Also the documentation should in my opinion clearly state the difference (and/or similarity) between DISABLED and LINK_LOCAL. The summary of the bug mentions slaves but they can live with just anything if the implementation keeps away from forcing configuration on the device. AFAIK by default the kernel doesn't try to perform any configuration for slave devices.

(In reply to comment #22)

I reviewed it, my ~comments~ are on branch th/review/danw/ipv6disable.

Also, `make check` fails for me... I will look into that later.

(In reply to comment #26)
> I reviewed it, my ~comments~ are on branch th/review/danw/ipv6disable.

I disagree with the fixups to "libnm-util: centralize find_setting_by_name code"; making the function type-safe just requires us to do extra typecasting. And if nm_setting_get_name() returns NULL, then things are horribly horribly messed up, so there's no need to check for that.

Your comments on update_ip6_disable_save() forced me to look at the surrounding code more carefully (that function had basically just been a cut+paste of update_accept_ra_save()), which then led to a long IRC discussion between dcbw and me, which led to a new commit, "core: clean up accept_ra/use_tempaddr handling". The commit message should explain what it's doing. We probably want dcbw to review that patch before it goes in.

Re-pushed, and also, I forgot to mention before, but there's also an ipv6disable branch in network-manager-applet.

Late to the party, but danw/ipwarning looked good to me as well.

> libnm-util: centralize find_setting_by_name code

Shouldn't nm_setting_find_in_list() return an NMSetting* instead of a gpointer?  Seems more correct even if it's not used right now.

> libnm-util: fix slave IP config checking

Should now handle 'team' interfaces too.  And I'd just call it "is_slave" here, since is_bond_bridge_team_slave is getting kinda long :)

> core: remove redundant setting of accept_ra

Short description of why?  Maybe "Now that NetworkManager uses libndp for userspace RA handling, it always sets accept_ra to 0 and thus we don't need to re-set it in stage3" or something along those lines.

> all: add NM_SETTING_IP6_CONFIG_METHOD_DISABLED, deprecate _IGNORE

Maybe we could consoldiate the update_xxx_save() functions into one generic function, and then re-implement the individual ones to just call the generic one with some addresses of variables?  Not sure if that's less/more code or if it's more confusing due to the additional indirection.  What do you think?

Whitespace in dispose() for the disable_save bit, you fixed it for accept_ra there but not for disable.


So the patch series looks good, though I'd like to take 20 minutes to think about the implications of making IGNORE = DISABLED.  That is a behavior change, since previously if a connection had IPv6 disabled in the config files the interface would still get an IPv6 address via kernel IPv6LL addressing, and now it won't.  I don't know if that's a problem or not, but I'll think about it.

What do you think about the following for IGNORE/DISABLE to preserve backwards-compat behavior?  I'm a bit uneasy about a change like that at this point, because it will mean pretty big behavior changes for existing installs.

1) make IGNORE = LINK_LOCAL+may-fail instead of disabled; this would preserve the current semantics of IGNORE, where the kernel will assign an IPV6LL address whenever the interface is IFF_UP.  We would still automatically set slaves to DISABLED though.

2) ifcfg-rh introduces a new IPV6INIT value "disabled" that maps to DISABLED, but keeps "no" mapping to LINK_LOCAL.

3) Editor and cli tools add DISABLED and remove IGNORE.

Yeah, it's a bit confusing, and if we had to start over we wouldn't have done it this way, but I think it's important to not hugely change how IGNORE works if people still have scripts using it.  For Fedora, we can work with the initscripts team to add "disabled" with the same semantics as NetworkManager.

Ok, reviewing the right commits this time...

> core: clean up accept_ra/use_tempaddr handling

We talked about this on IRC, but I think we should set accept_ra when the ip_iface gets set if ip_iface != iface.  We don't need to save that value at all though, since yeah it'll be transient and go away when the connection is torn down.

(In reply to comment #30)
> What do you think about the following for IGNORE/DISABLE to preserve
> backwards-compat behavior?

BTW, I don't think this is currently terribly clear in the code/commit messages, but if you create an NMConnection with method=IGNORE and then save it via nm_remote_settings_add_connection(), it will end up being saved as method=AUTO, not method=DISABLED (unless it's a slave). (I guess it probably should be setting may-fail too.) So the backward-compatibility concerns are only around the interpretation of "IPV6INIT=no", not around the interpretation of NM_IP6_CONFIG_METHOD_IGNORE.

> 1) make IGNORE = LINK_LOCAL+may-fail instead of disabled; this would preserve
> the current semantics of IGNORE

No, the current semantics of IGNORE are closer to AUTO. It's like AUTO + may-fail + ignore-auto-dns + "never-default-unless-there-are-no-managed-ipv6-connections-in-which-case-go-ahead-and-default" + "don't-do-dhcp-even-if-the-RA-says-to". Which are pretty ugly semantics.

OTOH, it seems that those are also the semantics of IPV6INIT=no in initscripts. Ugh. So maybe we do need to add IPV6INIT=disabled...

> Yeah, it's a bit confusing, and if we had to start over we wouldn't have done
> it this way, but I think it's important to not hugely change how IGNORE works
> if people still have scripts using it.

So, as above, if you have scripts using IGNORE via nmcli/D-Bus, then it will get translated to AUTO.

And if you have scripts writing out ifcfg files that say "IPV6INIT=no", it would be hard to argue that you wrote that expecting that it would cause IPv6 to still work...

The only real compatibility problem IMHO is with peoples' existing connection definitions when they upgrade, but it's not very difficult for them to just go in and fix them in nm-connection-editor when they notice that they're broken...

Let me back up a bit and explain what IGNORE was supposed to achieve.  Then we can figure out if it actually still works that way or not, and what to do about it.

Back when NM didn't support IPv6 natively it was important not to step on any IPv6 configuration that the kernel did, and having NM completely ignore IPv6 did that.

Then when we added IPv6 support, which arguably wasn't very good at the start, we added the IGNORE mode and defaulted to it so that users wouldn't experience disruption or changes when doing a "yum upgrade" on NM one day.  I believe we even defaulted to IGNORE if IPV6INIT didn't exist in the ifcfg file, so that only if IPV6INIT=yes did we treat that as AUTO.

Anaconda didn't write ifcfg files with IPV6INIT at all then, and nm-connection-editor defaulted to IGNORE too.  At some point we changed the editor to default to AUTO, but only after we added the parallel-IP-methods stuff and the 'may-fail' logic, so that we could reliably turn IPv6 on-by-default and not have RA-watching block a successful IPv4 retrieval.

The initscripts treat IPV6INIT=no as "don't do anything IPv6, exit early".  They don't turn IPv6 *off* at all, they simply exit early and touch nothing.  That's the behavior that IGNORE was supposed to emulate, because in this case, the kernel was still set up for IPv6 RA by default, and if IPV6INIT=no the kernel would still happily listen for RAs and assign the global IPv6 address + prefix to the interface.

So now, if we treat IPV6INIT=no as DISABLED, we'll kill those configurations that currently have IPv6 working, due only to the kernel's defaults.  Note that we actually *already* half broke them when we switched to libndp because we set accept_ra to 0 now everywhere.  But even now, you'll still get an IPv6LL address from the kenrel on that interface, like you always did.  With these patches, that will no longer happen.

I'm just trying to find a way to preserve the existing semantics of 'IPV6INIT=no' so we don't just change behavior of existing installs on a package upgrade.  I'd thought that IPV6INIT=disabled distinct from =no would do that if we can accomodate that somehow.

I looked at: core: clean up accept_ra/use_tempaddr handling

Regarding 'accept_ra', looks actually great (except the IGNORE problem found by dcbw, see below). Looks like what I wanted to do plus much more.

But unless the patch is intended to go to the 0.9.8 branch, now when we always use userspace router discovery, there's IMO no need to ever set use_tempaddr, as it's suppressed by accept_ra=0 anyway. In git master, it would be IMO better to ignore use_tempaddr altogether.

Regarding the ipv6_disabled, I don't think it's a good idea to use 'method' for [not] setting up kernel link-local IPv6, nor I see the alleged importance of setting disable_ipv6 from NetworkManager instead of sysctl.conf.

(In reply to comment #33)
> The initscripts treat IPV6INIT=no as "don't do anything IPv6, exit early". 
> They don't turn IPv6 *off* at all, they simply exit early and touch nothing. 
> That's the behavior that IGNORE was supposed to emulate, because in this case,
> the kernel was still set up for IPv6 RA by default, and if IPV6INIT=no the
> kernel would still happily listen for RAs and assign the global IPv6 address +
> prefix to the interface.

That explains a lot of existing API/dispatcher problems.

> So now, if we treat IPV6INIT=no as DISABLED, we'll kill those configurations
> that currently have IPv6 working, due only to the kernel's defaults.

I think we should *never* treat 'ignore' as 'disabled' as it was mostly used to achieve a working configuration. I would instead treat it as 'auto' when phasing out.

> Note that we actually *already* half broke them when we switched to libndp
> because we set accept_ra to 0 now everywhere.

That doesn't seem to be true for git master. The only added accept_ra modification I see in bd1c7fb is in addrconf6_start() which isn't called for IGNORE. All other accept_ra calls seem to be older than libndp rdisc integration.

But that means it's a feedback for danw's patchset here, as he's making substantial changes to accept_ra usage. Either IGNORE should be removed (aliased to AUTO) before the respective patch is applied, or the patch should be changed to 

> But even now, you'll still get an IPv6LL address from the kenrel on that interface, like you always did. With these patches, that will no longer happen.
> 
> I'm just trying to find a way to preserve the existing semantics of
> 'IPV6INIT=no' so we don't just change behavior of existing installs on a
> package upgrade.  I'd thought that IPV6INIT=disabled distinct from =no would do
> that if we can accomodate that somehow.

I think we're technically pretty much done if we:

1) Avoid setting sysctl's disable_ipv6.
2) Avoid setting sysctl's use_tempaddr.
3) When method=IGNORE, also avoid setting sysctl's accept_ra.
4) When sysctl's disable_ipv6 is set, avoid setting any ip6 sysctls and don't perform router discovery.

All other things could be se

(In reply to comment #34)
> All other things could be se

All other things could be done carefully at any time as part of the API redesign (bug #682932).

(In reply to comment #34)
> But unless the patch is intended to go to the 0.9.8 branch, now when we always
> use userspace router discovery, there's IMO no need to ever set use_tempaddr,
> as it's suppressed by accept_ra=0 anyway.

Ah, right. 

> Regarding the ipv6_disabled, I don't think it's a good idea to use 'method' for
> [not] setting up kernel link-local IPv6, nor I see the alleged importance of
> setting disable_ipv6 from NetworkManager instead of sysctl.conf.

Currently, if an NM-managed device has carrier, then it will have a link-local IPv6 address, even when NM considers the device to be disconnected. That means that any daemon listening on :: will be accessible to all other machines on your subnet. This is potentially bad for security reasons, and in general a violation of the principle of least surprise.

initscripts bug https://bugzilla.redhat.com/show_bug.cgi?id=496444 (later cloned to NM bug https://bugzilla.redhat.com/show_bug.cgi?id=982740) proposed making IPV6INIT=no mean "completely disabled", but the bug eventually got WONTFIXed because the provided patch sucked...

(Really, it seems to me like we ought to be setting default/ipv6_disable to 1, and then re-enabling it as needed on individual interfaces. That's the only way to solve the problem of the kernel autoconfiguring IPv6 on a newly-created interface before we even get a chance to see it...)

(In reply to comment #34)
> Regarding the ipv6_disabled, I don't think it's a good idea to use 'method' for
> [not] setting up kernel link-local IPv6, nor I see the alleged importance of
> setting disable_ipv6 from NetworkManager instead of sysctl.conf.

The kernel will still assign a link-local IPv6 address to the interface unless IPv6 is disabled by NM.  If you have one connection with IPv6 disabled and another with it enabled and switch between them, then we need to poke the ipv6 disable sysctl manually in NM.  sysctl.conf is more about the defaults, it's not really useful for runtime switching on a per-connection basis.

> (In reply to comment #33)
> > So now, if we treat IPV6INIT=no as DISABLED, we'll kill those configurations
> > that currently have IPv6 working, due only to the kernel's defaults.
> 
> I think we should *never* treat 'ignore' as 'disabled' as it was mostly used to
> achieve a working configuration. I would instead treat it as 'auto' when
> phasing out.

Agree that it shouldn't be treated as DISABLED, but treating it as AUTO is also problematic as now we're changing how the interface's address works.  It will now be configured under the control of libndp and NetworkManager instead of using the kernel's addressing.  That's a behavior change.

> > Note that we actually *already* half broke them when we switched to libndp
> > because we set accept_ra to 0 now everywhere.
> 
> That doesn't seem to be true for git master. The only added accept_ra
> modification I see in bd1c7fb is in addrconf6_start() which isn't called for
> IGNORE. All other accept_ra calls seem to be older than libndp rdisc
> integration.

Actually it is true.  See the setting of accept_ra to 0 in nm_device_deactivate() which gets run for all devices when they transition from UNAVAILABLE -> DISCONNECTED states.  So any device that is managed by NM and is hardware-available will get accept_ra set to 0 by design.  Because NM keeps all devices IFF_UP for carrier purposes, if we didn't set accept_ra=0, then the kernel would listen for RAs and assign routable addresses to all interfaces regardless of  whether they were activated or not.  Security problem.

> I think we're technically pretty much done if we:
> 
> 1) Avoid setting sysctl's disable_ipv6.
> 2) Avoid setting sysctl's use_tempaddr.
> 3) When method=IGNORE, also avoid setting sysctl's accept_ra.
> 4) When sysctl's disable_ipv6 is set, avoid setting any ip6 sysctls and don't
> perform router discovery.

1) Again, if we don't set disable_ipv6 then the kernel still assigns link-local addresses and thus you have link-local connectivity, even if the IPv6 addressing method is DISABLED.  Which seems kinda wrong.

2) agreed, we don't need to set use_tempaddr at runtime, though we still should restore it on shutdown/unmanage.  Since we need to implement IPv6 privacy internally in NM now that we're using libndp and not the kernel, we don't care about use_tempaddr on the kernel-side.

3) do you mean avoid setting it to 0?

4) agreed here as well, and when set, NM should probably also filter out IPv6 may-fail=FALSE connections (as in, they should not be candidates for autoconnect nor should they be in 'available-connections') since we know they cannot be completed successfully.

(In reply to comment #37)
> (In reply to comment #34)
> > Regarding the ipv6_disabled, I don't think it's a good idea to use 'method' for
> > [not] setting up kernel link-local IPv6, nor I see the alleged importance of
> > setting disable_ipv6 from NetworkManager instead of sysctl.conf.
> 
> The kernel will still assign a link-local IPv6 address to the interface unless
> IPv6 is disabled by NM.  If you have one connection with IPv6 disabled and
> another with it enabled and switch between them, then we need to poke the ipv6
> disable sysctl manually in NM.

Not if we leave it up to sysctl settings and explicitly state that we don't support per-connection disable_ipv6. Please note that it can be used as a temporary measure to get the important work done while leaving nitpicking for a later release.

> sysctl.conf is more about the defaults, it's
> not really useful for runtime switching on a per-connection basis.

Obviously. The question is (1) whether we need that option and (2) whether we need it to do it now. A "no" to either of them would mean you could benefit from a simple solution without API changes.

> > (In reply to comment #33)
> > > So now, if we treat IPV6INIT=no as DISABLED, we'll kill those configurations
> > > that currently have IPv6 working, due only to the kernel's defaults.
> > 
> > I think we should *never* treat 'ignore' as 'disabled' as it was mostly used to
> > achieve a working configuration. I would instead treat it as 'auto' when
> > phasing out.
> 
> Agree that it shouldn't be treated as DISABLED, but treating it as AUTO is also
> problematic as now we're changing how the interface's address works.

If we indeed want to remove the original meaning of IGNORE, we can either go with an error or with a best match. Another option is to keep it either with its original semantics (also problematic) or with modified semantics (not very useful then).

> It will
> now be configured under the control of libndp and NetworkManager instead of
> using the kernel's addressing.  That's a behavior change.

Indeed. And a large one that deserves the 10 in 0.9.10, but possibly not large enough to be a problem now that IPv6 configuration works pretty well.

> > > Note that we actually *already* half broke them when we switched to libndp
> > > because we set accept_ra to 0 now everywhere.
> > 
> > That doesn't seem to be true for git master. The only added accept_ra
> > modification I see in bd1c7fb is in addrconf6_start() which isn't called for
> > IGNORE. All other accept_ra calls seem to be older than libndp rdisc
> > integration.
> 
> Actually it is true.

It's a result of commit 7926b3c, looks like a follow-up to rdisc implementation. It wasn't strictly needed to make userspace router discovery work.

> See the setting of accept_ra to 0 in
> nm_device_deactivate() which gets run for all devices when they transition from
> UNAVAILABLE -> DISCONNECTED states.  So any device that is managed by NM and is
> hardware-available will get accept_ra set to 0 by design.

And I'm for keeping this change even if that means we need to drop IGNORE functionality.

> Because NM keeps all
> devices IFF_UP for carrier purposes, if we didn't set accept_ra=0, then the
> kernel would listen for RAs and assign routable addresses to all interfaces
> regardless of  whether they were activated or not.  Security problem.

Unless we set the accept_ra to the wrong value, it's questionable whether it's a NetworkManager problem.

> > I think we're technically pretty much done if we:
> > 
> > 1) Avoid setting sysctl's disable_ipv6.
> > 2) Avoid setting sysctl's use_tempaddr.
> > 3) When method=IGNORE, also avoid setting sysctl's accept_ra.
> > 4) When sysctl's disable_ipv6 is set, avoid setting any ip6 sysctls and don't
> > perform router discovery.
> 
> 1) Again, if we don't set disable_ipv6 then the kernel still assigns link-local
> addresses and thus you have link-local connectivity, even if the IPv6
> addressing method is DISABLED.  Which seems kinda wrong.

It's not wrong at all if you expand 'method' to 'global address (dynamic) configuration method' or 'NetworkManager's address configuration features' instead of 'configuration of all IPv6 things including the kernel sysctl'.

Also it's now it has worked for long time without much problem so I think it's rightful to question its urgency and leave it up to the method redesign which at least doesn't explicitly block the 0.9.10 release.

> 2) agreed, we don't need to set use_tempaddr at runtime, though we still should
> restore it on shutdown/unmanage.

Why to restore something we never alter?

> Since we need to implement IPv6 privacy
> internally in NM now that we're using libndp and not the kernel, we don't care
> about use_tempaddr on the kernel-side.
> 
> 3) do you mean avoid setting it to 0?

I meant that but it won't work, unfortunately. The IGNORE method is yet to be dealt with, as accept_ra is modified before connection activation. If we really want to provide such functionality, it should be per-device, not per-connection (like some other recently added features).

> 4) agreed here as well, and when set, NM should probably also filter out IPv6
> may-fail=FALSE connections (as in, they should not be candidates for
> autoconnect nor should they be in 'available-connections') since we know they
> cannot be completed successfully.

#4 more or less depends on #1, as if NM sets disable_ipv6 (i.e. uses it as output configuration), it doesn't make much sense to obey it (except configuration take over of course). Any ideas?

(In reply to comment #36)
> > Regarding the ipv6_disabled, I don't think it's a good idea to use 'method' for
> > [not] setting up kernel link-local IPv6, nor I see the alleged importance of
> > setting disable_ipv6 from NetworkManager instead of sysctl.conf.
> 
> Currently, if an NM-managed device has carrier, then it will have a link-local
> IPv6 address, even when NM considers the device to be disconnected. That means
> that any daemon listening on :: will be accessible to all other machines on
> your subnet.

Yes. This is in line with bug #683206 which is *very* useful and would be a good default behavior.

> This is potentially bad for security reasons,

Indeed that's up to a long discussion. But it's the sysctl default which makes it *more usable* on one hand, and *less secure* on the other. On my systems I prefer the *less secure* variant and I would use a firewall to limit the access.

I agree there are people with different tastes and opinions and it's hard to agree upon good defaults. That would mean *overriding* the default sysctl setting for security reasons but as a side affect you also override the administrator's explicit setting, causing a regression in link local networking.

But that also means my original thought that we should never touch disable_ipv6 is not right either! Because only using sysctl configuration gives us two possibilities:

1) Enable link-local on all connections *and* on disconnected devices
2) Enable link-local on no connections *neither* disconnected devices

But we probably also need:

3) Enable link-local on all connections *but* not on disconnected devices

This one would not be my preference but I can imagine requests for such behavior (indeed AFAIK this is what you would request).

That said, I changed my mind a bit. I still call for leaving the status quo (don't use 'disable_ipv6' at all) until the behavior is properly considered. But I changed my view on the desired behavior:

 * Use the original value found in sysctl's 'disable_ipv6' for the DISCONNECTED state so that I can explicitly turn on link-local networking when no specific network configuration is used. Possibly ask distribution maintainers to consider setting 'all.disable_ipv6' by default in '/etc/sysctl' to provide a more conservative default.
 * Use a separate connection option to fully disable IPv6 with that connection (or, temporarily, indeed provide a method=disconnected). Use disable_ipv6 for that.

Does that make sense? I'll have to check how much your patch differs from that. Also I think we should document the behavior very carefully, you see how much it took me to buy the idea of touching 'disable_ipv6' from NetworkManager at all.

> and in general a violation of the principle of least surprise.

It's a principle impossible to follow in this case. It's surprising both to lack link-local addresses and to communicate via 'disconnected' interfaces. But as written above, we can at least leave the choice upon the administrator.

> initscripts bug https://bugzilla.redhat.com/show_bug.cgi?id=496444 (later
> cloned to NM bug https://bugzilla.redhat.com/show_bug.cgi?id=982740) proposed
> making IPV6INIT=no mean "completely disabled", but the bug eventually got
> WONTFIXed because the provided patch sucked...
> 
> (Really, it seems to me like we ought to be setting default/ipv6_disable to 1,
> and then re-enabling it as needed on individual interfaces.

You mean in /etc/sysctl.conf? I'm all for it provided that I can simply change it there. Otherwise it would break feature described in bug #683206.

> That's the only way
> to solve the problem of the kernel autoconfiguring IPv6 on a newly-created
> interface before we even get a chance to see it...)

I'm reluctant to agree that NetworkManager should touch the sysctl 'default' (not to say 'all') though, for obvious reasons.

1) IGNORE method

I'm not comfortable changing IGNORE semantics at this time.  I'm happy to deprecate it, phase the value out, and transition people over to AUTO or DISABLED.  Just not to change how existing IGNORE connections have worked for years.  It's fine to change behavior for *new* connections.

With all that said, I don't think we can consolidate IGNORE into an existing method.  Neither AUTO nor LINK_LOCAL is quite correct

A solution might be to:

a) keep IGNORE a separate method
b) when activating IGNORE, disable_ipv6 + accept_ra + use_tempaddr get set to their pre-NetworkManager value
c) NM ignores any IPv6 events or addresses for the connection (though we probably should add them to the D-Bus NMIP6Config)
d) mark IGNORE as deprecated in libnm-util
e) continue defaulting IPv6 to "auto"
f) hide the IGNORE option in nm-connection-editor and nmcli unless the connection is already set to IGNORE, then show it


2) DISABLE method

I believe this should set the interface's disable_ipv6 as appropriate; we should save the value as appropriate and then set it when activating the connection based on the connetion's method.  I believe that per-connection disable/enable is the right thing here; sysctl doesn't work well because many hotplug devices (USB, PCIe, etc) won't have consistent device names if you plug into different ports.  Plus, it's not that hard to do the the per-connection disable/enable; all it takes is writing disable_ipv6 when activating the connection.

This would also imply doing the IPV6INIT=disabled vs. IPV6INIT=no thing in initscripts and ifcfg-rh.  But that should be a pretty simple change in Fedora initscripts.

3) IPv6LL when disconnected

Link-local behavior can be easily achieved using a simple IPv6 autoconnect connection.  Allowing network connectivity for an interface should be an *intentional* decision, whether that's by creating a saved IPv6 connection, or assigning a static address, or by setting accept_ra=1, or whatever.  When I run "nmcli dev disconnect eth0" I expect that device to be completely unreachable and deconfigured; that's what disconnected means.  I just don't see why a user who wants an IPv6 Link-Local address on their interface would not simply use a method=LINK_LOCAL + autoconnect connection; this achieves exactly the same semantics as a simple "IFF_UP" + disable_ipv6=0 when NetworkManager is not running.

I would love to set disconnected interfaces !IFF_UP, which is the real meaning of "disconnected" (eg administratively down), but unfortunately the kernel doesn't allow L2 operations in that state.

(In reply to comment #40)
> 1) IGNORE method
> 
> I'm not comfortable changing IGNORE semantics at this time.  I'm happy to
> deprecate it, phase the value out, and transition people over to AUTO or
> DISABLED.  Just not to change how existing IGNORE connections have worked for
> years.  It's fine to change behavior for *new* connections.
> 
> With all that said, I don't think we can consolidate IGNORE into an existing
> method.  Neither AUTO nor LINK_LOCAL is quite correct
> 
> A solution might be to:
> 
> a) keep IGNORE a separate method
> b) when activating IGNORE, disable_ipv6 + accept_ra + use_tempaddr get set to
> their pre-NetworkManager value

I have no problem with that.

> c) NM ignores any IPv6 events or addresses for the connection (though we
> probably should add them to the D-Bus NMIP6Config)

We can use libndp for that. We shouldn't run, though (it wasn't run in the past and it may have a side effect of reserving an address).

> d) mark IGNORE as deprecated in libnm-util

Sure.

> e) continue defaulting IPv6 to "auto"
> f) hide the IGNORE option in nm-connection-editor and nmcli unless the
> connection is already set to IGNORE, then show it

Sure.

> 2) DISABLE method
> 
> I believe this should set the interface's disable_ipv6 as appropriate;

Sure (changed my mind as already noted).

> we
> should save the value as appropriate and then set it when activating the
> connection based on the connetion's method.  I believe that per-connection
> disable/enable is the right thing here;

Sure.

> sysctl doesn't work well because many
> hotplug devices (USB, PCIe, etc) won't have consistent device names if you plug
> into different ports.

You shouldn't mix global, per-device and per-connection here. Otherwise, yes.

> Plus, it's not that hard to do the the per-connection
> disable/enable; all it takes is writing disable_ipv6 when activating the
> connection.

Sure, if we don't write per-device disable_ipv6 before a connection is marked for activation.

> This would also imply doing the IPV6INIT=disabled vs. IPV6INIT=no thing in
> initscripts and ifcfg-rh.  But that should be a pretty simple change in Fedora
> initscripts.

Sure.

> 3) IPv6LL when disconnected
> 
> Link-local behavior can be easily achieved using a simple IPv6 autoconnect
> connection.

That's not what I request in bug #683206. What I request is to *never* remove link-local addresses from wired devices unless explicitly requested by the administrator (e.g. using 'sysctl' before NetworkManager is started, or in NetworkManager connection configuration).

This is the core idea of zero configuration networking via link-local addresses. All I want is when zero configuration networking works before NetworkManager is started, that NetworkManager doesn't break it.

> Allowing network connectivity for an interface should be an
> *intentional* decision,

1) Plugging a cable into your computer is usually an intentional decision.

2) NetworkManager always provided automatic connectivity and it wasn't considered a problem.

3) NetworkManager shouldn't conflict with local zero configuration networking if the administrator wants to use it.

> whether that's by creating a saved IPv6 connection, or
> assigning a static address, or by setting accept_ra=1, or whatever.

Or setting disable_ipv6=0. Both are permissive by default and NetworkManager upstream should IMO respect their pre-existing values.

> When I run
> "nmcli dev disconnect eth0" I expect that device to be completely unreachable
> and deconfigured;

All I'm asking is to let the administrator decide and not force your expectations upon him and not kill future development of zerconf.

> that's what disconnected means.

That's not what NetworkManager does, though. It explicitly leaves the device up *because* it wants it to communicate on the link layer. All I want is to let the administrator choose whether the link layer also implies link-local IP connectivity.

> I just don't see why a user
> who wants an IPv6 Link-Local address on their interface would not simply use a
> method=LINK_LOCAL + autoconnect connection;

1) Because we talk about zero configuration networking. Apart from turning it on/off in the system, there shouldn't be any other configuration steps necessary.

2) Because the user doesn't expect zeroconf addresses to come and go. Starting/stopping the network daemon as well as starting/stopping network connections shouldn't interrupt any zeroconf communication.

> this achieves exactly the same semantics as a simple "IFF_UP"

There's a problem with timing.

In an ideal scenario, with zeroconf enabled on the machine, I expect the ethernet connectivity to be as reliable as a serial line. Just connect and communicate.

> + disable_ipv6=0 when NetworkManager is not running.

In order to be able to use zeroconf, the respective interfaces need never to be !IFF_UP and never to be disable_ipv6=1. This works on any system without NetworkManager as you simply IFF_UP the device at boot time (or upon udev event) and keep the global default disable_ipv6=0.

> I would love to set disconnected interfaces !IFF_UP, which is the real meaning
> of "disconnected" (eg administratively down), but unfortunately the kernel
> doesn't allow L2 operations in that state.

That's because the real meaning of "disconnected" is in conflict with "communicating" (on any level). I'm quite surprised that you expect to get the two at once.

(In reply to comment #41)
> > Allowing network connectivity for an interface should be an
> > *intentional* decision,
> 
> 1) Plugging a cable into your computer is usually an intentional decision.
> 
> 2) NetworkManager always provided automatic connectivity and it wasn't
> considered a problem.

Note that nothing changes for users who aren't using NetworkManager-config-server, because in that case we autocreate an AUTO connection for them anyway.

> 3) NetworkManager shouldn't conflict with local zero configuration networking
> if the administrator wants to use it.

If the administrator wants to have LL connectivity, and wants to use NetworkManager, then they should not configure NetworkManager in such a way that it would disable their LL connectivity.

or this may all be irrelevant...

sudo sysctl -w net.ipv6.conf.em1.disable_ipv6=1

(Plug in ethernet cable, wait, "ifconfig em1", note that there is no IPv6LL address. Smile. Unplug cable.)

sudo sysctl -w net.ipv6.conf.em1.disable_ipv6=0

(Plug in ethernet cable, wait, "ifconfig em1", note that there is *still* no IPv6LL address. Frown...)

It seems that after getting carrier with disable_ipv6=1 once, the interface gets stuck with IPv6 disabled regardless of the setting. (If you set disable_ipv6 and then unset it without connecting a cable in between, it works as expected.)

(In reply to comment #42)
> > 3) NetworkManager shouldn't conflict with local zero configuration networking
> > if the administrator wants to use it.
> 
> If the administrator wants to have LL connectivity, and wants to use
> NetworkManager, then they should not configure NetworkManager in such a way
> that it would disable their LL connectivity.

True but irrelevant. The whole comment is about breaking or not breaking zeroconf networking by NetworkManager without being explicitly instructed to do so.

(In reply to comment #43)
> It seems that after getting carrier with disable_ipv6=1 once, the interface
> gets stuck with IPv6 disabled regardless of the setting.

Sounds like a bug. Or missing functionality, at the least.

I created a table of methods where you can see some interesting attributes of the current system:

https://fedoraproject.org/wiki/Networking/Ideas/NetworkManagerMethods

For example it once again shows that turning off link-local addresses by adding a link-local method doesn't make real sense here, except as a temporary measure to live with methods for a longer while.

OK, I've pushed a branch that addresses this bug (and the rhbz "ipv6 routes don't get deleted" bug) but doesn't actually add a DISABLED method, which then avoids some of the tricky bits for now (and allows us to potentially solve all that in a better way in that other config methods bug)...

so the patches are now:

devices: remove dead ipv6 privacy code

  Removes the use_tempaddr code, which as Pavel pointed out, is
  useless now

core: fix default-unmanaged activation/deactivation

  Issue noticed while testing the next patch. Activating a
  default-unmanaged device claimed that it failed, even though it
  didn't, and deactivating it caused a crash, because the AC's
  device field was getting cleared because it thought the device
  was deactivating when really it was activating...

devices: "deactivate" devices when going from UNMANAGED->UNAVAILABLE

  Reverts a change I made (probably accidentally/incorrectly) with
  the default-unmanaged code. It seems to have no side effects

devices: fix multiple issues with accept_ra handling

  Cleanup, bugfixing

devices: use disable_ipv6 as appropriate

  And finally, the disabley stuff. As I said above, this doesn't add
  a DISABLED method, but it makes us use disable_ipv6 in the places
  where currently devices have IPv6 connectivity and we want them not
  to.

(In reply to comment #46)
> OK, I've pushed a branch that addresses this bug (and the rhbz "ipv6 routes
> don't get deleted" bug) but doesn't actually add a DISABLED method, which then
> avoids some of the tricky bits for now (and allows us to potentially solve all
> that in a better way in that other config methods bug)...
> 
> so the patches are now:
> 
> devices: remove dead ipv6 privacy code
> 
>   Removes the use_tempaddr code, which as Pavel pointed out, is
>   useless now

Thanks!

> core: fix default-unmanaged activation/deactivation
> 
>   Issue noticed while testing the next patch. Activating a
>   default-unmanaged device claimed that it failed, even though it
>   didn't, and deactivating it caused a crash, because the AC's
>   device field was getting cleared because it thought the device
>   was deactivating when really it was activating...

Looks good.

> devices: "deactivate" devices when going from UNMANAGED->UNAVAILABLE
> 
>   Reverts a change I made (probably accidentally/incorrectly) with
>   the default-unmanaged code. It seems to have no side effects

Good.

> devices: fix multiple issues with accept_ra handling
> 
>   Cleanup, bugfixing

Didn't check the code thoroughly, but should be good.

> devices: use disable_ipv6 as appropriate
> 
>   And finally, the disabley stuff. As I said above, this doesn't add
>   a DISABLED method, but it makes us use disable_ipv6 in the places
>   where currently devices have IPv6 connectivity and we want them not
>   to.

According to the commit message makes bug #683206 worse and will force me to maintain a patchset (of at least one patch) for NetworkManager to continue working as before. I always tried to keep NetworkManager patch-free so that I can use upstream/distribution NetworkManager without the patch and recompile hassle.

(In reply to comment #46)

Pavel said in comment #34

> But unless the patch is intended to go to the 0.9.8 branch, now when we always
> use userspace router discovery, there's IMO no need to ever set use_tempaddr,
> as it's suppressed by accept_ra=0 anyway. In git master, it would be IMO 
> better to ignore use_tempaddr altogether.

I think this is wrong. When we add temporary addresses from user-space, we still have to tell the kernel, whether to prefer public or private addresses as source address. So, we still must set use_tempaddr and the kernel must take it into account, even without accept_ra (I think, this might also need changes in the kernel, see rhbz #1021871)


> devices: remove dead ipv6 privacy code
> 
>   Removes the use_tempaddr code, which as Pavel pointed out, is
>   useless now

We will soon re-add ip6-privacy implemented in NetworkManager. This will have a conflict with th/ip6-privacy branch (rhbz #1003859) -- maybe we could move this patch over to th/ip6-privacy, unless it's getting merged to master soon.

(In reply to comment #46)
> OK, I've pushed a branch that addresses this bug (and the rhbz "ipv6 routes
> don't get deleted" bug) but doesn't actually add a DISABLED method, which then
> avoids some of the tricky bits for now (and allows us to potentially solve all
> that in a better way in that other config methods bug)...
> 
> so the patches are now:
> 
> devices: remove dead ipv6 privacy code
> 
>   Removes the use_tempaddr code, which as Pavel pointed out, is
>   useless now
> 
Yeah, Thomas is working on reimplementing the IPv6 privacy. What's the bug
number again?

> devices: use disable_ipv6 as appropriate
> 
>   And finally, the disabley stuff. As I said above, this doesn't add
>   a DISABLED method, but it makes us use disable_ipv6 in the places
>   where currently devices have IPv6 connectivity and we want them not
>   to.

I would name the variables
+    char *         ip6_disable_ipv6_path;
+    gint           ip6_disable_ipv6_save;
It is longer, but more explicit and clear what option they represent.

- /* If ip_iface did change, then any values we saved before is irrelevant. */
+ /* If ip_iface did change, then any values we saved before are irrelevant. */

(In reply to comment #48)
> (In reply to comment #46)
> Pavel said in comment #34
> 
> > But unless the patch is intended to go to the 0.9.8 branch, now when we always
> > use userspace router discovery, there's IMO no need to ever set use_tempaddr,
> > as it's suppressed by accept_ra=0 anyway. In git master, it would be IMO 
> > better to ignore use_tempaddr altogether.
> 
> I think this is wrong.

Really?

> When we add temporary addresses from user-space, we
> still have to tell the kernel, whether to prefer public or private addresses as
> source address.

Yep, the preference is important.

> So, we still must set use_tempaddr and the kernel must take it
> into account, even without accept_ra

Do you have any relevant sources to back up that sysctl's use_tmpaddr will work for addresses supplied from user space? Have you tested that? Otherwise it's useless.

> (I think, this might also need changes in the kernel, see rhbz #1021871)

That would indeed change the situation *when* NetworkManager requires a kernel *with* that feature. Do you have any contingency plan, here? Could you please describe it in bug #705170 which has been started as part of userspace router discovery development?

> > devices: remove dead ipv6 privacy code
> > 
> >   Removes the use_tempaddr code, which as Pavel pointed out, is
> >   useless now
> 
> We will soon re-add ip6-privacy implemented in NetworkManager.

Great! Could you please update bug #705170 with the new information?

> This will have a
> conflict with th/ip6-privacy branch (rhbz #1003859) -- maybe we could move this
> patch over to th/ip6-privacy, unless it's getting merged to master soon.

As bug #705170 doesn't carry any patches or other information, I don't think it's practical to delay this bug because of it. Also, you might have to handle the kernel feature dependency. Dan Winship's patchset is pretty much done except
the zeroconf breakage which doesn't block the other changes, though.

(In reply to comment #50)

> As bug #705170 doesn't carry any patches or other information, I don't think
> it's practical to delay this bug because of it. Also, you might have to handle
> the kernel feature dependency. Dan Winship's patchset is pretty much done
> except
> the zeroconf breakage which doesn't block the other changes, though.

Sure. Just get it done!! :-)

I updated bug #705170, let's discuss further question about this there.

(In reply to comment #48)
> I think this is wrong. When we add temporary addresses from user-space, we
> still have to tell the kernel, whether to prefer public or private addresses as
> source address. So, we still must set use_tempaddr

Ah, right. OK, removed that patch, and updated the "fix accept_ra" patch to deal with "use_tempaddr" too.

(In reply to comment #49)
> I would name the variables
> +    char *         ip6_disable_ipv6_path;
> +    gint           ip6_disable_ipv6_save;
> It is longer, but more explicit and clear what option they represent.

Yeah, I actually even accidentally typed them that way a few times while writing the patch. Fixed (and I renamed "ip6_privacy_tempaddr_*" to "ip6_use_tempaddr_*" too, so now all of them match their sysfs names).

re-pushed

pokety poke poke poke

You can drop "core: fix default-unmanaged activation/deactivation" since that actually got added as part of pending2xxx, though to nm-active-connection.c instead.  You'll run into that when you rebase.

Right, rebased and pushed without that patch

> devices: use disable_ipv6 as appropriate
-		/* Restore IP6 properties to their original values. */
-		restore_original_ip6_properties (self);
+		/* Restore disable_ipv6/accept_ra to their original values, unless this
+		 * is a slave connection, in which case we leave disable_ipv6 set.
+		 */
+		if (!priv->master)
+			restore_original_ip6_properties (self);

I expect that we restore all the IPv6 sysctl values for master devices, and none for slaves. So the comment should be adjusted.
And using 'is_master' would be better IMO:
		if (priv->is_master)
			restore_original_ip6_properties (self);

Other then that, the branch seems right to me.

Note that take_down/bring_up are run for slaves before and after adding them to a bond, do we really want to restore the properties for a very short period of time before removing them again?  What case was it required in take_down() that it wasn't done somewhere else, like when quitting?

One thing, that does not seem right to me is that we save_original_ip6_properties() only in constructed(), but we restore several times.

A device instance potentially lives a long time, so when NM starts it will fetch the properties. It will not update them and will repeatedly reset the old values, even if the user in the meantime reset them by hand.

Maybe it's better to save them every time, before we bring up the device and in restore to set priv->ip6_*_save to -1, so that we only restore once (unless we save again in the meantime).

Created attachment 259552 [details] [review]
[PATCH] core: ensure that ifname of the device is valid

We use the ifname/ip_ifname field to construct the pathname without checking. Add some checks, to ensure, that this does not fail due to invalid/malicious contents of ifname.

(In reply to comment #58)
> Maybe it's better to save them every time, before we bring up the device and in
> restore to set priv->ip6_*_save to -1, so that we only restore once (unless we
> save again in the meantime).

+1

(In reply to comment #58)
> Maybe it's better to save them every time, before we bring up the device and in
> restore to set priv->ip6_*_save to -1, so that we only restore once (unless we
> save again in the meantime).

Maybe, yes, but that would be a separate bug. (In fact, it's basically bug 708820.) This patch preserves the current behavior. (Or at least, it preserves the intent of the current behavior, while fixing a few cases.)

(In reply to comment #61)

I don't agree (beside that bug 708820 focuses on the MAC address instead).

Resetting the properties every time to a value that was recorded when the device got created, is IMO wrong behaviour, even if that was the original intent.


Thinking about it again..., whenever we decide to set one of the 3 flags, we should for each of them:

void set_ip6_disable_ipv6(self, value) {
  if (priv->ip6_disable_ipv6_save == -1) {
     priv->ip6_disable_ipv6_save = 
       nm_utils_get_proc_sys_net_value_with_bounds(priv->ip6_disable_ipv6_path);
  }
  nm_utils_do_sysctl (priv->ip6_disable_ipv6_path, value);
}

Instead of directly calling 
  nm_utils_do_sysctl (priv->ip6_disable_ipv6_path, value);



restore_original_ip6_properties() should do the same as now, but reset the priv->ip6_*_save fields to -1 every time.

save_original_ip6_properties() can be removed.

(In reply to comment #62)
> Resetting the properties every time to a value that was recorded when the
> device got created, is IMO wrong behaviour

It's not really "every time" though. In the normal case, the properties will be saved when NM starts up, and restored when NM exits. In particular, note that the properties are not restored just because a connection is disconnected; NM needs to have completely control over the property values at all times when a device is managed, because otherwise the kernel could do autoconf when NM (and the user) considered the device "disconnected".

The exception to this is for IGNORE connections, where we restore the startup-time properties when activating and then blow them away when deactivating... You could perhaps argue that we should re-save their current values when deactivating, but IGNORE is really only intended/useful for people who are mostly agnostic towards IPv6, so they shouldn't have fiddled with the properties anyway. If you want to fiddle with the ipv6 properties on your own, then you need to make the device unmanaged. (Although... by that argument, we shouldn't be restoring the pre-NM values when we activate the connection; we should be restoring the *kernel default* values. And you could make the same argument for shutdown; why should NM reset the properties to the state they were when it started? It doesn't do that for anything else. It could instead only guarantee that it would leave them in a "reasonable" state; ie, the kernel defaults. In this case then, we would never save or restore the values; we'd just always forcibly set them to the "right" values whenever they were wrong...)

All that said, it seems like if we're going to restore their values on shutdown, that we should restore them if the device becomes unmanaged as well... so the current patch moves the save_ip6_properties() call to happen when the device becomes managed (rather than construction time), and calls restore_ip6_properties() if the device is unmanaged.

Though I'm starting to think that maybe the "never save/restore" strategy above may actually be correct...

(In reply to comment #57)
> Note that take_down/bring_up are run for slaves before and after adding them to
> a bond, do we really want to restore the properties for a very short period of
> time before removing them again?  What case was it required in take_down() that
> it wasn't done somewhere else, like when quitting?

Ah, I forgot that we sometimes take_down devices that are in use. There was no case where we were failing to restore them, it's just that it seemed like a simplification to do it all in one place. But I guess it's better not to. Fixed.


Re-pushed... The first two patches are unchanged, the third and fourth are mostly unchanged other than the fact that they were a single patch before and are now two.

(In reply to comment #63)
> ...
> Though I'm starting to think that maybe the "never save/restore" strategy above
> may actually be correct...

Yes, it will cause problems if another application interferes with NM managed interfaces. So, in the expected case, only NM manages all properties and both approaches are identical (because as often as we re-read the properties, they do not change).

It's only interesting when the user actually changes it. Then we should make the best possible guess, what to do (and more often then not, it will be wrong).

Despite all arguments :) , I still think its best to re-read the fresh values shortly before we decide to manage a device and reset those values, when it becomes unmanaged again.

(In reply to comment #64)

I admit, re-setting to default values has also a certain appeal...

+ 0.5

review+; seems to work correctly.

ok, pushed as-is; we can consider not-saving-ip6-properties again later if we want