[PATCH net] ipv6: set all.accept_dad to 0 by default

[PATCH net] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

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

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Erik Kline]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4835 bytes --]

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Matteo Croce]

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?

-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

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?

-- 
Stefano

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Maciej Żenczykowski]

>> 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.

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

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.

-- 
Stefano

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

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

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

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.

-- 
Stefano

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

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>

-- 
Stefano

Re: [PATCH net] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

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

[PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
is also taken into account (default value is 1). If either global or
per-interface flag is non-zero, DAD will be enabled on a given interface.

This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
          global    per-interface    DAD enabled
[default]   1             1              yes
            X             0              no
            X             1              yes

- After 35e015e1f577 and a2d3f3e33853:
          global    per-interface    DAD enabled
[default]   1             1              yes
            0             0              no
            0             1              yes
            1             0              yes

- After this fix:
          global    per-interface    DAD enabled
            1             1              yes
            0             0              no
[default]   0             1              yes
            1             0              yes

Fixes: 35e015e1f577 ("ipv6: fix net.ipv6.conf.all interface DAD handlers")
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>
---

v1 -> v2:
  reword the commitlog

Stefano, I've kept both 'Fixes' lines because technically, the behavior has
changed with a2d3f3e33853, not with 35e015e1f577.

 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.15.0

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

On Tue, 14 Nov 2017 14:21:32 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
> is also taken into account (default value is 1). If either global or
> per-interface flag is non-zero, DAD will be enabled on a given interface.
> 
> This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
>           global    per-interface    DAD enabled
> [default]   1             1              yes
>             X             0              no
>             X             1              yes
> 
> - After 35e015e1f577 and a2d3f3e33853:
>           global    per-interface    DAD enabled
> [default]   1             1              yes
>             0             0              no
>             0             1              yes
>             1             0              yes
> 
> - After this fix:
>           global    per-interface    DAD enabled
>             1             1              yes
>             0             0              no
> [default]   0             1              yes
>             1             0              yes
> 
> Fixes: 35e015e1f577 ("ipv6: fix net.ipv6.conf.all interface DAD handlers")
> 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>

Acked-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Girish Moodalbail]

On 11/14/17 5:21 AM, Nicolas Dichtel wrote:
> With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
> is also taken into account (default value is 1). If either global or
> per-interface flag is non-zero, DAD will be enabled on a given interface.
> 
> This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
>            global    per-interface    DAD enabled
> [default]   1             1              yes
>              X             0              no
>              X             1              yes
> 
> - After 35e015e1f577 and a2d3f3e33853:
>            global    per-interface    DAD enabled
> [default]   1             1              yes
>              0             0              no
>              0             1              yes
>              1             0              yes
> 
> - After this fix:
>            global    per-interface    DAD enabled
>              1             1              yes
>              0             0              no
> [default]   0             1              yes
>              1             0              yes

Above table can be summarized to..

- After this fix:
           global    per-interface    DAD enabled
             1             X              yes
             0             0              no
[default]   0             1              yes

So, if global is set to '1', then irrespective of what the per-interface value 
is DAD will be enabled. Is it not confusing. Shouldn't the more specific value 
override the general value?

On the other hand, if the global is set to '0', then per-interface value will be 
honored (overrides global). So, the meaning of global varies based on its value. 
Isn't that confusing as well.

thanks,
~Girish

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Stefano Brivio]

On Tue, 14 Nov 2017 10:30:33 -0800
Girish Moodalbail <girish.moodalbail@oracle.com> wrote:

> On 11/14/17 5:21 AM, Nicolas Dichtel wrote:
> > With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
> > is also taken into account (default value is 1). If either global or
> > per-interface flag is non-zero, DAD will be enabled on a given interface.
> > 
> > This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
> >            global    per-interface    DAD enabled
> > [default]   1             1              yes
> >              X             0              no
> >              X             1              yes
> > 
> > - After 35e015e1f577 and a2d3f3e33853:
> >            global    per-interface    DAD enabled
> > [default]   1             1              yes
> >              0             0              no
> >              0             1              yes
> >              1             0              yes
> > 
> > - After this fix:
> >            global    per-interface    DAD enabled
> >              1             1              yes
> >              0             0              no
> > [default]   0             1              yes
> >              1             0              yes  
> 
> Above table can be summarized to..
> 
> - After this fix:
>            global    per-interface    DAD enabled
>              1             X              yes
>              0             0              no
> [default]   0             1              yes
> 
> So, if global is set to '1', then irrespective of what the per-interface value 
> is DAD will be enabled. Is it not confusing. Shouldn't the more specific value 
> override the general value?

Might be a bit confusing, yes, but in order to implement an overriding
mechanism you would need to implement a tristate option as Eric K.
proposed. That is, by default you would have -1 (meaning "don't care")
on per-interface flags, and if this value is changed then the
per-interface value wins over the global one.

Sensible, but I think it's outside of the scope of this patch, which is
just intended to restore a specific pre-existing userspace expectation.

> On the other hand, if the global is set to '0', then per-interface value will be 
> honored (overrides global). So, the meaning of global varies based on its value. 
> Isn't that confusing as well.

