[PATCH v2 net-next 0/7] neighbour: Convert RTM_GETNEIGH and RTM_{GET,SET}NEIGHTBL to RCU.

[PATCH v2 net-next 0/7] neighbour: Convert RTM_GETNEIGH and RTM_{GET,SET}NEIGHTBL to RCU.

[Kuniyuki Iwashima]

Patch 1 - 4 moves validations and skb allocation in neigh_get()
as prep for patch 5, which converts RTM_GETNEIGH to RCU.

Patch 6 & 7 converts RTM_GETNEIGHTBL and RTM_SETNEIGHTBL to RCU,
which requires almost nothing.


Changes:
  v2:
    * Patch 1 & 2 : Fix ERR_PTR(0) paths

  v1: https://lore.kernel.org/netdev/20250416004253.20103-1-kuniyu@amazon.com/


Kuniyuki Iwashima (7):
  neighbour: Make neigh_valid_get_req() return ndmsg.
  neighbour: Move two validations from neigh_get() to
    neigh_valid_get_req().
  neighbour: Allocate skb in neigh_get().
  neighbour: Move neigh_find_table() to neigh_get().
  neighbour: Convert RTM_GETNEIGH to RCU.
  neighbour: Convert RTM_GETNEIGHTBL to RCU.
  neighbour: Convert RTM_SETNEIGHTBL to RCU.

 net/core/neighbour.c | 211 +++++++++++++++++++++----------------------
 1 file changed, 105 insertions(+), 106 deletions(-)

-- 
2.49.0

[PATCH v2 net-next 1/7] neighbour: Make neigh_valid_get_req() return ndmsg.

[Kuniyuki Iwashima]

neigh_get() passes 4 local variable pointers to neigh_valid_get_req().

If it returns a pointer of struct ndmsg, we do not need to pass two
of them.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
v2: Fix ERR_PTR(0) paths
---
 net/core/neighbour.c | 51 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 65cf582b5dac..e1b0d9177270 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2846,10 +2846,9 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
 	return err;
 }
 
-static int neigh_valid_get_req(const struct nlmsghdr *nlh,
-			       struct neigh_table **tbl,
-			       void **dst, int *dev_idx, u8 *ndm_flags,
-			       struct netlink_ext_ack *extack)
+static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
+					 struct neigh_table **tbl, void **dst,
+					 struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[NDA_MAX + 1];
 	struct ndmsg *ndm;
