[PATCH net-next 0/3] inet: implement lockless RTM_GETNETCONF ops

[PATCH net-next 0/3] inet: implement lockless RTM_GETNETCONF ops

[Eric Dumazet]

This series removes RTNL use for RTM_GETNETCONF operations on AF_INET.

- Annotate data-races to avoid possible KCSAN splats.

- "ip -4 netconf show dev XXX" can be implemented without RTNL [1]

- "ip -4 netconf" dumps can be implemented using RCU instead of RTNL [1]

[1] This only refers to RTM_GETNETCONF operation, "ip" command
    also uses RTM_GETLINK dumps which are using RTNL at this moment.

Eric Dumazet (3):
  inet: annotate devconf data-races
  inet: do not use RTNL in inet_netconf_get_devconf()
  inet: use xa_array iterator to implement inet_netconf_dump_devconf()

 include/linux/inetdevice.h |  14 ++--
 net/ipv4/devinet.c         | 147 +++++++++++++++++--------------------
 net/ipv4/igmp.c            |   4 +-
 net/ipv4/proc.c            |   2 +-
 net/ipv4/route.c           |   4 +-
 5 files changed, 81 insertions(+), 90 deletions(-)

-- 
2.44.0.rc1.240.g4c46232300-goog

[PATCH net-next 1/3] inet: annotate devconf data-races

[Eric Dumazet]

Add READ_ONCE() in ipv4_devconf_get() and corresponding
WRITE_ONCE() in ipv4_devconf_set()

Add IPV4_DEVCONF_RO() and IPV4_DEVCONF_ALL_RO() macros,
and use them when reading devconf fields.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/inetdevice.h | 14 ++++++++------
 net/ipv4/devinet.c         | 21 +++++++++++----------
 net/ipv4/igmp.c            |  4 ++--
 net/ipv4/proc.c            |  2 +-
 net/ipv4/route.c           |  4 ++--
 5 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index ddb27fc0ee8c8862d62f8c6243c4239ea53374f2..cb5280e6cc219106bcc55972dac850edf34988ff 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -53,13 +53,15 @@ struct in_device {
 };
 
 #define IPV4_DEVCONF(cnf, attr) ((cnf).data[IPV4_DEVCONF_ ## attr - 1])
+#define IPV4_DEVCONF_RO(cnf, attr) READ_ONCE(IPV4_DEVCONF(cnf, attr))
 #define IPV4_DEVCONF_ALL(net, attr) \
 	IPV4_DEVCONF((*(net)->ipv4.devconf_all), attr)
+#define IPV4_DEVCONF_ALL_RO(net, attr) READ_ONCE(IPV4_DEVCONF_ALL(net, attr))
 
