From: Ido Schimmel <idosch@idosch.org>
To: Hangbin Liu <liuhangbin@gmail.com>
Cc: netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
David Ahern <dsahern@kernel.org>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Beniamino Galvani <bgalvani@redhat.com>
Subject: Re: [PATCHv2 net-next] IPv6: add extack info for inet6_addr_add/del
Date: Thu, 20 Jul 2023 13:26:37 +0300 [thread overview]
Message-ID: <ZLkL3eNVNfzZbaBv@shredder> (raw)
In-Reply-To: <20230719135644.3011570-1-liuhangbin@gmail.com>
On Wed, Jul 19, 2023 at 09:56:44PM +0800, Hangbin Liu wrote:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index e5213e598a04..4e0836d90e65 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1027,7 +1027,8 @@ static bool ipv6_chk_same_addr(struct net *net, const struct in6_addr *addr,
> return false;
> }
>
> -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)
> err = -EEXIST;
> } else {
> hlist_add_head_rcu(&ifa->addr_lst, &net->ipv6.inet6_addr_lst[hash]);
> @@ -1066,15 +1067,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);
> + }
>
> if (idev->dead) {
> - err = -ENODEV; /*XXX*/
> + NL_SET_ERR_MSG(extack, "Device marked as dead");
This seems to be a transient state when IPv6 device is being deleted. See
addrconf_ifdown(). Maybe "IPv6 device is going away".
> + err = -ENODEV;
> goto out;
> }
>
> if (idev->cnf.disable_ipv6) {
> + NL_SET_ERR_MSG(extack, "IPv6 is disabled on this device");
> err = -EACCES;
> goto out;
> }
> @@ -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.
> err = PTR_ERR(f6i);
> f6i = NULL;
> goto out;
> @@ -1140,7 +1146,7 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
>
> rcu_read_lock();
>
> - err = ipv6_add_addr_hash(idev->dev, ifa);
> + err = ipv6_add_addr_hash(idev->dev, ifa, extack);
> if (err < 0) {
> rcu_read_unlock();
> goto out;
> @@ -2488,18 +2494,22 @@ static void addrconf_add_mroute(struct net_device *dev)
> ip6_route_add(&cfg, GFP_KERNEL, NULL);
> }
>
> -static struct inet6_dev *addrconf_add_dev(struct net_device *dev)
> +static struct inet6_dev *addrconf_add_dev(struct net_device *dev, struct netlink_ext_ack *extack)
> {
> struct inet6_dev *idev;
>
> ASSERT_RTNL();
>
> idev = ipv6_find_idev(dev);
> - if (IS_ERR(idev))
> + if (IS_ERR(idev)) {
> + NL_SET_ERR_MSG(extack, "No such device");
This is not very accurate. See comment below regarding __in6_dev_get().
> return idev;
> + }
>
> - if (idev->cnf.disable_ipv6)
> + if (idev->cnf.disable_ipv6) {
> + NL_SET_ERR_MSG(extack, "IPv6 is disabled on this device");
> return ERR_PTR(-EACCES);
> + }
>
> /* Add default multicast route */
> if (!(dev->flags & IFF_LOOPBACK) && !netif_is_l3_master(dev))
> @@ -2919,21 +2929,29 @@ static int inet6_addr_add(struct net *net, int ifindex,
>
> ASSERT_RTNL();
>
> - 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.
Same comment for delete path.
> return -EINVAL;
> + }
>
> /* check the lifetime */
> - if (!cfg->valid_lft || cfg->preferred_lft > cfg->valid_lft)
> + if (!cfg->valid_lft || cfg->preferred_lft > cfg->valid_lft) {
> + NL_SET_ERR_MSG(extack, "IPv6 address lifetime invalid");
> return -EINVAL;
> + }
>
> - if (cfg->ifa_flags & IFA_F_MANAGETEMPADDR && cfg->plen != 64)
> + if (cfg->ifa_flags & IFA_F_MANAGETEMPADDR && cfg->plen != 64) {
> + NL_SET_ERR_MSG(extack, "IPv6 address with mngtmpaddr flag must have prefix length 64");
\"mngtmpaddr\"
a prefix length of
> return -EINVAL;
> + }
>
> dev = __dev_get_by_index(net, ifindex);
> - if (!dev)
> + if (!dev) {
> + NL_SET_ERR_MSG(extack, "Unable to find the interface");
This is already checked in the netlink path (see inet6_rtm_newaddr()),
so this message will never be displayed. If you want to see it, then add
it in inet6_rtm_newaddr().
> return -ENODEV;
> + }
>
> - idev = addrconf_add_dev(dev);
> + idev = addrconf_add_dev(dev, extack);
> if (IS_ERR(idev))
> return PTR_ERR(idev);
>
> @@ -2941,8 +2959,10 @@ static int inet6_addr_add(struct net *net, int ifindex,
> int ret = ipv6_mc_config(net->ipv6.mc_autojoin_sk,
> true, cfg->pfx, ifindex);
>
> - if (ret < 0)
> + if (ret < 0) {
> + NL_SET_ERR_MSG(extack, "Multicast auto join failed");
> return ret;
> + }
> }
>
> cfg->scope = ipv6_addr_scope(cfg->pfx);
> @@ -2999,22 +3019,29 @@ static int inet6_addr_add(struct net *net, int ifindex,
> }
>
> static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
> - const struct in6_addr *pfx, unsigned int plen)
> + const struct in6_addr *pfx, unsigned int plen,
> + struct netlink_ext_ack *extack)
> {
> struct inet6_ifaddr *ifp;
> struct inet6_dev *idev;
> struct net_device *dev;
>
> - if (plen > 128)
> + if (plen > 128) {
> + NL_SET_ERR_MSG(extack, "IPv6 address prefix length larger than 128");
> return -EINVAL;
> + }
>
> dev = __dev_get_by_index(net, ifindex);
> - if (!dev)
> + if (!dev) {
> + NL_SET_ERR_MSG(extack, "Unable to find the interface");
> return -ENODEV;
> + }
>
> idev = __in6_dev_get(dev);
> - if (!idev)
> + if (!idev) {
> + NL_SET_ERR_MSG(extack, "No such address on the device");
A more accurate message would be "IPv6 is disabled on this device". See
ndisc_allow_add().
> return -ENXIO;
> + }
>
> read_lock_bh(&idev->lock);
> list_for_each_entry(ifp, &idev->addr_list, if_list) {
> @@ -3037,6 +3064,8 @@ static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
> }
> }
> read_unlock_bh(&idev->lock);
> +
> + NL_SET_ERR_MSG(extack, "IPv6 address not found");
> return -EADDRNOTAVAIL;
> }
>
> @@ -3079,7 +3108,7 @@ int addrconf_del_ifaddr(struct net *net, void __user *arg)
>
> rtnl_lock();
> err = inet6_addr_del(net, ireq.ifr6_ifindex, 0, &ireq.ifr6_addr,
> - ireq.ifr6_prefixlen);
> + ireq.ifr6_prefixlen, NULL);
> rtnl_unlock();
> return err;
> }
> @@ -3378,7 +3407,7 @@ static void addrconf_dev_config(struct net_device *dev)
> return;
> }
>
> - idev = addrconf_add_dev(dev);
> + idev = addrconf_add_dev(dev, NULL);
> if (IS_ERR(idev))
> return;
>
> @@ -4692,7 +4721,7 @@ inet6_rtm_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh,
> ifa_flags &= IFA_F_MANAGETEMPADDR;
>
> return inet6_addr_del(net, ifm->ifa_index, ifa_flags, pfx,
> - ifm->ifa_prefixlen);
> + ifm->ifa_prefixlen, extack);
> }
>
> static int modify_prefix_route(struct inet6_ifaddr *ifp,
> --
> 2.38.1
>
next prev parent reply other threads:[~2023-07-20 10:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-19 13:56 [PATCHv2 net-next] IPv6: add extack info for inet6_addr_add/del Hangbin Liu
2023-07-20 10:26 ` Ido Schimmel [this message]
2023-07-24 3:59 ` Hangbin Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZLkL3eNVNfzZbaBv@shredder \
--to=idosch@idosch.org \
--cc=bgalvani@redhat.com \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=liuhangbin@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.