| Message ID | 20171113134536.15382-1-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: set all.accept_dad to 0 by default | expand |
Should we consider rolling back the patch that caused this? "accept_dad = 1" is the proper IETF-expected default behaviour. Alternatively, if we really want to make all, default, and ifname useful perhaps we need to investigate a tristate option (for currently boolean values, at least). -1 could mean no preference, for example. On 13 November 2017 at 13:45, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > The commit a2d3f3e33853 modifies the way to disable dad on an interface. > Before the patch, setting <iface>.accept_dad to 0 was enough to disable it. > Because all.accept_dad is set to 1 by default, after the patch, the user > needs to set both all.accept_dad and <iface>.accept_dad to 0 to disable it. > > This is not backward compatible. When a user updates its kernel, the dad > may be enabled by error. > > Let's set all.accept_dad to 0 by default to restore the previous behavior. > > Fixes: a2d3f3e33853 ("ipv6: fix net.ipv6.conf.all.accept_dad behaviour for real") > CC: Stefano Brivio <sbrivio@redhat.com> > CC: Matteo Croce <mcroce@redhat.com> > CC: Erik Kline <ek@google.com> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > net/ipv6/addrconf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 8a1c846d3df9..ef5b61507b9a 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -231,7 +231,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = { > .proxy_ndp = 0, > .accept_source_route = 0, /* we do not accept RH0 by default. */ > .disable_ipv6 = 0, > - .accept_dad = 1, > + .accept_dad = 0, > .suppress_frag_ndisc = 1, > .accept_ra_mtu = 1, > .stable_secret = { > -- > 2.13.2 >
On Mon, Nov 13, 2017 at 3:21 PM, Erik Kline <ek@google.com> wrote: > Should we consider rolling back the patch that caused this? > "accept_dad = 1" is the proper IETF-expected default behaviour. > > Alternatively, if we really want to make all, default, and ifname > useful perhaps we need to investigate a tristate option (for currently > boolean values, at least). -1 could mean no preference, for example. > > On 13 November 2017 at 13:45, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: >> The commit a2d3f3e33853 modifies the way to disable dad on an interface. >> Before the patch, setting <iface>.accept_dad to 0 was enough to disable it. >> Because all.accept_dad is set to 1 by default, after the patch, the user >> needs to set both all.accept_dad and <iface>.accept_dad to 0 to disable it. >> >> This is not backward compatible. When a user updates its kernel, the dad >> may be enabled by error. >> >> Let's set all.accept_dad to 0 by default to restore the previous behavior. >> >> Fixes: a2d3f3e33853 ("ipv6: fix net.ipv6.conf.all.accept_dad behaviour for real") >> CC: Stefano Brivio <sbrivio@redhat.com> >> CC: Matteo Croce <mcroce@redhat.com> >> CC: Erik Kline <ek@google.com> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> --- >> net/ipv6/addrconf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 8a1c846d3df9..ef5b61507b9a 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -231,7 +231,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = { >> .proxy_ndp = 0, >> .accept_source_route = 0, /* we do not accept RH0 by default. */ >> .disable_ipv6 = 0, >> - .accept_dad = 1, >> + .accept_dad = 0, >> .suppress_frag_ndisc = 1, >> .accept_ra_mtu = 1, >> .stable_secret = { >> -- >> 2.13.2 >> Another way could be putting the "all" and per interface handlers in logical AND. The fact is before the patch, the "all" handler really was a noop. What do you think?
On Mon, 13 Nov 2017 14:21:52 +0000 Erik Kline <ek@google.com> wrote: > Should we consider rolling back the patch that caused this? > "accept_dad = 1" is the proper IETF-expected default behaviour. > > Alternatively, if we really want to make all, default, and ifname > useful perhaps we need to investigate a tristate option (for currently > boolean values, at least). -1 could mean no preference, for example. I haven't checked how ugly it would be, yet. But another way to restore the previous behaviour, while keeping the new functionality, would be to keep the global default as 1 and instead set the per-interface accept_dad default value to 0. What do you think?
>> Should we consider rolling back the patch that caused this? >> "accept_dad = 1" is the proper IETF-expected default behaviour. >> >> Alternatively, if we really want to make all, default, and ifname >> useful perhaps we need to investigate a tristate option (for currently >> boolean values, at least). -1 could mean no preference, for example. > > I haven't checked how ugly it would be, yet. But another way to restore > the previous behaviour, while keeping the new functionality, would be > to keep the global default as 1 and instead set the per-interface > accept_dad default value to 0. What do you think? The default out-of-the-box behaviour should definitely be to do DAD. You can achieve this in 4 ways: [A] all=1, default=1, AND --> the OLD pre-patch behaviour [B] all=1, default=1, OR --> the NEW post-patch behaviour - problematic [C] all=1, default=0, OR --> problematic for same reason: iface=0 is a no-op [D] all=0, default=1, OR Note that: AND == (all < 1 || interface < 1) OR == (all < 1 && interface < 1) [C] requires one to set all but one interface (incl. default) to 1, then set all=0, just to disable a single interface's dad [D] is weird, because with the default already being dad enabled, there's really no reason to ever set all=1 Being able to disable either for all interfaces (via all=0) or for a specific interface (via iface=0) seems the most useful. Setting all=1, default=0, specific_interfaces=1, AND-logic also seems useful. Hence my vote to rollback a2d3f3e33853.
On Mon, 13 Nov 2017 23:52:26 +0900 Maciej Żenczykowski <maze@google.com> wrote: > >> Should we consider rolling back the patch that caused this? > >> "accept_dad = 1" is the proper IETF-expected default behaviour. > >> > >> Alternatively, if we really want to make all, default, and ifname > >> useful perhaps we need to investigate a tristate option (for currently > >> boolean values, at least). -1 could mean no preference, for example. > > > > I haven't checked how ugly it would be, yet. But another way to restore > > the previous behaviour, while keeping the new functionality, would be > > to keep the global default as 1 and instead set the per-interface > > accept_dad default value to 0. What do you think? > > The default out-of-the-box behaviour should definitely be to do DAD. > > You can achieve this in 4 ways: > > [A] all=1, default=1, AND --> the OLD pre-patch behaviour Old pre-patch behaviour simply ignored the 'all' value though. > [B] all=1, default=1, OR --> the NEW post-patch behaviour - problematic > [C] all=1, default=0, OR --> problematic for same reason: iface=0 is a no-op But this way you could still globally disable DAD, starting from default values, by simply setting 'all' to zero, which is what Nicolas wanted. > [D] all=0, default=1, OR > > Note that: > AND == (all < 1 || interface < 1) > OR == (all < 1 && interface < 1) > > [C] requires one to set all but one interface (incl. default) to 1, > then set all=0, > just to disable a single interface's dad > > [D] is weird, because with the default already being dad enabled, there's really > no reason to ever set all=1 > > Being able to disable either for all interfaces (via all=0) or for a > specific interface (via iface=0) seems > the most useful. > > Setting all=1, default=0, specific_interfaces=1, AND-logic also seems useful. > > Hence my vote to rollback a2d3f3e33853. We're mostly talking about 35e015e1f577 here.
Le 13/11/2017 à 16:05, Stefano Brivio a écrit : > On Mon, 13 Nov 2017 23:52:26 +0900 > Maciej Żenczykowski <maze@google.com> wrote: > >>>> Should we consider rolling back the patch that caused this? >>>> "accept_dad = 1" is the proper IETF-expected default behaviour. >>>> >>>> Alternatively, if we really want to make all, default, and ifname >>>> useful perhaps we need to investigate a tristate option (for currently >>>> boolean values, at least). -1 could mean no preference, for example. >>> >>> I haven't checked how ugly it would be, yet. But another way to restore >>> the previous behaviour, while keeping the new functionality, would be >>> to keep the global default as 1 and instead set the per-interface >>> accept_dad default value to 0. What do you think? >> >> The default out-of-the-box behaviour should definitely be to do DAD. Yes, and my patch didn't modify this. >> >> You can achieve this in 4 ways: >> >> [A] all=1, default=1, AND --> the OLD pre-patch behaviour > > Old pre-patch behaviour simply ignored the 'all' value though. > >> [B] all=1, default=1, OR --> the NEW post-patch behaviour - problematic >> [C] all=1, default=0, OR --> problematic for same reason: iface=0 is a no-op > > But this way you could still globally disable DAD, starting from > default values, by simply setting 'all' to zero, which is what Nicolas > wanted. > >> [D] all=0, default=1, OR >> >> Note that: >> AND == (all < 1 || interface < 1) >> OR == (all < 1 && interface < 1) >> >> [C] requires one to set all but one interface (incl. default) to 1, >> then set all=0, >> just to disable a single interface's dad >> >> [D] is weird, because with the default already being dad enabled, there's really >> no reason to ever set all=1 >> >> Being able to disable either for all interfaces (via all=0) or for a >> specific interface (via iface=0) seems >> the most useful. >> >> Setting all=1, default=0, specific_interfaces=1, AND-logic also seems useful. >> >> Hence my vote to rollback a2d3f3e33853. > > We're mostly talking about 35e015e1f577 here. > I don't have a strong opinion about what to do. My reasoning was that before patch 35e015e1f577, all.accept_dad had no effect, thus I took the assumption that users didn't modify it, but only default.accept_dad and <iface>.accept_dad. With this assumption, my patch helps to keep the same settings when upgrading the kernel. Nicolas
On Mon, 13 Nov 2017 14:45:36 +0100 Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > The commit a2d3f3e33853 modifies the way to disable dad on an interface. > Before the patch, setting <iface>.accept_dad to 0 was enough to disable it. > Because all.accept_dad is set to 1 by default, after the patch, the user > needs to set both all.accept_dad and <iface>.accept_dad to 0 to disable it. Perhaps it would make sense to be a bit more descriptive here, this seems to have generated quite some confusion. Besides, a2d3f3e33853 was just a fix-up for 35e015e1f577, which is instead the change which actually changed the behaviour. What about: --- With commit 35e015e1f577 ("ipv6: fix net.ipv6.conf.all interface DAD handlers"), the global 'accept_dad' flag is also taken into account and set to 1 by default. If either global or per-interface flag is non-zero, DAD will be enabled on a given interface. This is not backward compatible: before 35e015e1f577, the user could disable DAD just by setting the per-interface flag to 0. Now, the user instead needs to set both flags to 0 to actually disable DAD. Restore the previous behaviour by setting the default for the global 'accept_dad' flag to 0. This way, DAD is still enabled by default, as per-interface flags are set to 1 on device creation, but setting them to 0 is enough to disable DAD on a given interface. - Before 35e015e1f577: global per-interface DAD enabled [default] 1 1 yes X 0 no X 1 yes - After 35e015e1f577: global per-interface DAD enabled 0 0 no 0 1 yes 1 0 yes [default] 1 1 yes - After this fix: global per-interface DAD enabled 0 0 no [default] 0 1 yes 1 0 yes 1 1 yes --- > Fixes: a2d3f3e33853 ("ipv6: fix net.ipv6.conf.all.accept_dad behaviour for real") And I'd rather say: Fixes: 35e015e1f577 ("ipv6: fix net.ipv6.conf.all interface DAD handlers") > CC: Stefano Brivio <sbrivio@redhat.com> > CC: Matteo Croce <mcroce@redhat.com> > CC: Erik Kline <ek@google.com> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> With a more descriptive commit message and the appropriate Fixes: reference, FWIW, Acked-by: Stefano Brivio <sbrivio@redhat.com>
On Mon, 13 Nov 2017 14:21:52 +0000 Erik Kline <ek@google.com> wrote: > Alternatively, if we really want to make all, default, and ifname > useful perhaps we need to investigate a tristate option (for currently > boolean values, at least). -1 could mean no preference, for example. I think this would make sense in general, but on the other hand it would be quite a big change, and Nicolas' patch has the advantages of being small, keeping the global flag functional, and restoring the previous default behaviour out of the box when 'accept_dad' is disabled by the user for a given interface. Besides, this still wouldn't solve the case where flags are >= 0 and conflicting -- there, it's still debatable whether we want a logical OR or a logical AND. In the end, I would prefer either Nicolas' patch, or to get rid of the global 'accept_dad' flag altogether. Having a flag which does absolutely nothing, which was the case before 35e015e1f577, doesn't sound correct by any means.
Le 14/11/2017 à 03:24, Stefano Brivio a écrit : > On Mon, 13 Nov 2017 14:45:36 +0100 > Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > >> The commit a2d3f3e33853 modifies the way to disable dad on an interface. >> Before the patch, setting <iface>.accept_dad to 0 was enough to disable it. >> Because all.accept_dad is set to 1 by default, after the patch, the user >> needs to set both all.accept_dad and <iface>.accept_dad to 0 to disable it. > > Perhaps it would make sense to be a bit more descriptive here, this > seems to have generated quite some confusion. Yes, I agree. I will send a v2. Thank you, Nicolas
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 8a1c846d3df9..ef5b61507b9a 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -231,7 +231,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = { .proxy_ndp = 0, .accept_source_route = 0, /* we do not accept RH0 by default. */ .disable_ipv6 = 0, - .accept_dad = 1, + .accept_dad = 0, .suppress_frag_ndisc = 1, .accept_ra_mtu = 1, .stable_secret = {
The commit a2d3f3e33853 modifies the way to disable dad on an interface. Before the patch, setting <iface>.accept_dad to 0 was enough to disable it. Because all.accept_dad is set to 1 by default, after the patch, the user needs to set both all.accept_dad and <iface>.accept_dad to 0 to disable it. This is not backward compatible. When a user updates its kernel, the dad may be enabled by error. Let's set all.accept_dad to 0 by default to restore the previous behavior. Fixes: a2d3f3e33853 ("ipv6: fix net.ipv6.conf.all.accept_dad behaviour for real") CC: Stefano Brivio <sbrivio@redhat.com> CC: Matteo Croce <mcroce@redhat.com> CC: Erik Kline <ek@google.com> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/ipv6/addrconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)