| Message ID | 20171114132132.25833-1-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] ipv6: set all.accept_dad to 0 by default | expand |
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>
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
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.
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
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
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.
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
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 = {
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(-)