-static inline int ipv4_devconf_get(struct in_device *in_dev, int index)
+static inline int ipv4_devconf_get(const struct in_device *in_dev, int index)
 {
 	index--;
-	return in_dev->cnf.data[index];
+	return READ_ONCE(in_dev->cnf.data[index]);
 }
 
 static inline void ipv4_devconf_set(struct in_device *in_dev, int index,
@@ -67,7 +69,7 @@ static inline void ipv4_devconf_set(struct in_device *in_dev, int index,
 {
 	index--;
 	set_bit(index, in_dev->cnf.state);
-	in_dev->cnf.data[index] = val;
+	WRITE_ONCE(in_dev->cnf.data[index], val);
 }
 
 static inline void ipv4_devconf_setall(struct in_device *in_dev)
@@ -81,18 +83,18 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 	ipv4_devconf_set((in_dev), IPV4_DEVCONF_ ## attr, (val))
 
 #define IN_DEV_ANDCONF(in_dev, attr) \
-	(IPV4_DEVCONF_ALL(dev_net(in_dev->dev), attr) && \
+	(IPV4_DEVCONF_ALL_RO(dev_net(in_dev->dev), attr) && \
 	 IN_DEV_CONF_GET((in_dev), attr))
 
 #define IN_DEV_NET_ORCONF(in_dev, net, attr) \
-	(IPV4_DEVCONF_ALL(net, attr) || \
+	(IPV4_DEVCONF_ALL_RO(net, attr) || \
 	 IN_DEV_CONF_GET((in_dev), attr))
 
 #define IN_DEV_ORCONF(in_dev, attr) \
 	IN_DEV_NET_ORCONF(in_dev, dev_net(in_dev->dev), attr)
 
 #define IN_DEV_MAXCONF(in_dev, attr) \
-	(max(IPV4_DEVCONF_ALL(dev_net(in_dev->dev), attr), \
+	(max(IPV4_DEVCONF_ALL_RO(dev_net(in_dev->dev), attr), \
 	     IN_DEV_CONF_GET((in_dev), attr)))
 
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index bc74f131fe4dfad327e71c1a8f0a4b66cdc526e5..ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1982,7 +1982,7 @@ static int inet_fill_link_af(struct sk_buff *skb, const struct net_device *dev,
 		return -EMSGSIZE;
 
 	for (i = 0; i < IPV4_DEVCONF_MAX; i++)
-		((u32 *) nla_data(nla))[i] = in_dev->cnf.data[i];
+		((u32 *) nla_data(nla))[i] = READ_ONCE(in_dev->cnf.data[i]);
 
 	return 0;
 }
@@ -2068,9 +2068,9 @@ static int inet_netconf_msgsize_devconf(int type)
 }
 
 static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
-				     struct ipv4_devconf *devconf, u32 portid,
-				     u32 seq, int event, unsigned int flags,
-				     int type)
+				     const struct ipv4_devconf *devconf,
+				     u32 portid, u32 seq, int event,
+				     unsigned int flags, int type)
 {
 	struct nlmsghdr  *nlh;
 	struct netconfmsg *ncm;
@@ -2095,27 +2095,28 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 
 	if ((all || type == NETCONFA_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_FORWARDING,
-			IPV4_DEVCONF(*devconf, FORWARDING)) < 0)
+			IPV4_DEVCONF_RO(*devconf, FORWARDING)) < 0)
 		goto nla_put_failure;
 	if ((all || type == NETCONFA_RP_FILTER) &&
 	    nla_put_s32(skb, NETCONFA_RP_FILTER,
-			IPV4_DEVCONF(*devconf, RP_FILTER)) < 0)
+			IPV4_DEVCONF_RO(*devconf, RP_FILTER)) < 0)
 		goto nla_put_failure;
 	if ((all || type == NETCONFA_MC_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
-			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
+			IPV4_DEVCONF_RO(*devconf, MC_FORWARDING)) < 0)
 		goto nla_put_failure;
 	if ((all || type == NETCONFA_BC_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_BC_FORWARDING,
-			IPV4_DEVCONF(*devconf, BC_FORWARDING)) < 0)
+			IPV4_DEVCONF_RO(*devconf, BC_FORWARDING)) < 0)
 		goto nla_put_failure;
 	if ((all || type == NETCONFA_PROXY_NEIGH) &&
 	    nla_put_s32(skb, NETCONFA_PROXY_NEIGH,
-			IPV4_DEVCONF(*devconf, PROXY_ARP)) < 0)
+			IPV4_DEVCONF_RO(*devconf, PROXY_ARP)) < 0)
 		goto nla_put_failure;
 	if ((all || type == NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN) &&
 	    nla_put_s32(skb, NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
-			IPV4_DEVCONF(*devconf, IGNORE_ROUTES_WITH_LINKDOWN)) < 0)
+			IPV4_DEVCONF_RO(*devconf,
+					IGNORE_ROUTES_WITH_LINKDOWN)) < 0)
 		goto nla_put_failure;
 
 out:
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index efeeca2b13285a3149645ab945b0364391f6721b..717e97a389a8aee75f51a5a5d859cb94a5d868eb 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -120,12 +120,12 @@
  */
 
 #define IGMP_V1_SEEN(in_dev) \
