[PATCH net] net/ipv6: allow token to be set when accept_ra disabled

[PATCH net] net/ipv6: allow token to be set when accept_ra disabled

[Hangbin Liu]

The token setting should not depend on whether accept_ra is enabled or
disabled. The user could set the token at any time. Enable or disable
accept_ra only affects when the token address take effective.

On the other hand, we didn't remove the token setting when disable
accept_ra. So let's just remove the accept_ra checking when user want
to set token address.

Fixes: f53adae4eae5 ("net: ipv6: add tokenized interface identifier support")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/addrconf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 24e319dfb510..4e63330f63e5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5689,8 +5689,6 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
 		return -EINVAL;
 	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
 		return -EINVAL;
-	if (!ipv6_accept_ra(idev))
-		return -EINVAL;
 	if (idev->cnf.rtr_solicits == 0)
 		return -EINVAL;
 
-- 
2.19.2

Re: [PATCH net] net/ipv6: allow token to be set when accept_ra disabled

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Thu,  9 Apr 2020 14:56:04 +0800

> The token setting should not depend on whether accept_ra is enabled or
> disabled. The user could set the token at any time. Enable or disable
> accept_ra only affects when the token address take effective.
> 
> On the other hand, we didn't remove the token setting when disable
> accept_ra. So let's just remove the accept_ra checking when user want
> to set token address.
> 
> Fixes: f53adae4eae5 ("net: ipv6: add tokenized interface identifier support")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

It is dangerous to change this, because now people can write bootup
and configuration scripts that will work with newer kernels yet fail
unexpectedly in older kernels.

I think requiring that RA be enabled in order to set the token is
an absolutely reasonable requirement.

Re: [PATCH net] net/ipv6: allow token to be set when accept_ra disabled

[Hangbin Liu]

On Thu, Apr 09, 2020 at 10:13:55AM -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Thu,  9 Apr 2020 14:56:04 +0800
> 
> > The token setting should not depend on whether accept_ra is enabled or
> > disabled. The user could set the token at any time. Enable or disable
> > accept_ra only affects when the token address take effective.
> > 
> > On the other hand, we didn't remove the token setting when disable
> > accept_ra. So let's just remove the accept_ra checking when user want
> > to set token address.
> > 
> > Fixes: f53adae4eae5 ("net: ipv6: add tokenized interface identifier support")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> It is dangerous to change this, because now people can write bootup
> and configuration scripts that will work with newer kernels yet fail
> unexpectedly in older kernels.

Hmm, this makes sense to me. Thanks for the explanation.

Regards
Hangbin

> 
> I think requiring that RA be enabled in order to set the token is
> an absolutely reasonable requirement.

Re: [PATCH net] net/ipv6: allow token to be set when accept_ra disabled

[Thomas Haller]

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

On Thu, 2020-04-09 at 10:13 -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Thu,  9 Apr 2020 14:56:04 +0800
> 
> > The token setting should not depend on whether accept_ra is enabled
> or
> > disabled. The user could set the token at any time. Enable or
> disable
> > accept_ra only affects when the token address take effective.
> > 
> > On the other hand, we didn't remove the token setting when disable
> > accept_ra. So let's just remove the accept_ra checking when user
> want
> > to set token address.
> > 
> > Fixes: f53adae4eae5 ("net: ipv6: add tokenized interface identifier
> support")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 

Hi,

I don't agree.


> It is dangerous to change this, because now people can write bootup
> and configuration scripts that will work with newer kernels yet fail
> unexpectedly in older kernels.

This concern is a very strict interpretation of "forward
compatibility".

The patch relaxes a check. Every change that enables something that
wasn't possible before has this danger. It seems acceptable that people
cannot use newer kernel features on kernel that don't support it. 


> I think requiring that RA be enabled in order to set the token is
> an absolutely reasonable requirement.


That seems to be the real problem: "Why would you posibly want to do
this?"

1)

NetworkManager sets accept_ra=0, because it does autoconf in user
space. It supports tokens, which are entirely handled in user space.

However, when using tokens, NetworkManager likes to configure the token
also in kernel. Yes, it's not overly useful, but it's pretty nice that
you see the token in `ip token` too.

This wasn't an issue until recently, because NetworkManager didn't
actually set accept_ra=0.


2)

If you want to set

  a) token ::1 dev eth0
  b) echo 1 >  /proc/sys/net/ipv6/conf/eth0/accept_ra
  c) ip link set
eth0 up

then you can do the 3 steps in several different orders, but not in the
most(?) sensible one: a,b,c).

Yes, this makes the earlier concern about the danger of people doing
the sensible thing on newer kernels bigger.



3)

There is the oddity that
 
  # echo 1 >  /proc/sys/net/ipv6/conf/w/accept_ra
  # ip token set ::123 dev w
  # ip token

shows the token. Then, 

  # echo 0 >  /proc/sys/net/ipv6/conf/w/accept_ra
  # ip token

still shows the token. The EINVAL indicates you that having a token
with accept_ra=0 is wrong. But still, it shows a token set, and you
have no way of clearing it (except toggling accept_ra).



I don't care so much about 1) either. If this is really how kernel
wants to do it, fine. NetworkManager won't set the token. It just
doesn't seem sensible to me.



best,
Thomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]