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

Message ID 20171114132132.25833-1-nicolas.dichtel@6wind.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net,v2] ipv6: set all.accept_dad to 0 by default
Related show

Commit Message

Nicolas Dichtel Nov. 14, 2017, 1:21 p.m.
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(-)

Comments

Stefano Brivio Nov. 14, 2017, 1:53 p.m. | #1
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>
Girish Moodalbail Nov. 14, 2017, 6:30 p.m. | #2
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
Stefano Brivio Nov. 14, 2017, 7:10 p.m. | #3
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.
Girish Moodalbail Nov. 14, 2017, 8:52 p.m. | #4
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
Nicolas Dichtel Nov. 15, 2017, 10:17 a.m. | #5
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
David Miller Nov. 15, 2017, 10:25 a.m. | #6
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.
Nicolas Dichtel Nov. 15, 2017, 10:49 a.m. | #7
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

Patch

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		= {