Re: [PATCHv3 net-next] IPv6: add extack info for IPv6 address add/delete

[Hangbin Liu <liuhangbin@gmail.com>]

On Mon, Jul 24, 2023 at 09:02:42AM -0600, David Ahern wrote:
> On 7/24/23 1:50 AM, Hangbin Liu wrote:
> > Add extack info for IPv6 address add/delete, which would be useful for
> > users to understand the problem without having to read kernel code.
> > 
> > Suggested-by: Beniamino Galvani <bgalvani@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> > v3: * Update extack message. Pass extack to addrconf_f6i_alloc().
> >     * Return "IPv6 is disabled" for addrconf_add_dev(), as the same
> >       with ndisc_allow_add() does.
> >     * Set dup addr extack message in inet6_rtm_newaddr() instead of
> >       ipv6_add_addr_hash().
> > v2: Update extack msg for dead dev. Remove msg for NOBUFS error.
> >     Add extack for ipv6_add_addr_hash()
> > ---
> >  include/net/ip6_route.h |  2 +-
> >  net/ipv6/addrconf.c     | 63 +++++++++++++++++++++++++++++------------
> >  net/ipv6/anycast.c      |  2 +-
> >  net/ipv6/route.c        |  5 ++--
> >  4 files changed, 50 insertions(+), 22 deletions(-)
> > 
> 
> This patch is getting long enough, so:
> Reviewed-by: David Ahern <dsahern@kernel.org>
> 

Thanks a lot for your review.

> Followup requests below. Thanks,

> 
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 19eb4b3d26ea..53dea18a4a07 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -1068,15 +1068,19 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
> >  	     !(cfg->ifa_flags & IFA_F_MCAUTOJOIN)) ||
> >  	    (!(idev->dev->flags & IFF_LOOPBACK) &&
> >  	     !netif_is_l3_master(idev->dev) &&
> > -	     addr_type & IPV6_ADDR_LOOPBACK))
> > +	     addr_type & IPV6_ADDR_LOOPBACK)) {
> > +		NL_SET_ERR_MSG(extack, "Cannot assign requested address");
> >  		return ERR_PTR(-EADDRNOTAVAIL);
> > +	}
> 
> It would be good to split the above checks into separate ones with more
> specific messages.

How about this.

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 53dea18a4a07..e6c3fe413441 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1063,13 +1063,17 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
        struct fib6_info *f6i = NULL;
        int err = 0;

-       if (addr_type == IPV6_ADDR_ANY ||
-           (addr_type & IPV6_ADDR_MULTICAST &&
-            !(cfg->ifa_flags & IFA_F_MCAUTOJOIN)) ||
-           (!(idev->dev->flags & IFF_LOOPBACK) &&
-            !netif_is_l3_master(idev->dev) &&
-            addr_type & IPV6_ADDR_LOOPBACK)) {
-               NL_SET_ERR_MSG(extack, "Cannot assign requested address");
+       if (addr_type == IPV6_ADDR_ANY) {
+               NL_SET_ERR_MSG(extack, "IPv6: Cannot assign unspecified address");
+               return ERR_PTR(-EADDRNOTAVAIL);
+       } else if (addr_type & IPV6_ADDR_MULTICAST &&
+                  !(cfg->ifa_flags & IFA_F_MCAUTOJOIN)) {
+               NL_SET_ERR_MSG(extack, "IPv6: Cannot assign multicast address without \"IFA_F_MCAUTOJOIN\" flag");
+               return ERR_PTR(-EADDRNOTAVAIL);
+       } else if (!(idev->dev->flags & IFF_LOOPBACK) &&
+                  !netif_is_l3_master(idev->dev) &&
+                  addr_type & IPV6_ADDR_LOOPBACK) {
+               NL_SET_ERR_MSG(extack, "IPv6: Cannot assign loopback address on this device");
                return ERR_PTR(-EADDRNOTAVAIL);
        }

> ...
> 
> >  
> >  	if (cfg->ifa_flags & IFA_F_MCAUTOJOIN) {
> >  		int ret = ipv6_mc_config(net->ipv6.mc_autojoin_sk,
> >  					 true, cfg->pfx, ifindex);
> 
> and pass extack to this one for better message as well.

This one looks a little deep. We need pass extack to
- ipv6_mc_config
  - ipv6_sock_mc_join
    - __ipv6_sock_mc_join
      - __ipv6_dev_mc_inc	maybe also this one??

to get more detailed message. And all these are "Join multicast group failed".
Do you still want to do this?

Thanks
Hangbin

> 
> >  
> > -		if (ret < 0)
> > +		if (ret < 0) {
> > +			NL_SET_ERR_MSG(extack, "Multicast auto join failed");
> >  			return ret;
> > +		}
> >  	}
> >  
> >  	cfg->scope = ipv6_addr_scope(cfg->pfx);
> 
>