-	(IPV4_DEVCONF_ALL(dev_net(in_dev->dev), FORCE_IGMP_VERSION) == 1 || \
+	(IPV4_DEVCONF_ALL_RO(dev_net(in_dev->dev), FORCE_IGMP_VERSION) == 1 || \
 	 IN_DEV_CONF_GET((in_dev), FORCE_IGMP_VERSION) == 1 || \
 	 ((in_dev)->mr_v1_seen && \
 	  time_before(jiffies, (in_dev)->mr_v1_seen)))
 #define IGMP_V2_SEEN(in_dev) \
-	(IPV4_DEVCONF_ALL(dev_net(in_dev->dev), FORCE_IGMP_VERSION) == 2 || \
+	(IPV4_DEVCONF_ALL_RO(dev_net(in_dev->dev), FORCE_IGMP_VERSION) == 2 || \
 	 IN_DEV_CONF_GET((in_dev), FORCE_IGMP_VERSION) == 2 || \
 	 ((in_dev)->mr_v2_seen && \
 	  time_before(jiffies, (in_dev)->mr_v2_seen)))
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 5f4654ebff487494b6afbaa69174df4c004395dc..914bc9c35cc702395aee257fb010034294e501de 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -395,7 +395,7 @@ static int snmp_seq_show_ipstats(struct seq_file *seq, void *v)
 		seq_printf(seq, " %s", snmp4_ipstats_list[i].name);
 
 	seq_printf(seq, "\nIp: %d %d",
-		   IPV4_DEVCONF_ALL(net, FORWARDING) ? 1 : 2,
+		   IPV4_DEVCONF_ALL_RO(net, FORWARDING) ? 1 : 2,
 		   READ_ONCE(net->ipv4.sysctl_ip_default_ttl));
 
 	BUILD_BUG_ON(offsetof(struct ipstats_mib, mibs) != 0);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index b512288d6fcc6bc0ce586bc5dc501b979ac3b9c1..c8f76f56dc1653371ca39663f29cc798b062e60d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2313,7 +2313,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		if (IN_DEV_BFORWARD(in_dev))
 			goto make_route;
 		/* not do cache if bc_forwarding is enabled */
-		if (IPV4_DEVCONF_ALL(net, BC_FORWARDING))
+		if (IPV4_DEVCONF_ALL_RO(net, BC_FORWARDING))
 			do_cache = false;
 		goto brd_input;
 	}
@@ -2993,7 +2993,7 @@ static int rt_fill_info(struct net *net, __be32 dst, __be32 src,
 #ifdef CONFIG_IP_MROUTE
 			if (ipv4_is_multicast(dst) &&
 			    !ipv4_is_local_multicast(dst) &&
-			    IPV4_DEVCONF_ALL(net, MC_FORWARDING)) {
+			    IPV4_DEVCONF_ALL_RO(net, MC_FORWARDING)) {
 				int err = ipmr_get_route(net, skb,
 							 fl4->saddr, fl4->daddr,
 							 r, portid);
-- 
2.44.0.rc1.240.g4c46232300-goog

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

[Eric Dumazet]

"ip -4 netconf show dev XXXX" no longer acquires RTNL.

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);
+		if (dev)
+			in_dev = in_dev_get(dev);
 		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

[PATCH net-next 3/3] inet: use xa_array iterator to implement inet_netconf_dump_devconf()

[Eric Dumazet]

1) inet_netconf_dump_devconf() can run under RCU protection
   instead of RTNL.

2) properly return 0 at the end of a dump, avoiding an
   an extra recvmsg() system call.