@@ -2858,31 +2857,33 @@ static int neigh_valid_get_req(const struct nlmsghdr *nlh,
 	ndm = nlmsg_payload(nlh, sizeof(*ndm));
 	if (!ndm) {
 		NL_SET_ERR_MSG(extack, "Invalid header for neighbor get request");
-		return -EINVAL;
+		err = -EINVAL;
+		goto err;
 	}
 
 	if (ndm->ndm_pad1  || ndm->ndm_pad2  || ndm->ndm_state ||
 	    ndm->ndm_type) {
 		NL_SET_ERR_MSG(extack, "Invalid values in header for neighbor get request");
-		return -EINVAL;
+		err = -EINVAL;
+		goto err;
 	}
 
 	if (ndm->ndm_flags & ~NTF_PROXY) {
 		NL_SET_ERR_MSG(extack, "Invalid flags in header for neighbor get request");
-		return -EINVAL;
+		err = -EINVAL;
+		goto err;
 	}
 
 	err = nlmsg_parse_deprecated_strict(nlh, sizeof(struct ndmsg), tb,
 					    NDA_MAX, nda_policy, extack);
 	if (err < 0)
-		return err;
+		goto err;
 
-	*ndm_flags = ndm->ndm_flags;
-	*dev_idx = ndm->ndm_ifindex;
 	*tbl = neigh_find_table(ndm->ndm_family);
-	if (*tbl == NULL) {
+	if (!*tbl) {
 		NL_SET_ERR_MSG(extack, "Unsupported family in header for neighbor get request");
-		return -EAFNOSUPPORT;
+		err = -EAFNOSUPPORT;
+		goto err;
 	}
 
 	for (i = 0; i <= NDA_MAX; ++i) {
@@ -2893,17 +2894,21 @@ static int neigh_valid_get_req(const struct nlmsghdr *nlh,
 		case NDA_DST:
 			if (nla_len(tb[i]) != (int)(*tbl)->key_len) {
 				NL_SET_ERR_MSG(extack, "Invalid network address in neighbor get request");
-				return -EINVAL;
+				err = -EINVAL;
+				goto err;
 			}
 			*dst = nla_data(tb[i]);
 			break;
 		default:
 			NL_SET_ERR_MSG(extack, "Unsupported attribute in neighbor get request");
-			return -EINVAL;
+			err = -EINVAL;
+			goto err;
 		}
 	}
 
-	return 0;
+	return ndm;
+err:
+	return ERR_PTR(err);
 }
 
 static inline size_t neigh_nlmsg_size(void)
@@ -2974,18 +2979,16 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	struct net_device *dev = NULL;
 	struct neigh_table *tbl = NULL;
 	struct neighbour *neigh;
+	struct ndmsg *ndm;
 	void *dst = NULL;
-	u8 ndm_flags = 0;
-	int dev_idx = 0;
 	int err;
 
-	err = neigh_valid_get_req(nlh, &tbl, &dst, &dev_idx, &ndm_flags,
-				  extack);
-	if (err < 0)
-		return err;
+	ndm = neigh_valid_get_req(nlh, &tbl, &dst, extack);
+	if (IS_ERR(ndm))
+		return PTR_ERR(ndm);
 
-	if (dev_idx) {
-		dev = __dev_get_by_index(net, dev_idx);
+	if (ndm->ndm_ifindex) {
+		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
 		if (!dev) {
 			NL_SET_ERR_MSG(extack, "Unknown device ifindex");
 			return -ENODEV;
@@ -2997,7 +3000,7 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
-	if (ndm_flags & NTF_PROXY) {
+	if (ndm->ndm_flags & NTF_PROXY) {
 		struct pneigh_entry *pn;
 
 		pn = pneigh_lookup(tbl, net, dst, dev, 0);
-- 
2.49.0

[PATCH v2 net-next 2/7] neighbour: Move two validations from neigh_get() to neigh_valid_get_req().

[Kuniyuki Iwashima]

We will remove RTNL for neigh_get() and run it under RCU instead.

neigh_get() returns -EINVAL in the following cases:

  * NDA_DST is not specified
  * Both ndm->ndm_ifindex and NTF_PROXY are not specified

These validations do not require RCU.

Let's move them to neigh_valid_get_req().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
v2: Fix ERR_PTR(0) paths
---
 net/core/neighbour.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index e1b0d9177270..29f3d5e31901 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2874,6 +2874,12 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
 		goto err;
 	}
 
+	if (!(ndm->ndm_flags & NTF_PROXY) && !ndm->ndm_ifindex) {
+		NL_SET_ERR_MSG(extack, "No device specified");
+		err = -EINVAL;
+		goto err;
+	}
+
 	err = nlmsg_parse_deprecated_strict(nlh, sizeof(struct ndmsg), tb,
 					    NDA_MAX, nda_policy, extack);
 	if (err < 0)
@@ -2887,11 +2893,14 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
 	}
 
 	for (i = 0; i <= NDA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
 		switch (i) {
 		case NDA_DST:
+			if (!tb[i]) {
+				NL_SET_ERR_MSG(extack, "Network address not specified");
+				err = -EINVAL;
+				goto err;
+			}
+
 			if (nla_len(tb[i]) != (int)(*tbl)->key_len) {
 				NL_SET_ERR_MSG(extack, "Invalid network address in neighbor get request");
 				err = -EINVAL;
@@ -2900,6 +2909,9 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
 			*dst = nla_data(tb[i]);
 			break;
 		default:
+			if (!tb[i])
+				continue;
+
 			NL_SET_ERR_MSG(extack, "Unsupported attribute in neighbor get request");
 			err = -EINVAL;
 			goto err;
@@ -2995,11 +3007,6 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		}
 	}
 
-	if (!dst) {
-		NL_SET_ERR_MSG(extack, "Network address not specified");
-		return -EINVAL;
-	}
-
 	if (ndm->ndm_flags & NTF_PROXY) {
 		struct pneigh_entry *pn;
 
@@ -3012,11 +3019,6 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 					nlh->nlmsg_seq, tbl);
 	}
 
-	if (!dev) {
-		NL_SET_ERR_MSG(extack, "No device specified");
-		return -EINVAL;
-	}
-
 	neigh = neigh_lookup(tbl, dst, dev);
 	if (!neigh) {
 		NL_SET_ERR_MSG(extack, "Neighbour entry not found");
-- 
2.49.0

[PATCH v2 net-next 3/7] neighbour: Allocate skb in neigh_get().

[Kuniyuki Iwashima]

We will remove RTNL for neigh_get() and run it under RCU instead.

neigh_get_reply() and pneigh_get_reply() allocate skb with GFP_KERNEL.

Let's move the allocation before __dev_get_by_index() in neigh_get().

Now, neigh_get_reply() and pneigh_get_reply() are inlined and
rtnl_unicast() is factorised.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/neighbour.c | 88 ++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 56 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 29f3d5e31901..1abce19040bf 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2934,27 +2934,6 @@ static inline size_t neigh_nlmsg_size(void)
 	       + nla_total_size(1); /* NDA_PROTOCOL */
 }
 
-static int neigh_get_reply(struct net *net, struct neighbour *neigh,
-			   u32 pid, u32 seq)
-{
-	struct sk_buff *skb;
-	int err = 0;
-
-	skb = nlmsg_new(neigh_nlmsg_size(), GFP_KERNEL);
-	if (!skb)
-		return -ENOBUFS;
-
-	err = neigh_fill_info(skb, neigh, pid, seq, RTM_NEWNEIGH, 0);
-	if (err) {
-		kfree_skb(skb);
-		goto errout;
-	}
-
-	err = rtnl_unicast(skb, net, pid);
-errout:
-	return err;
-}
-
 static inline size_t pneigh_nlmsg_size(void)
 {
 	return NLMSG_ALIGN(sizeof(struct ndmsg))
@@ -2963,34 +2942,16 @@ static inline size_t pneigh_nlmsg_size(void)
 	       + nla_total_size(1); /* NDA_PROTOCOL */
 }
 
-static int pneigh_get_reply(struct net *net, struct pneigh_entry *neigh,
-			    u32 pid, u32 seq, struct neigh_table *tbl)
-{
-	struct sk_buff *skb;
-	int err = 0;
-
-	skb = nlmsg_new(pneigh_nlmsg_size(), GFP_KERNEL);
-	if (!skb)
-		return -ENOBUFS;
-
-	err = pneigh_fill_info(skb, neigh, pid, seq, RTM_NEWNEIGH, 0, tbl);
-	if (err) {
-		kfree_skb(skb);
-		goto errout;
-	}
-
-	err = rtnl_unicast(skb, net, pid);
-errout:
-	return err;
-}
-
 static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		     struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(in_skb->sk);
+	u32 pid = NETLINK_CB(in_skb).portid;
 	struct net_device *dev = NULL;
 	struct neigh_table *tbl = NULL;
+	u32 seq = nlh->nlmsg_seq;
 	struct neighbour *neigh;
+	struct sk_buff *skb;
 	struct ndmsg *ndm;
 	void *dst = NULL;
 	int err;
@@ -2999,11 +2960,19 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	if (IS_ERR(ndm))
 		return PTR_ERR(ndm);
 
+	if (ndm->ndm_flags & NTF_PROXY)
+		skb = nlmsg_new(neigh_nlmsg_size(), GFP_KERNEL);
+	else
+		skb = nlmsg_new(pneigh_nlmsg_size(), GFP_KERNEL);
+	if (!skb)
+		return -ENOBUFS;
+
 	if (ndm->ndm_ifindex) {
 		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
 		if (!dev) {
 			NL_SET_ERR_MSG(extack, "Unknown device ifindex");
-			return -ENODEV;
+			err = -ENODEV;
+			goto err;
 		}
 	}
 
@@ -3013,23 +2982,30 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		pn = pneigh_lookup(tbl, net, dst, dev, 0);
 		if (!pn) {
 			NL_SET_ERR_MSG(extack, "Proxy neighbour entry not found");
-			return -ENOENT;
+			err = -ENOENT;
+			goto err;
 		}
-		return pneigh_get_reply(net, pn, NETLINK_CB(in_skb).portid,
-					nlh->nlmsg_seq, tbl);
-	}
-
-	neigh = neigh_lookup(tbl, dst, dev);
-	if (!neigh) {
-		NL_SET_ERR_MSG(extack, "Neighbour entry not found");
-		return -ENOENT;
-	}
 
-	err = neigh_get_reply(net, neigh, NETLINK_CB(in_skb).portid,
-			      nlh->nlmsg_seq);
+		err = pneigh_fill_info(skb, pn, pid, seq, RTM_NEWNEIGH, 0, tbl);
+		if (err)
+			goto err;
+	} else {
+		neigh = neigh_lookup(tbl, dst, dev);
+		if (!neigh) {
+			NL_SET_ERR_MSG(extack, "Neighbour entry not found");
+			err = -ENOENT;
+			goto err;
+		}
 
-	neigh_release(neigh);
+		err = neigh_fill_info(skb, neigh, pid, seq, RTM_NEWNEIGH, 0);
+		neigh_release(neigh);
+		if (err)
+			goto err;
+	}
 
+	return rtnl_unicast(skb, net, pid);
+err:
+	kfree_skb(skb);
 	return err;
 }
 
-- 
2.49.0

Re: [PATCH v2 net-next 3/7] neighbour: Allocate skb in neigh_get().

[Paolo Abeni]

On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> @@ -3013,23 +2982,30 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  		pn = pneigh_lookup(tbl, net, dst, dev, 0);

pneigh_lookup() can create the neighbor when the last argument is 1, and
contains an ASSERT_RTNL() on such code path that may confuse the casual
reader. I think here you could use __pneigh_lookup().

Thanks,

Paolo

Re: [PATCH v2 net-next 3/7] neighbour: Allocate skb in neigh_get().

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 24 Apr 2025 10:29:19 +0200
> On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> > @@ -3013,23 +2982,30 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> >  		pn = pneigh_lookup(tbl, net, dst, dev, 0);
> 
> pneigh_lookup() can create the neighbor when the last argument is 1, and
> contains an ASSERT_RTNL() on such code path that may confuse the casual
> reader.

Agree, I didn't like the ASSERT_RTNL() in the middle of the function...


> I think here you could use __pneigh_lookup().

read_lock_bh() is needed, but yes.

Only one caller passes 1 to pneigh_lookup(), and only pndisc_is_router()
calls __pneigh_lookup().

I'll clean them up in v3 too.

Thanks!

[PATCH v2 net-next 4/7] neighbour: Move neigh_find_table() to neigh_get().

[Kuniyuki Iwashima]

neigh_valid_get_req() calls neigh_find_table() to fetch neigh_tables[].

neigh_find_table() uses rcu_dereference_rtnl(), but RTNL actually does
not protect it at all; neigh_table_clear() can be called without RTNL
and only waits for RCU readers by synchronize_rcu().

Fortunately, there is no bug because IPv4 is built-in, IPv6 cannot be
unloaded, and DECNET was removed.

To fetch neigh_tables[] by rcu_dereference() later, let's move
neigh_find_table() from neigh_valid_get_req() to neigh_get().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/neighbour.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 1abce19040bf..b1d90696ca0c 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2847,10 +2847,9 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
-					 struct neigh_table **tbl, void **dst,
+					 struct nlattr **tb,
 					 struct netlink_ext_ack *extack)
 {
-	struct nlattr *tb[NDA_MAX + 1];
 	struct ndmsg *ndm;
 	int err, i;
 
@@ -2885,13 +2884,6 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
 	if (err < 0)
 		goto err;
 
-	*tbl = neigh_find_table(ndm->ndm_family);
-	if (!*tbl) {
-		NL_SET_ERR_MSG(extack, "Unsupported family in header for neighbor get request");
-		err = -EAFNOSUPPORT;
-		goto err;
-	}
-
 	for (i = 0; i <= NDA_MAX; ++i) {
 		switch (i) {
 		case NDA_DST:
@@ -2900,13 +2892,6 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
 				err = -EINVAL;
 				goto err;
 			}
-
-			if (nla_len(tb[i]) != (int)(*tbl)->key_len) {
-				NL_SET_ERR_MSG(extack, "Invalid network address in neighbor get request");
-				err = -EINVAL;
-				goto err;
-			}
-			*dst = nla_data(tb[i]);
 			break;
 		default:
 			if (!tb[i])
@@ -2947,16 +2932,17 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 {
 	struct net *net = sock_net(in_skb->sk);
 	u32 pid = NETLINK_CB(in_skb).portid;
+	struct nlattr *tb[NDA_MAX + 1];
 	struct net_device *dev = NULL;
-	struct neigh_table *tbl = NULL;
 	u32 seq = nlh->nlmsg_seq;
+	struct neigh_table *tbl;
 	struct neighbour *neigh;
 	struct sk_buff *skb;
 	struct ndmsg *ndm;
-	void *dst = NULL;
+	void *dst;
 	int err;
 
-	ndm = neigh_valid_get_req(nlh, &tbl, &dst, extack);
+	ndm = neigh_valid_get_req(nlh, tb, extack);
 	if (IS_ERR(ndm))
 		return PTR_ERR(ndm);
 
@@ -2967,6 +2953,21 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	if (!skb)
 		return -ENOBUFS;
 
+	tbl = neigh_find_table(ndm->ndm_family);
+	if (!tbl) {
+		NL_SET_ERR_MSG(extack, "Unsupported family in header for neighbor get request");
+		err = -EAFNOSUPPORT;
+		goto err;
+	}
+
+	if (nla_len(tb[NDA_DST]) != (int)tbl->key_len) {
+		NL_SET_ERR_MSG(extack, "Invalid network address in neighbor get request");
+		err = -EINVAL;
+		goto err;
+	}
+
+	dst = nla_data(tb[NDA_DST]);
+
 	if (ndm->ndm_ifindex) {
 		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
 		if (!dev) {
-- 
2.49.0

[PATCH v2 net-next 5/7] neighbour: Convert RTM_GETNEIGH to RCU.

[Kuniyuki Iwashima]

Only __dev_get_by_index() is the RTNL dependant in neigh_get().

Let's replace it with dev_get_by_index_rcu() and convert RTM_GETNEIGH
to RCU.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/neighbour.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index b1d90696ca0c..ccfef86bb044 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2953,6 +2953,8 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	if (!skb)
 		return -ENOBUFS;
 
+	rcu_read_lock();
+
 	tbl = neigh_find_table(ndm->ndm_family);
 	if (!tbl) {
 		NL_SET_ERR_MSG(extack, "Unsupported family in header for neighbor get request");
@@ -2969,7 +2971,7 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 	dst = nla_data(tb[NDA_DST]);
 
 	if (ndm->ndm_ifindex) {
-		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
+		dev = dev_get_by_index_rcu(net, ndm->ndm_ifindex);
 		if (!dev) {
 			NL_SET_ERR_MSG(extack, "Unknown device ifindex");
 			err = -ENODEV;
@@ -3004,8 +3006,11 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 			goto err;
 	}
 
+	rcu_read_unlock();
+
 	return rtnl_unicast(skb, net, pid);
 err:
+	rcu_read_unlock();
 	kfree_skb(skb);
 	return err;
 }
@@ -3805,7 +3810,7 @@ static const struct rtnl_msg_handler neigh_rtnl_msg_handlers[] __initconst = {
 	{.msgtype = RTM_NEWNEIGH, .doit = neigh_add},
 	{.msgtype = RTM_DELNEIGH, .doit = neigh_delete},
 	{.msgtype = RTM_GETNEIGH, .doit = neigh_get, .dumpit = neigh_dump_info,
-	 .flags = RTNL_FLAG_DUMP_UNLOCKED},
+	 .flags = RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED},
 	{.msgtype = RTM_GETNEIGHTBL, .dumpit = neightbl_dump_info},
 	{.msgtype = RTM_SETNEIGHTBL, .doit = neightbl_set},
 };
-- 
2.49.0

Re: [PATCH v2 net-next 5/7] neighbour: Convert RTM_GETNEIGH to RCU.

[Paolo Abeni]

On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> Only __dev_get_by_index() is the RTNL dependant in neigh_get().

Very minor nit catched by checkpatch and only noted because I think the
series needs a new revision for other reasons. Typo above: 'dependent'

/P

Re: [PATCH v2 net-next 5/7] neighbour: Convert RTM_GETNEIGH to RCU.

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 24 Apr 2025 10:31:44 +0200
> On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> > Only __dev_get_by_index() is the RTNL dependant in neigh_get().
> 
> Very minor nit catched by checkpatch and only noted because I think the
> series needs a new revision for other reasons. Typo above: 'dependent'

Rather I thought dependant was the correct spell, and after noticing
the checkpatch warning and googling, it turned out to be one of
flavo"u"r, so I'll post a patch for checkpatch :)

[PATCH v2 net-next 6/7] neighbour: Convert RTM_GETNEIGHTBL to RCU.

[Kuniyuki Iwashima]

neightbl_dump_info() calls neightbl_fill_info() and
neightbl_fill_param_info() for each neigh_tables[] entry.

Both functions rely on the table lock (read_lock_bh(&tbl->lock)),
so RTNL is not needed.

Let's fetch the table under RCU and convert RTM_GETNEIGHTBL to RCU.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/neighbour.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index ccfef86bb044..38732d8f0ed7 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2467,10 +2467,12 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
 
 	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
 
+	rcu_read_lock();
+
 	for (tidx = 0; tidx < NEIGH_NR_TABLES; tidx++) {
 		struct neigh_parms *p;
 
-		tbl = rcu_dereference_rtnl(neigh_tables[tidx]);
+		tbl = rcu_dereference(neigh_tables[tidx]);
 		if (!tbl)
 			continue;
 
@@ -2504,6 +2506,8 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
 		neigh_skip = 0;
 	}
 out:
+	rcu_read_unlock();
+
 	cb->args[0] = tidx;
 	cb->args[1] = nidx;
 
@@ -3811,7 +3815,8 @@ static const struct rtnl_msg_handler neigh_rtnl_msg_handlers[] __initconst = {
 	{.msgtype = RTM_DELNEIGH, .doit = neigh_delete},
 	{.msgtype = RTM_GETNEIGH, .doit = neigh_get, .dumpit = neigh_dump_info,
 	 .flags = RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED},
-	{.msgtype = RTM_GETNEIGHTBL, .dumpit = neightbl_dump_info},
+	{.msgtype = RTM_GETNEIGHTBL, .dumpit = neightbl_dump_info,
+	 .flags = RTNL_FLAG_DUMP_UNLOCKED},
 	{.msgtype = RTM_SETNEIGHTBL, .doit = neightbl_set},
 };
 
-- 
2.49.0

Re: [PATCH v2 net-next 6/7] neighbour: Convert RTM_GETNEIGHTBL to RCU.

[Paolo Abeni]

On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> neightbl_dump_info() calls neightbl_fill_info() and

AFAICS neightbl_fill_info() is only invoked from neightbl_dump_info()
and acquires/releases the RCU internally. Such lock pair should be dropped.

> neightbl_fill_param_info() for each neigh_tables[] entry.
> 
> Both functions rely on the table lock (read_lock_bh(&tbl->lock)),
> so RTNL is not needed.
> 
> Let's fetch the table under RCU and convert RTM_GETNEIGHTBL to RCU.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/core/neighbour.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index ccfef86bb044..38732d8f0ed7 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2467,10 +2467,12 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  
>  	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
>  
> +	rcu_read_lock();
> +
>  	for (tidx = 0; tidx < NEIGH_NR_TABLES; tidx++) {
>  		struct neigh_parms *p;
>  
> -		tbl = rcu_dereference_rtnl(neigh_tables[tidx]);
> +		tbl = rcu_dereference(neigh_tables[tidx]);
>  		if (!tbl)
>  			continue;

Later statements:

		p = list_next_entry(&tbl->parms, list);
		list_for_each_entry_from(p, &tbl->parms_list, list) {

are now without any protection, and AFAICS parms_list is write protected
by tbl->lock. I think it's necessary to move the
read_lock_bh(&tbl->lock) from neightbl_fill_param_info() to
neightbl_dump_info() ?!?

Side note: I guess it would be to follow-up replacing R/W lock with
plain spinlock/rcu?!?

Thanks!

Paolo

Re: [PATCH v2 net-next 6/7] neighbour: Convert RTM_GETNEIGHTBL to RCU.

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 24 Apr 2025 10:19:17 +0200
> On 4/18/25 3:26 AM, Kuniyuki Iwashima wrote:
> > neightbl_dump_info() calls neightbl_fill_info() and
> 
> AFAICS neightbl_fill_info() is only invoked from neightbl_dump_info()
> and acquires/releases the RCU internally. Such lock pair should be dropped.

Maybe I got lost, which lock do you mean ??


> 
> > neightbl_fill_param_info() for each neigh_tables[] entry.
> > 
> > Both functions rely on the table lock (read_lock_bh(&tbl->lock)),
> > so RTNL is not needed.
> > 
> > Let's fetch the table under RCU and convert RTM_GETNEIGHTBL to RCU.
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  net/core/neighbour.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index ccfef86bb044..38732d8f0ed7 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -2467,10 +2467,12 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> >  
> >  	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
> >  
> > +	rcu_read_lock();
> > +
> >  	for (tidx = 0; tidx < NEIGH_NR_TABLES; tidx++) {
> >  		struct neigh_parms *p;
> >  
> > -		tbl = rcu_dereference_rtnl(neigh_tables[tidx]);
> > +		tbl = rcu_dereference(neigh_tables[tidx]);
> >  		if (!tbl)
> >  			continue;
> 
> Later statements:
> 
> 		p = list_next_entry(&tbl->parms, list);
> 		list_for_each_entry_from(p, &tbl->parms_list, list) {
> 
> are now without any protection, and AFAICS parms_list is write protected
> by tbl->lock. I think it's necessary to move the
> read_lock_bh(&tbl->lock) from neightbl_fill_param_info() to
> neightbl_dump_info() ?!?

Oh, thanks for catching this!

Maybe we should use list_add/del_rcu() instead ?
given neigh_parms_release() frees it after RCU.

> 
> Side note: I guess it would be to follow-up replacing R/W lock with
> plain spinlock/rcu?!?

Yes, it's on my todo list ;)

[PATCH v2 net-next 7/7] neighbour: Convert RTM_SETNEIGHTBL to RCU.

[Kuniyuki Iwashima]

neightbl_set() fetches neigh_tables[] and updates attributes under
write_lock_bh(&tbl->lock), so RTNL is not needed.

Let's fetch the table under RCU and drop RTNL for RTM_SETNEIGHTBL.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/neighbour.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 38732d8f0ed7..09894e559244 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2281,8 +2281,10 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	ndtmsg = nlmsg_data(nlh);
 
+	rcu_read_lock();
+
 	for (tidx = 0; tidx < NEIGH_NR_TABLES; tidx++) {
-		tbl = rcu_dereference_rtnl(neigh_tables[tidx]);
+		tbl = rcu_dereference(neigh_tables[tidx]);
 		if (!tbl)
 			continue;
 		if (ndtmsg->ndtm_family && tbl->family != ndtmsg->ndtm_family)
@@ -2293,8 +2295,10 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 	}
 
-	if (!found)
-		return -ENOENT;
+	if (!found) {
+		err = -ENOENT;
+		goto errout_rcu;
+	}
 
 	/*
 	 * We acquire tbl->lock to be nice to the periodic timers and
@@ -2421,6 +2425,8 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 errout_tbl_lock:
 	write_unlock_bh(&tbl->lock);
+errout_rcu:
+	rcu_read_unlock();
 errout:
 	return err;
 }
@@ -3817,7 +3823,8 @@ static const struct rtnl_msg_handler neigh_rtnl_msg_handlers[] __initconst = {
 	 .flags = RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED},
 	{.msgtype = RTM_GETNEIGHTBL, .dumpit = neightbl_dump_info,
 	 .flags = RTNL_FLAG_DUMP_UNLOCKED},
-	{.msgtype = RTM_SETNEIGHTBL, .doit = neightbl_set},
+	{.msgtype = RTM_SETNEIGHTBL, .doit = neightbl_set,
+	 .flags = RTNL_FLAG_DOIT_UNLOCKED},
 };
 
 static int __init neigh_init(void)
-- 
2.49.0