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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZL99lOAlwAsvsJU1@Laptop-X1>
Date: Tue, 25 Jul 2023 15:45:24 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: David Ahern <dsahern@...nel.org>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Ido Schimmel <idosch@...sch.org>,
	Beniamino Galvani <bgalvani@...hat.com>
Subject: Re: [PATCHv3 net-next] IPv6: add extack info for IPv6 address
 add/delete

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@...hat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.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@...nel.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);
> 
>