Re: [PATCH net-next 2/3] inet: do not use RTNL in inet_netconf_get_devconf()

[Jiri Pirko <jiri@resnulli.us>]

Tue, Feb 27, 2024 at 10:24:10AM CET, edumazet@google.com wrote:
>"ip -4 netconf show dev XXXX" no longer acquires RTNL.

I was under impression that you refer to the current code, confused me a
bit :/


>
>Return -ENODEV instead of -EINVAL if no netdev or idev can be found.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>---
> net/ipv4/devinet.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
>diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>index ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c..f045a34e90b974b17512a30c3b719bdfc3cba153 100644
>--- a/net/ipv4/devinet.c
>+++ b/net/ipv4/devinet.c
>@@ -2205,21 +2205,20 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,
> 				    struct netlink_ext_ack *extack)
> {
> 	struct net *net = sock_net(in_skb->sk);
>-	struct nlattr *tb[NETCONFA_MAX+1];
>+	struct nlattr *tb[NETCONFA_MAX + 1];
>+	const struct ipv4_devconf *devconf;
>+	struct in_device *in_dev = NULL;
>+	struct net_device *dev = NULL;
> 	struct sk_buff *skb;
>-	struct ipv4_devconf *devconf;
>-	struct in_device *in_dev;
>-	struct net_device *dev;
> 	int ifindex;
> 	int err;
> 
> 	err = inet_netconf_valid_get_req(in_skb, nlh, tb, extack);
> 	if (err)
>-		goto errout;
>+		return err;
> 
>-	err = -EINVAL;
> 	if (!tb[NETCONFA_IFINDEX])
>-		goto errout;
>+		return -EINVAL;
> 
> 	ifindex = nla_get_s32(tb[NETCONFA_IFINDEX]);
> 	switch (ifindex) {
>@@ -2230,10 +2229,10 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,
> 		devconf = net->ipv4.devconf_dflt;
> 		break;
> 	default:
>-		dev = __dev_get_by_index(net, ifindex);
>-		if (!dev)
>-			goto errout;
>-		in_dev = __in_dev_get_rtnl(dev);
>+		err = -ENODEV;
>+		dev = dev_get_by_index(net, ifindex);

Comment says:
/* Deprecated for new users, call netdev_get_by_index() instead */
struct net_device *dev_get_by_index(struct net *net, int ifindex)

Perhaps better to use:
netdev_get_by_index() and netdev_put()?


>+		if (dev)
>+			in_dev = in_dev_get(dev);

The original flow:
		err = -ENODEV;
		dev = dev_get_by_index(net, ifindex);
		if (!dev)
			goto errout;
		in_dev = in_dev_get(dev);
 		if (!in_dev)
 			goto errout;
Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit.



> 		if (!in_dev)
> 			goto errout;
> 		devconf = &in_dev->cnf;
>@@ -2257,6 +2256,9 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,
> 	}
> 	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> errout:
>+	if (in_dev)
>+		in_dev_put(in_dev);
>+	dev_put(dev);
> 	return err;
> }
> 
>@@ -2826,5 +2828,6 @@ void __init devinet_init(void)
> 	rtnl_register(PF_INET, RTM_DELADDR, inet_rtm_deladdr, NULL, 0);
> 	rtnl_register(PF_INET, RTM_GETADDR, NULL, inet_dump_ifaddr, 0);
> 	rtnl_register(PF_INET, RTM_GETNETCONF, inet_netconf_get_devconf,
>-		      inet_netconf_dump_devconf, 0);
>+		      inet_netconf_dump_devconf,
>+		      RTNL_FLAG_DOIT_UNLOCKED);
> }
>-- 
>2.44.0.rc1.240.g4c46232300-goog
>
>