[PATCH iproute2] iptoken: Add doc on the conditions of iptoken

[PATCH iproute2] iptoken: Add doc on the conditions of iptoken

[Hongren Zheng]

`ip token set suffix dev interface' may be unsuccessful
with only the error 'RTNETLINK answers: Invalid argument'
prompted. For users this is mysterious and hard to debug.
Hence a more user-friendly prompt is added.

This commit adds doc for conditions for setting the token and
making the token take effect. For the former one, conditions
in the function 'inet6_set_iftoken' of 'net/ipv6/addrconf.c'
of the Linux kernel code is documented.

For the latter one, conditions in the function 'addrconf_prefix_rcv'
of 'net/ipv6/addrconf.c' of the Linux kernel code is docuemnted.

Signed-off-by: Hongren Zheng <i@zenithal.me>
---
 ip/iptoken.c        |  4 +++-
 man/man8/ip-token.8 | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ip/iptoken.c b/ip/iptoken.c
index 9f356890..d56fd68d 100644
--- a/ip/iptoken.c
+++ b/ip/iptoken.c
@@ -177,8 +177,10 @@ static int iptoken_set(int argc, char **argv, bool delete)
 	addattr_nest_end(&req.n, afs6);
 	addattr_nest_end(&req.n, afs);
 
-	if (rtnl_talk(&rth, &req.n, NULL) < 0)
+	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
+		fprintf(stderr, "Conditions not met: 'man ip-token' for more info\n");
 		return -2;
+	}
 
 	return 0;
 }
diff --git a/man/man8/ip-token.8 b/man/man8/ip-token.8
index 6505b8c5..ac64eb66 100644
--- a/man/man8/ip-token.8
+++ b/man/man8/ip-token.8
@@ -67,6 +67,30 @@ must be left out.
 list all tokenized interface identifiers for the networking interfaces from
 the kernel.
 
+.SH "NOTES"
+Several conditions should be met before setting the token for an interface.
+.RS
+.IP A
+\- The interface is not a loopback device.
+.IP B
+\- The interface does not have NOARP flag.
+.IP C
+\- The interface accepts router advertisement (RA). To be more specific,
+net.ipv6.conf.interface.accept_ra=1,
+and when net.ipv6.conf.interface.forwarding=1,
+net.ipv6.conf.interface.accept_ra=2.
+.RE
+
+For the token to take effect, several conditions should be met.
+.RS
+.IP A
+\- The interface has autoconf flag turned on. To be more specific, net.ipv6.conf.interface.autoconf=1
+.IP B
+\- The router advertisement (RA) has autonomous address-configuration flag turned on.
+.IP C
+\- The length of the prefix in the router advertisement (RA) is 64.
+.RE
+
 .SH SEE ALSO
 .br
 .BR ip (8)
-- 
2.31.0

Re: [PATCH iproute2] iptoken: Add doc on the conditions of iptoken

[Stephen Hemminger]

On Sat, 27 Mar 2021 21:36:07 +0800
Hongren Zheng <i@zenithal.me> wrote:

> `ip token set suffix dev interface' may be unsuccessful
> with only the error 'RTNETLINK answers: Invalid argument'
> prompted. For users this is mysterious and hard to debug.
> Hence a more user-friendly prompt is added.
> 
> This commit adds doc for conditions for setting the token and
> making the token take effect. For the former one, conditions
> in the function 'inet6_set_iftoken' of 'net/ipv6/addrconf.c'
> of the Linux kernel code is documented.
> 
> For the latter one, conditions in the function 'addrconf_prefix_rcv'
> of 'net/ipv6/addrconf.c' of the Linux kernel code is docuemnted.
> 
> Signed-off-by: Hongren Zheng <i@zenithal.me>
> ---
>  ip/iptoken.c        |  4 +++-
>  man/man8/ip-token.8 | 24 ++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/ip/iptoken.c b/ip/iptoken.c
> index 9f356890..d56fd68d 100644
> --- a/ip/iptoken.c
> +++ b/ip/iptoken.c
> @@ -177,8 +177,10 @@ static int iptoken_set(int argc, char **argv, bool delete)
>  	addattr_nest_end(&req.n, afs6);
>  	addattr_nest_end(&req.n, afs);
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0)
> +	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +		fprintf(stderr, "Conditions not met: 'man ip-token' for more info\n");
>  		return -2;
> +	}
>  
>  	return 0;
>  }
> diff --git a/man/man8/ip-token.8 b/man/man8/ip-token.8
> index 6505b8c5..ac64eb66 100644
> --- a/man/man8/ip-token.8
> +++ b/man/man8/ip-token.8
> @@ -67,6 +67,30 @@ must be left out.
>  list all tokenized interface identifiers for the networking interfaces from
>  the kernel.
>  
> +.SH "NOTES"
> +Several conditions should be met before setting the token for an interface.
> +.RS
> +.IP A
> +\- The interface is not a loopback device.
> +.IP B
> +\- The interface does not have NOARP flag.
> +.IP C
> +\- The interface accepts router advertisement (RA). To be more specific,
> +net.ipv6.conf.interface.accept_ra=1,
> +and when net.ipv6.conf.interface.forwarding=1,
> +net.ipv6.conf.interface.accept_ra=2.
> +.RE
> +
> +For the token to take effect, several conditions should be met.
> +.RS
> +.IP A
> +\- The interface has autoconf flag turned on. To be more specific, net.ipv6.conf.interface.autoconf=1
> +.IP B
> +\- The router advertisement (RA) has autonomous address-configuration flag turned on.
> +.IP C
> +\- The length of the prefix in the router advertisement (RA) is 64.
> +.RE
> +
>  .SH SEE ALSO
>  .br
>  .BR ip (8)