3) Do not use inet_base_seq() anymore, for_each_netdev_dump()
   has nice properties. Restarting a GETDEVCONF dump if a device has
   been added/removed or if net->ipv4.dev_addr_genid has changed is moot.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/devinet.c | 101 +++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index f045a34e90b974b17512a30c3b719bdfc3cba153..af741af61830aeb695e7e75608515547dade8f39 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2267,11 +2267,13 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
 {
 	const struct nlmsghdr *nlh = cb->nlh;
 	struct net *net = sock_net(skb->sk);
-	int h, s_h;
-	int idx, s_idx;
+	struct {
+		unsigned long ifindex;
+		unsigned int all_default;
+	} *ctx = (void *)cb->ctx;
+	const struct in_device *in_dev;
 	struct net_device *dev;
-	struct in_device *in_dev;
-	struct hlist_head *head;
+	int err = 0;
 
 	if (cb->strict_check) {
 		struct netlink_ext_ack *extack = cb->extack;
@@ -2288,64 +2290,47 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
 		}
 	}
 
-	s_h = cb->args[0];
-	s_idx = idx = cb->args[1];
-
-	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
-		idx = 0;
-		head = &net->dev_index_head[h];
-		rcu_read_lock();
-		cb->seq = inet_base_seq(net);
-		hlist_for_each_entry_rcu(dev, head, index_hlist) {
-			if (idx < s_idx)
-				goto cont;
-			in_dev = __in_dev_get_rcu(dev);
-			if (!in_dev)
-				goto cont;
-
-			if (inet_netconf_fill_devconf(skb, dev->ifindex,
-						      &in_dev->cnf,
-						      NETLINK_CB(cb->skb).portid,
-						      nlh->nlmsg_seq,
-						      RTM_NEWNETCONF,
-						      NLM_F_MULTI,
-						      NETCONFA_ALL) < 0) {
-				rcu_read_unlock();
-				goto done;
-			}
-			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
-cont:
-			idx++;
-		}
-		rcu_read_unlock();
+	rcu_read_lock();
+	for_each_netdev_dump(net, dev, ctx->ifindex) {
+		in_dev = __in_dev_get_rcu(dev);
+		if (!in_dev)
+			continue;
+		err = inet_netconf_fill_devconf(skb, dev->ifindex,
+						&in_dev->cnf,
+						NETLINK_CB(cb->skb).portid,
+						nlh->nlmsg_seq,
+						RTM_NEWNETCONF, NLM_F_MULTI,
+						NETCONFA_ALL);
+		if (err < 0)
+			goto done;
 	}
-	if (h == NETDEV_HASHENTRIES) {
-		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
-					      net->ipv4.devconf_all,
-					      NETLINK_CB(cb->skb).portid,
-					      nlh->nlmsg_seq,
-					      RTM_NEWNETCONF, NLM_F_MULTI,
-					      NETCONFA_ALL) < 0)
+	if (ctx->all_default == 0) {
+		err = inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
+						net->ipv4.devconf_all,
+						NETLINK_CB(cb->skb).portid,
+						nlh->nlmsg_seq,
+						RTM_NEWNETCONF, NLM_F_MULTI,
+						NETCONFA_ALL);
+		if (err < 0)
 			goto done;
-		else
-			h++;
-	}
-	if (h == NETDEV_HASHENTRIES + 1) {
-		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
-					      net->ipv4.devconf_dflt,
-					      NETLINK_CB(cb->skb).portid,
-					      nlh->nlmsg_seq,
-					      RTM_NEWNETCONF, NLM_F_MULTI,
-					      NETCONFA_ALL) < 0)
+		ctx->all_default++;
+	}
+	if (ctx->all_default == 1) {
+		err = inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
+						net->ipv4.devconf_dflt,
+						NETLINK_CB(cb->skb).portid,
+						nlh->nlmsg_seq,
+						RTM_NEWNETCONF, NLM_F_MULTI,
+						NETCONFA_ALL);
+		if (err < 0)
 			goto done;
-		else
-			h++;
+		ctx->all_default++;
 	}
 done:
