netdev - Re: [PATCHv2 net-next] IPv6: add extack info for inet6_addr

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: Windows password security audit tool. GUI, reports in PDF.

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

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

On Thu, Jul 20, 2023 at 01:26:37PM +0300, Ido Schimmel wrote:
> > -static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
> > +static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa,
> > +			      struct netlink_ext_ack *extack)
> >  {
> >  	struct net *net = dev_net(dev);
> >  	unsigned int hash = inet6_addr_hash(net, &ifa->addr);
> > @@ -1037,7 +1038,7 @@ static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
> >  
> >  	/* Ignore adding duplicate addresses on an interface */
> >  	if (ipv6_chk_same_addr(net, &ifa->addr, dev, hash)) {
> > -		netdev_dbg(dev, "ipv6_add_addr: already assigned\n");
> > +		NL_SET_ERR_MSG(extack, "ipv6_add_addr: already assigned");
> 
> How do you trigger it?
> 
> # ip link add name dummy10 up type dummy
> # ip address add 2001:db8:1::1/64 dev dummy10
> # ip address add 2001:db8:1::1/64 dev dummy10
> RTNETLINK answers: File exists
> 
> Better to add extack in inet6_rtm_newaddr():
> 
> if (nlh->nlmsg_flags & NLM_F_EXCL || 
>     !(nlh->nlmsg_flags & NLM_F_REPLACE))
> 	err = -EEXIST;
> else
> 	err = inet6_addr_modify(net, ifa, &cfg)

Thanks. Since the dup addr checking will be done on inet6_rtm_newaddr(). I
will remove the extack msg in ipv6_add_addr_hash().

> > @@ -1103,6 +1108,7 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
> >  
> >  	f6i = addrconf_f6i_alloc(net, idev, cfg->pfx, false, gfp_flags);
> >  	if (IS_ERR(f6i)) {
> > +		NL_SET_ERR_MSG(extack, "Dest allocate failed");
> 
> The only thing that can fail in this function is ip6_route_info_create()
> which already has an extack argument. Better to pass extack to
> addrconf_f6i_alloc() and get a more accurate error message.

OK, I will update the patch.

> > -	if (cfg->plen > 128)
> > +	if (cfg->plen > 128) {
> > +		NL_SET_ERR_MSG(extack, "IPv6 address prefix length larger than 128");
> 
> For RTM_NEWROUTE IPv6 code just says "Invalid prefix length", so might
> as well be consistent with it. Also, I see IPv4 doesn't have such
> messages for its RTM_{NEW,DEL}ADDR messages. If you think it's useful
> for IPv6, then I suggest also adding it to IPv4.

OK. But since this patch is for IPv6. I will post another patch for IPv4
specifically.

Thanks
Hangbin