It would be better if kernel provided the error messages through external ack
of the netlink message, rather than providing potentially out of date
recommendations on the man page.

[RFC] add extack errors for iptoken

[Stephen Hemminger]

Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
could be added.

Note: requires trickling down the extack parameter, but that is a good thing because
other place like devconf could use it as well.

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 4da61c950e93..479f60ef54c0 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -147,8 +147,8 @@ struct rtnl_af_ops {
 	int			(*validate_link_af)(const struct net_device *dev,
 						    const struct nlattr *attr);
 	int			(*set_link_af)(struct net_device *dev,
-					       const struct nlattr *attr);
-
+					       const struct nlattr *attr,
+					       struct netlink_ext_ack *extack);
 	int			(*fill_stats_af)(struct sk_buff *skb,
 						 const struct net_device *dev);
 	size_t			(*get_stats_af_size)(const struct net_device *dev);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1bdcb33fb561..3485b16a7ff3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2863,7 +2863,7 @@ static int do_setlink(const struct sk_buff *skb,
 
 			BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af))));
 
-			err = af_ops->set_link_af(dev, af);
+			err = af_ops->set_link_af(dev, af, extack);
 			if (err < 0) {
 				rcu_read_unlock();
 				goto errout;
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 75f67994fc85..2e35f68da40a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1978,7 +1978,8 @@ static int inet_validate_link_af(const struct net_device *dev,
 	return 0;
 }
 
-static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
+static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
+			    struct netlink_ext_ack *extack)
 {
 	struct in_device *in_dev = __in_dev_get_rcu(dev);
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 120073ffb666..b817086fbf42 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5672,7 +5672,8 @@ static int inet6_fill_link_af(struct sk_buff *skb, const struct net_device *dev,
 	return 0;
 }
 
-static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
+static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token,
+			     struct netlink_ext_ack *extack)
 {
 	struct inet6_ifaddr *ifp;
 	struct net_device *dev = idev->dev;
@@ -5681,14 +5682,29 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
 
 	ASSERT_RTNL();
 
-	if (!token)
+	if (!token) {
 		return -EINVAL;
-	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
+	}
+
+	if (dev->flags & IFF_LOOPBACK) {
+		NL_SET_ERR_MSG_MOD(extack, "Device is loopback");
 		return -EINVAL;
-	if (!ipv6_accept_ra(idev))
+	}
+
+	if (dev->flags & IFF_NOARP) {
+		NL_SET_ERR_MSG_MOD(extack, "Device does not do discovery");
 		return -EINVAL;
-	if (idev->cnf.rtr_solicits == 0)
+	}
+
+	if (!ipv6_accept_ra(idev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");
+		return -EINVAL;
+	}
+
+	if (idev->cnf.rtr_solicits == 0) {
+		NL_SET_ERR_MSG(extack, "Device has disabled router solicitation");
 		return -EINVAL;
+	}
 
 	write_lock_bh(&idev->lock);
 
@@ -5796,7 +5812,8 @@ static int inet6_validate_link_af(const struct net_device *dev,
 	return 0;
 }
 
-static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
+static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla,
+			     struct netlink_ext_ack *extack)
 {
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	struct nlattr *tb[IFLA_INET6_MAX + 1];
@@ -5809,7 +5826,8 @@ static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
 		BUG();
 
 	if (tb[IFLA_INET6_TOKEN]) {
-		err = inet6_set_iftoken(idev, nla_data(tb[IFLA_INET6_TOKEN]));
+		err = inet6_set_iftoken(idev, nla_data(tb[IFLA_INET6_TOKEN]),
+					extack);
 		if (err)
 			return err;
 	}

Re: [RFC] add extack errors for iptoken

[Hongren Zheng]

On Wed, Mar 31, 2021 at 08:49:02PM -0700, Stephen Hemminger wrote:
> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> could be added.

This is an elegant solution. I found extack is extensively used in 
the other parts of the kernel code for similar purposes.

I also checked the code of iproute2 and found a good support for
extack.

So I am OK with this PATCH.

Also I tested this patch against v5.12-rc5, it compiles and can boot 
with `make ARCH=x86_64 x86_64_defconfig` config in qemu.

I tested it with iproute2 and found a more friendly error prompt:

    $ ip token set ::2 dev enp0s3
    Error: ipv6: Device does accept route adverts.

Tested-by: Hongren Zheng <i@zenithal.me>

> +		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");

Should be "Device does not accept route adverts".

Re: [PATCH iproute2] iptoken: Add doc on the conditions of iptoken

[Hongren Zheng]

On Wed, Mar 31, 2021 at 03:26:02PM -0700, Stephen Hemminger wrote:
> It would be better if kernel provided the error messages through external ack
> of the netlink message,

Agreed.

> rather than providing potentially out of date
> recommendations on the man page.

I still think conditions for ip-token to be accepted and take
effect should be documented on the man page.

Errors in kernel extack only give hints to users in case they 
forget to configure some flags. For new users, a complete 
condition reference should be documented for them to
evaluate the use case of ip-token.
Also the autoconf flag would not prompt errors when the user
forgets to turn it on, this is unexpected when the user does
intend to use ip-token.

Even /proc/sys interface may be out of date, these conditions
may remain unchanged or only be altered slightly, hence
documenting them does not hurt.

Re: [RFC] add extack errors for iptoken

[David Ahern]

On 3/31/21 9:49 PM, Stephen Hemminger wrote:
> @@ -5681,14 +5682,29 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
>  
>  	ASSERT_RTNL();
>  
> -	if (!token)
> +	if (!token) {

You forgot to add a message here.

>  		return -EINVAL;
> -	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
> +	}
> +
> +	if (dev->flags & IFF_LOOPBACK) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device is loopback");
>  		return -EINVAL;
> -	if (!ipv6_accept_ra(idev))
> +	}
> +
> +	if (dev->flags & IFF_NOARP) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device does not do discovery");

'Device does not do neighbor discovery'

>  		return -EINVAL;
> -	if (idev->cnf.rtr_solicits == 0)
> +	}
> +
> +	if (!ipv6_accept_ra(idev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");

How about
'Router advertisements are disabled for this device'


> +		return -EINVAL;
> +	}
> +
> +	if (idev->cnf.rtr_solicits == 0) {
> +		NL_SET_ERR_MSG(extack, "Device has disabled router solicitation");

How about

'Router solicitation is disabled on device'

Re: [RFC] add extack errors for iptoken

[Stephen Hemminger]

On Thu, 1 Apr 2021 08:31:05 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 3/31/21 9:49 PM, Stephen Hemminger wrote:
> > @@ -5681,14 +5682,29 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
> >  
> >  	ASSERT_RTNL();
> >  
> > -	if (!token)
> > +	if (!token) {  
> 
> You forgot to add a message here.

This branch is unreachable, function is  only called from place where it is not NULL.

Re: [RFC] add extack errors for iptoken

[Hongren Zheng]

> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> could be added.

Since this patch has been tested, and we have waited a long time for
comments and there is no further response, I wonder if it is the time
to submit this patch to the kernel.

Re: [RFC] add extack errors for iptoken

[David Ahern]

On 4/28/21 6:55 AM, Hongren Zheng wrote:
>> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
>> could be added.
> 
> Since this patch has been tested, and we have waited a long time for
> comments and there is no further response, I wonder if it is the time
> to submit this patch to the kernel.
> 

send the patch

Re: [RFC] add extack errors for iptoken

[Hongren Zheng]

On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > could be added.
> 
> Since this patch has been tested, and we have waited a long time for
> comments and there is no further response, I wonder if it is the time
> to submit this patch to the kernel.

Is there any updates?

I'm not quite familiar with "RFC" procedure. Should I send this patch to
netdev mailing list with title "[PATCH] add extack errors for iptoken" now
(I suppose not), or wait for Stephen Hemminger sending it, or wait for
more comments?

-- 
GPG Fingerprint: 1127F188280AE3123619332987E17EEF9B18B6C9

Re: [RFC] add extack errors for iptoken

[Stephen Hemminger]

On Sat, 29 May 2021 14:31:56 +0800
Hongren Zheng <i@zenithal.me> wrote:

> On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > > could be added.  
> > 
> > Since this patch has been tested, and we have waited a long time for
> > comments and there is no further response, I wonder if it is the time
> > to submit this patch to the kernel.  
> 
> Is there any updates?
> 
> I'm not quite familiar with "RFC" procedure. Should I send this patch to
> netdev mailing list with title "[PATCH] add extack errors for iptoken" now
> (I suppose not), or wait for Stephen Hemminger sending it, or wait for
> more comments?
> 

The kernel changes is already upstream with this commit for 5.12 kernel

commit 3583a4e8d77d44697a21437227dd53fc6e7b2cb5
Author: Stephen Hemminger <stephen@networkplumber.org>
Date:   Wed Apr 7 08:59:12 2021 -0700

    ipv6: report errors for iftoken via netlink extack
    
    Setting iftoken can fail for several different reasons but there
    and there was no report to user as to the cause. Add netlink
    extended errors to the processing of the request.
    
    This requires adding additional argument through rtnl_af_ops
    set_link_af callback.
    
    Reported-by: Hongren Zheng <li@zenithal.me>
    Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
    Reviewed-by: David Ahern <dsahern@kernel.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [RFC] add extack errors for iptoken

[Hongren Zheng]

On Sun, May 30, 2021 at 07:42:34PM -0700, Stephen Hemminger wrote:
> On Sat, 29 May 2021 14:31:56 +0800
> Hongren Zheng <i@zenithal.me> wrote:
> 
> > On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > > > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > > > could be added.  
> > > 
> > > Since this patch has been tested, and we have waited a long time for
> > > comments and there is no further response, I wonder if it is the time
> > > to submit this patch to the kernel.  
> > 
> > Is there any updates?
> > 
> > I'm not quite familiar with "RFC" procedure. Should I send this patch to
> > netdev mailing list with title "[PATCH] add extack errors for iptoken" now
> > (I suppose not), or wait for Stephen Hemminger sending it, or wait for
> > more comments?
> > 
> 
> The kernel changes is already upstream with this commit for 5.12 kernel
> 
> commit 3583a4e8d77d44697a21437227dd53fc6e7b2cb5
> Author: Stephen Hemminger <stephen@networkplumber.org>
> Date:   Wed Apr 7 08:59:12 2021 -0700
> 
>     ipv6: report errors for iftoken via netlink extack
>     
>     Setting iftoken can fail for several different reasons but there
>     and there was no report to user as to the cause. Add netlink
>     extended errors to the processing of the request.
>     
>     This requires adding additional argument through rtnl_af_ops
>     set_link_af callback.
>     
>     Reported-by: Hongren Zheng <li@zenithal.me>

No wonder I did not receive this email and kept pinging in this thread.

My email address is i@zenithal.me, not li@zenithal.me

Still thank you for getting this upstreamed!

>     Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>     Reviewed-by: David Ahern <dsahern@kernel.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>