I don't find this confusing though. Setting the global flag always has
the meaning of "force enabling DAD on all interfaces".

You would have the same problem if you chose a logical AND between
global and per-interface flag. There, setting the global flag would mean
"force disabling DAD on all interfaces".

So the only indisputable improvement I see here would be to implement a
"don't care" value (either for global or for per-interface flags). But
I'd rather agree with Nicolas that we should fix a potentially broken
userspace assumption first.

-- 
Stefano

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Girish Moodalbail]

On 11/14/17 11:10 AM, Stefano Brivio wrote:
> On Tue, 14 Nov 2017 10:30:33 -0800
> Girish Moodalbail <girish.moodalbail@oracle.com> wrote:
> 
>> On 11/14/17 5:21 AM, Nicolas Dichtel wrote:
>>> With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
>>> is also taken into account (default value is 1). If either global or
>>> per-interface flag is non-zero, DAD will be enabled on a given interface.
>>>
>>> This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
>>>             global    per-interface    DAD enabled
>>> [default]   1             1              yes
>>>               X             0              no
>>>               X             1              yes
>>>
>>> - After 35e015e1f577 and a2d3f3e33853:
>>>             global    per-interface    DAD enabled
>>> [default]   1             1              yes
>>>               0             0              no
>>>               0             1              yes
>>>               1             0              yes
>>>
>>> - After this fix:
>>>             global    per-interface    DAD enabled
>>>               1             1              yes
>>>               0             0              no
>>> [default]   0             1              yes
>>>               1             0              yes
>>
>> Above table can be summarized to..
>>
>> - After this fix:
>>             global    per-interface    DAD enabled
>>               1             X              yes
>>               0             0              no
>> [default]   0             1              yes
>>
>> So, if global is set to '1', then irrespective of what the per-interface value
>> is DAD will be enabled. Is it not confusing. Shouldn't the more specific value
>> override the general value?
> 
> Might be a bit confusing, yes, but in order to implement an overriding
> mechanism you would need to implement a tristate option as Eric K.
> proposed. That is, by default you would have -1 (meaning "don't care")
> on per-interface flags, and if this value is changed then the
> per-interface value wins over the global one.
> 
> Sensible, but I think it's outside of the scope of this patch, which is
> just intended to restore a specific pre-existing userspace expectation.
> 
>> On the other hand, if the global is set to '0', then per-interface value will be
>> honored (overrides global). So, the meaning of global varies based on its value.
>> Isn't that confusing as well.
> 
> I don't find this confusing though. Setting the global flag always has
> the meaning of "force enabling DAD on all interfaces".
> 
> You would have the same problem if you chose a logical AND between
> global and per-interface flag. There, setting the global flag would mean
> "force disabling DAD on all interfaces".
> 
> So the only indisputable improvement I see here would be to implement a
> "don't care" value (either for global or for per-interface flags). But
> I'd rather agree with Nicolas that we should fix a potentially broken
> userspace assumption first.

Agree.

Thanks,
~Girish

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

Le 14/11/2017 à 14:21, Nicolas Dichtel a écrit :
> With commits 35e015e1f577 and a2d3f3e33853, the global 'accept_dad' flag
> is also taken into account (default value is 1). If either global or
> per-interface flag is non-zero, DAD will be enabled on a given interface.
> 
> This is not backward compatible: before those patches, 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 35e015e1f57a7 and a2d3f3e33853:
>           global    per-interface    DAD enabled
> [default]   1             1              yes
>             X             0              no
>             X             1              yes
> 
> - After 35e015e1f577 and a2d3f3e33853:
>           global    per-interface    DAD enabled
> [default]   1             1              yes
>             0             0              no
>             0             1              yes
>             1             0              yes
> 
> - After this fix:
>           global    per-interface    DAD enabled
>             1             1              yes
>             0             0              no
> [default]   0             1              yes
>             1             0              yes
> 
> Fixes: 35e015e1f577 ("ipv6: fix net.ipv6.conf.all interface DAD handlers")
> 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>
David,

I saw that you pushed this patch in net-next instead of net. Is it intentional?
I was expecting to see it in net, to fix the 4.14.


Thank you,
Nicolas

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 15 Nov 2017 11:17:28 +0100

> I saw that you pushed this patch in net-next instead of net. Is it intentional?
> I was expecting to see it in net, to fix the 4.14.

All changes are going into net-next as I prepare to send a pull request
to Linus for the merge window.

This is always how things happen.

Re: [PATCH net v2] ipv6: set all.accept_dad to 0 by default

[Nicolas Dichtel]

Le 15/11/2017 à 11:25, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Wed, 15 Nov 2017 11:17:28 +0100
> 
>> I saw that you pushed this patch in net-next instead of net. Is it intentional?
>> I was expecting to see it in net, to fix the 4.14.
> 
> All changes are going into net-next as I prepare to send a pull request
> to Linus for the merge window.
> 
> This is always how things happen.
> 
Oh ok, thanks for the explanation.
It's the first time I submit a patch just before the pull request of net-next ;-)

Nicolas