-	cb->args[0] = h;
-	cb->args[1] = idx;
-
-	return skb->len;
+	if (err < 0 && likely(skb->len))
+		err = skb->len;
+	rcu_read_unlock();
+	return err;
 }
 
 #ifdef CONFIG_SYSCTL
@@ -2829,5 +2814,5 @@ void __init devinet_init(void)
 	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,
-		      RTNL_FLAG_DOIT_UNLOCKED);
+		      RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED);
 }
-- 
2.44.0.rc1.240.g4c46232300-goog

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

[Jiri Pirko]

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
>
>

Re: [PATCH net-next 1/3] inet: annotate devconf data-races

[Jiri Pirko]

Tue, Feb 27, 2024 at 10:24:09AM CET, edumazet@google.com wrote:
>Add READ_ONCE() in ipv4_devconf_get() and corresponding
>WRITE_ONCE() in ipv4_devconf_set()
>
>Add IPV4_DEVCONF_RO() and IPV4_DEVCONF_ALL_RO() macros,
>and use them when reading devconf fields.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next 3/3] inet: use xa_array iterator to implement inet_netconf_dump_devconf()

[Jiri Pirko]

Tue, Feb 27, 2024 at 10:24:11AM CET, edumazet@google.com wrote:
>1) inet_netconf_dump_devconf() can run under RCU protection
>   instead of RTNL.
>
>2) properly return 0 at the end of a dump, avoiding an
>   an extra recvmsg() system call.
>
>3) Do not use inet_base_seq() anymore, for_each_netdev_dump()
>   has nice properties. Restarting a GETDEVCONF dump if a device has
>   been added/removed or if net->ipv4.dev_addr_genid has changed is moot.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

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

[Eric Dumazet]

On Tue, Feb 27, 2024 at 1:59 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> 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)

Only for long-standing allocations, where we are not sure if a leak
could happen or not.
We do not bother allocating a tracker otherwise.
Look at inet6_netconf_get_devconf() :
We left there dev_get_by_index() and dev_put().

I think I am aware of the tracking facility, I implemented it...


>
> 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;

A single goto looks nicer to me.

> Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit.
>

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

[Jiri Pirko]

Tue, Feb 27, 2024 at 02:09:49PM CET, edumazet@google.com wrote:
>On Tue, Feb 27, 2024 at 1:59 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> 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)
>
>Only for long-standing allocations, where we are not sure if a leak
>could happen or not.
>We do not bother allocating a tracker otherwise.

Makes sense. Would it make sense to fix the "deprecated" comment then to
also reflect this usecase?


>Look at inet6_netconf_get_devconf() :
>We left there dev_get_by_index() and dev_put().
>
>I think I am aware of the tracking facility, I implemented it...

Yeah, I was just refering to the comment.


>
>
>>
>> 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;
>
>A single goto looks nicer to me.

:)

Reviewed-by: Jiri Pirko <jiri@nvidia.com>


>
>> Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit.
>>

Re: [PATCH net-next 0/3] inet: implement lockless RTM_GETNETCONF ops

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 27 Feb 2024 09:24:08 +0000 you wrote:
> This series removes RTNL use for RTM_GETNETCONF operations on AF_INET.
> 
> - Annotate data-races to avoid possible KCSAN splats.
> 
> - "ip -4 netconf show dev XXX" can be implemented without RTNL [1]
> 
> - "ip -4 netconf" dumps can be implemented using RCU instead of RTNL [1]
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] inet: annotate devconf data-races
    https://git.kernel.org/netdev/net-next/c/0598f8f3bb77
  - [net-next,2/3] inet: do not use RTNL in inet_netconf_get_devconf()
    https://git.kernel.org/netdev/net-next/c/bbcf91053bb6
  - [net-next,3/3] inet: use xa_array iterator to implement inet_netconf_dump_devconf()
    https://git.kernel.org/netdev/net-next/c/167487070d64

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html