[PATCH v1 net-next 0/9] phonet: Convert all doit() and dumpit() to RCU.

[PATCH v1 net-next 0/9] phonet: Convert all doit() and dumpit() to RCU.

[Kuniyuki Iwashima]

addr_doit() and route_doit() access only phonet_device_list(dev_net(dev))
and phonet_pernet(dev_net(dev))->routes, respectively.

Each per-netns struct has its dedicated mutex, and RTNL also protects
the structs.  __dev_change_net_namespace() has synchronize_net(), so
we have two options to convert addr_doit() and route_doit().

  1. Use per-netns RTNL
  2. Use RCU and convert each struct mutex to spinlock_t

As RCU is preferable, this series converts all PF_PHONET's doit()
and dumpit() to RCU.

4 doit()s and 1 dumpit() are now converted to RCU, 70 doit()s and
28 dumpit()s are still under RTNL.


Kuniyuki Iwashima (9):
  phonet: Pass ifindex to fill_addr().
  phonet: Pass net and ifindex to phonet_address_notify().
  phonet: Convert phonet_device_list.lock to spinlock_t.
  phonet: Don't hold RTNL for addr_doit().
  phonet: Don't hold RTNL for getaddr_dumpit().
  phonet: Pass ifindex to fill_route().
  phonet: Pass net and ifindex to rtm_phonet_notify().
  phonet: Convert phonet_routes.lock to spinlock_t.
  phonet: Don't hold RTNL for route_doit().

 include/net/phonet/pn_dev.h |   8 +--
 net/phonet/pn_dev.c         |  69 ++++++++++++++--------
 net/phonet/pn_netlink.c     | 115 ++++++++++++++++++++++--------------
 3 files changed, 120 insertions(+), 72 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 1/9] phonet: Pass ifindex to fill_addr().

[Kuniyuki Iwashima]

We will convert addr_doit() and getaddr_dumpit() to RCU, both
of which call fill_addr().

The former will call phonet_address_notify() outside of RCU
due to GFP_KERNEL, so dev will not be available in fill_addr().

Let's pass ifindex directly to fill_addr().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/phonet/pn_netlink.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 894e5c72d6bf..3205d2457477 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -19,7 +19,7 @@
 
 /* Device address handling */
 
-static int fill_addr(struct sk_buff *skb, struct net_device *dev, u8 addr,
+static int fill_addr(struct sk_buff *skb, u32 ifindex, u8 addr,
 		     u32 portid, u32 seq, int event);
 
 void phonet_address_notify(int event, struct net_device *dev, u8 addr)
@@ -31,7 +31,8 @@ void phonet_address_notify(int event, struct net_device *dev, u8 addr)
 			nla_total_size(1), GFP_KERNEL);
 	if (skb == NULL)
 		goto errout;
-	err = fill_addr(skb, dev, addr, 0, 0, event);
+
+	err = fill_addr(skb, dev->ifindex, addr, 0, 0, event);
 	if (err < 0) {
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
@@ -92,8 +93,8 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-static int fill_addr(struct sk_buff *skb, struct net_device *dev, u8 addr,
-			u32 portid, u32 seq, int event)
+static int fill_addr(struct sk_buff *skb, u32 ifindex, u8 addr,
+		     u32 portid, u32 seq, int event)
 {
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr *nlh;
@@ -107,7 +108,7 @@ static int fill_addr(struct sk_buff *skb, struct net_device *dev, u8 addr,
 	ifm->ifa_prefixlen = 0;
 	ifm->ifa_flags = IFA_F_PERMANENT;
 	ifm->ifa_scope = RT_SCOPE_LINK;
-	ifm->ifa_index = dev->ifindex;
+	ifm->ifa_index = ifindex;
 	if (nla_put_u8(skb, IFA_LOCAL, addr))
 		goto nla_put_failure;
 	nlmsg_end(skb, nlh);
@@ -140,7 +141,7 @@ static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			if (addr_idx++ < addr_start_idx)
 				continue;
 
-			if (fill_addr(skb, pnd->netdev, addr << 2,
+			if (fill_addr(skb, pnd->netdev->ifindex, addr << 2,
 					 NETLINK_CB(cb->skb).portid,
 					cb->nlh->nlmsg_seq, RTM_NEWADDR) < 0)
 				goto out;
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 2/9] phonet: Pass net and ifindex to phonet_address_notify().

[Kuniyuki Iwashima]

Currently, phonet_address_notify() fetches netns and ifindex from dev.

Once addr_doit() is converted to RCU, phonet_address_notify() will be
called outside of RCU due to GFP_KERNEL, and dev will be unavailable
there.

Let's pass net and ifindex to phonet_address_notify().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/phonet/pn_dev.h |  2 +-
 net/phonet/pn_dev.c         | 10 +++++++---
 net/phonet/pn_netlink.c     | 12 ++++++------
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
index e9dc8dca5817..6b2102b4ece3 100644
--- a/include/net/phonet/pn_dev.h
+++ b/include/net/phonet/pn_dev.h
@@ -38,7 +38,7 @@ int phonet_address_add(struct net_device *dev, u8 addr);
 int phonet_address_del(struct net_device *dev, u8 addr);
 u8 phonet_address_get(struct net_device *dev, u8 addr);
 int phonet_address_lookup(struct net *net, u8 addr);
-void phonet_address_notify(int event, struct net_device *dev, u8 addr);
+void phonet_address_notify(struct net *net, int event, u32 ifindex, u8 addr);
 
 int phonet_route_add(struct net_device *dev, u8 daddr);
 int phonet_route_del(struct net_device *dev, u8 daddr);
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
index cde671d29d5d..2e7d850dc726 100644
--- a/net/phonet/pn_dev.c
+++ b/net/phonet/pn_dev.c
@@ -98,10 +98,13 @@ static void phonet_device_destroy(struct net_device *dev)
 	mutex_unlock(&pndevs->lock);
 
 	if (pnd) {
+		struct net *net = dev_net(dev);
+		u32 ifindex = dev->ifindex;
 		u8 addr;
 
 		for_each_set_bit(addr, pnd->addrs, 64)
-			phonet_address_notify(RTM_DELADDR, dev, addr);
+			phonet_address_notify(net, RTM_DELADDR, ifindex, addr);
+
 		kfree(pnd);
 	}
 }
@@ -244,8 +247,9 @@ static int phonet_device_autoconf(struct net_device *dev)
 	ret = phonet_address_add(dev, req.ifr_phonet_autoconf.device);
 	if (ret)
 		return ret;
-	phonet_address_notify(RTM_NEWADDR, dev,
-				req.ifr_phonet_autoconf.device);
+
+	phonet_address_notify(dev_net(dev), RTM_NEWADDR, dev->ifindex,
+			      req.ifr_phonet_autoconf.device);
 	return 0;
 }
 
diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 3205d2457477..23097085ad38 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -22,7 +22,7 @@
 static int fill_addr(struct sk_buff *skb, u32 ifindex, u8 addr,
 		     u32 portid, u32 seq, int event);
 
-void phonet_address_notify(int event, struct net_device *dev, u8 addr)
+void phonet_address_notify(struct net *net, int event, u32 ifindex, u8 addr)
 {
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
@@ -32,17 +32,17 @@ void phonet_address_notify(int event, struct net_device *dev, u8 addr)
 	if (skb == NULL)
 		goto errout;
 
-	err = fill_addr(skb, dev->ifindex, addr, 0, 0, event);
+	err = fill_addr(skb, ifindex, addr, 0, 0, event);
 	if (err < 0) {
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
 		goto errout;
 	}
-	rtnl_notify(skb, dev_net(dev), 0,
-		    RTNLGRP_PHONET_IFADDR, NULL, GFP_KERNEL);
+
+	rtnl_notify(skb, net, 0, RTNLGRP_PHONET_IFADDR, NULL, GFP_KERNEL);
 	return;
 errout:
-	rtnl_set_sk_err(dev_net(dev), RTNLGRP_PHONET_IFADDR, err);
+	rtnl_set_sk_err(net, RTNLGRP_PHONET_IFADDR, err);
 }
 
 static const struct nla_policy ifa_phonet_policy[IFA_MAX+1] = {
@@ -89,7 +89,7 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else
 		err = phonet_address_del(dev, pnaddr);
 	if (!err)
-		phonet_address_notify(nlh->nlmsg_type, dev, pnaddr);
+		phonet_address_notify(net, nlh->nlmsg_type, ifm->ifa_index, pnaddr);
 	return err;
 }
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 3/9] phonet: Convert phonet_device_list.lock to spinlock_t.

[Kuniyuki Iwashima]

addr_doit() calls phonet_address_add() or phonet_address_del()
for RTM_NEWADDR or RTM_DELADDR, respectively.

Both functions only touch phonet_device_list(dev_net(dev)),
which is currently protected by RTNL and its dedicated mutex,
phonet_device_list.lock.

We will convert addr_doit() to RCU and cannot use mutex inside RCU.

Let's convert the mutex to spinlock_t.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/phonet/pn_dev.h |  3 ++-
 net/phonet/pn_dev.c         | 26 +++++++++++++++++---------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
index 6b2102b4ece3..ac0331d83a81 100644
--- a/include/net/phonet/pn_dev.h
+++ b/include/net/phonet/pn_dev.h
@@ -12,12 +12,13 @@
 
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 
 struct net;
 
 struct phonet_device_list {
 	struct list_head list;
-	struct mutex lock;
+	spinlock_t lock;
 };
 
 struct phonet_device_list *phonet_device_list(struct net *net);
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
index 2e7d850dc726..545279ef5910 100644
--- a/net/phonet/pn_dev.c
+++ b/net/phonet/pn_dev.c
@@ -54,7 +54,7 @@ static struct phonet_device *__phonet_device_alloc(struct net_device *dev)
 	pnd->netdev = dev;
 	bitmap_zero(pnd->addrs, 64);
 
-	BUG_ON(!mutex_is_locked(&pndevs->lock));
+	lockdep_assert_held(&pndevs->lock);
 	list_add_rcu(&pnd->list, &pndevs->list);
 	return pnd;
 }
@@ -64,7 +64,8 @@ static struct phonet_device *__phonet_get(struct net_device *dev)
 	struct phonet_device_list *pndevs = phonet_device_list(dev_net(dev));
 	struct phonet_device *pnd;
 
-	BUG_ON(!mutex_is_locked(&pndevs->lock));
+	lockdep_assert_held(&pndevs->lock);
+
 	list_for_each_entry(pnd, &pndevs->list, list) {
 		if (pnd->netdev == dev)
 			return pnd;
@@ -91,11 +92,13 @@ static void phonet_device_destroy(struct net_device *dev)
 
 	ASSERT_RTNL();
 
-	mutex_lock(&pndevs->lock);
+	spin_lock(&pndevs->lock);
+
 	pnd = __phonet_get(dev);
 	if (pnd)
 		list_del_rcu(&pnd->list);
-	mutex_unlock(&pndevs->lock);
+
+	spin_unlock(&pndevs->lock);
 
 	if (pnd) {
 		struct net *net = dev_net(dev);
@@ -136,7 +139,8 @@ int phonet_address_add(struct net_device *dev, u8 addr)
 	struct phonet_device *pnd;
 	int err = 0;
 
-	mutex_lock(&pndevs->lock);
+	spin_lock(&pndevs->lock);
+
 	/* Find or create Phonet-specific device data */
 	pnd = __phonet_get(dev);
 	if (pnd == NULL)
@@ -145,7 +149,9 @@ int phonet_address_add(struct net_device *dev, u8 addr)
 		err = -ENOMEM;
 	else if (test_and_set_bit(addr >> 2, pnd->addrs))
 		err = -EEXIST;
-	mutex_unlock(&pndevs->lock);
+
+	spin_unlock(&pndevs->lock);
+
 	return err;
 }
 
@@ -155,7 +161,8 @@ int phonet_address_del(struct net_device *dev, u8 addr)
 	struct phonet_device *pnd;
 	int err = 0;
 
-	mutex_lock(&pndevs->lock);
+	spin_lock(&pndevs->lock);
+
 	pnd = __phonet_get(dev);
 	if (!pnd || !test_and_clear_bit(addr >> 2, pnd->addrs)) {
 		err = -EADDRNOTAVAIL;
@@ -164,7 +171,8 @@ int phonet_address_del(struct net_device *dev, u8 addr)
 		list_del_rcu(&pnd->list);
 	else
 		pnd = NULL;
-	mutex_unlock(&pndevs->lock);
+
+	spin_unlock(&pndevs->lock);
 
 	if (pnd)
 		kfree_rcu(pnd, rcu);
@@ -313,7 +321,7 @@ static int __net_init phonet_init_net(struct net *net)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&pnn->pndevs.list);
-	mutex_init(&pnn->pndevs.lock);
+	spin_lock_init(&pnn->pndevs.lock);
 	mutex_init(&pnn->routes.lock);
 	return 0;
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 4/9] phonet: Don't hold RTNL for addr_doit().

[Kuniyuki Iwashima]

Now only __dev_get_by_index() depends on RTNL in addr_doit().

Let's use dev_get_by_index_rcu() and register addr_doit() with
RTNL_FLAG_DOIT_UNLOCKED.

While at it, I changed phonet_rtnl_msg_handlers[]'s init to C99
style like other core networking code.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/phonet/pn_netlink.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 23097085ad38..5996141e258f 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -65,8 +65,6 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!netlink_capable(skb, CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ASSERT_RTNL();
-
 	err = nlmsg_parse_deprecated(nlh, sizeof(*ifm), tb, IFA_MAX,
 				     ifa_phonet_policy, extack);
 	if (err < 0)
@@ -80,16 +78,24 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		/* Phonet addresses only have 6 high-order bits */
 		return -EINVAL;
 
-	dev = __dev_get_by_index(net, ifm->ifa_index);
-	if (dev == NULL)
+	rcu_read_lock();
+
+	dev = dev_get_by_index_rcu(net, ifm->ifa_index);
+	if (!dev) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	if (nlh->nlmsg_type == RTM_NEWADDR)
 		err = phonet_address_add(dev, pnaddr);
 	else
 		err = phonet_address_del(dev, pnaddr);
+
+	rcu_read_unlock();
+
 	if (!err)
 		phonet_address_notify(net, nlh->nlmsg_type, ifm->ifa_index, pnaddr);
+
 	return err;
 }
 
@@ -287,13 +293,18 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 static const struct rtnl_msg_handler phonet_rtnl_msg_handlers[] __initdata_or_module = {
-	{THIS_MODULE, PF_PHONET, RTM_NEWADDR, addr_doit, NULL, 0},
-	{THIS_MODULE, PF_PHONET, RTM_DELADDR, addr_doit, NULL, 0},
-	{THIS_MODULE, PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0},
-	{THIS_MODULE, PF_PHONET, RTM_NEWROUTE, route_doit, NULL, 0},
-	{THIS_MODULE, PF_PHONET, RTM_DELROUTE, route_doit, NULL, 0},
-	{THIS_MODULE, PF_PHONET, RTM_GETROUTE, NULL, route_dumpit,
-	 RTNL_FLAG_DUMP_UNLOCKED},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_NEWADDR,
+	 .doit = addr_doit, .flags = RTNL_FLAG_DOIT_UNLOCKED},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_DELADDR,
+	 .doit = addr_doit, .flags = RTNL_FLAG_DOIT_UNLOCKED},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_GETADDR,
+	 .dumpit = getaddr_dumpit},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_NEWROUTE,
+	 .doit = route_doit},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_DELROUTE,
+	 .doit = route_doit},
+	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_GETROUTE,
+	 .dumpit = route_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
 };
 
 int __init phonet_netlink_register(void)
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Kuniyuki Iwashima]

getaddr_dumpit() already relies on RCU and does not need RTNL.

Let's use READ_ONCE() for ifindex and register getaddr_dumpit()
with RTNL_FLAG_DUMP_UNLOCKED.

While at it, the retval of getaddr_dumpit() is changed to combine
NLMSG_DONE and save recvmsg() as done in 58a4ff5d77b1 ("phonet: no
longer hold RTNL in route_dumpit()").

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/phonet/pn_netlink.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 5996141e258f..14928fa04675 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32 ifindex, u8 addr,
 
 static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
+	int addr_idx = 0, addr_start_idx = cb->args[1];
+	int dev_idx = 0, dev_start_idx = cb->args[0];
 	struct phonet_device_list *pndevs;
 	struct phonet_device *pnd;
-	int dev_idx = 0, dev_start_idx = cb->args[0];
-	int addr_idx = 0, addr_start_idx = cb->args[1];
+	int err = 0;
 
 	pndevs = phonet_device_list(sock_net(skb->sk));
+
 	rcu_read_lock();
 	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
+		DECLARE_BITMAP(addrs, 64);
 		u8 addr;
 
 		if (dev_idx > dev_start_idx)
@@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			continue;
 
 		addr_idx = 0;
-		for_each_set_bit(addr, pnd->addrs, 64) {
+		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
+
+		for_each_set_bit(addr, addrs, 64) {
 			if (addr_idx++ < addr_start_idx)
 				continue;
 
-			if (fill_addr(skb, pnd->netdev->ifindex, addr << 2,
-					 NETLINK_CB(cb->skb).portid,
-					cb->nlh->nlmsg_seq, RTM_NEWADDR) < 0)
+			err = fill_addr(skb, READ_ONCE(pnd->netdev->ifindex),
+					addr << 2, NETLINK_CB(cb->skb).portid,
+					cb->nlh->nlmsg_seq, RTM_NEWADDR);
+			if (err < 0)
 				goto out;
 		}
 	}
-
 out:
 	rcu_read_unlock();
+
 	cb->args[0] = dev_idx;
 	cb->args[1] = addr_idx;
 
-	return skb->len;
+	return err;
 }
 
 /* Routes handling */
@@ -298,7 +304,7 @@ static const struct rtnl_msg_handler phonet_rtnl_msg_handlers[] __initdata_or_mo
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_DELADDR,
 	 .doit = addr_doit, .flags = RTNL_FLAG_DOIT_UNLOCKED},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_GETADDR,
-	 .dumpit = getaddr_dumpit},
+	 .dumpit = getaddr_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_NEWROUTE,
 	 .doit = route_doit},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_DELROUTE,
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 6/9] phonet: Pass ifindex to fill_route().

[Kuniyuki Iwashima]

We will convert route_doit() to RCU.

route_doit() will call rtm_phonet_notify() outside of RCU due
to GFP_KERNEL, so dev will not be available in fill_route().

Let's pass ifindex directly to fill_route().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/phonet/pn_netlink.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 14928fa04675..c9a4215ec560 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -170,8 +170,8 @@ static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 /* Routes handling */
 
-static int fill_route(struct sk_buff *skb, struct net_device *dev, u8 dst,
-			u32 portid, u32 seq, int event)
+static int fill_route(struct sk_buff *skb, u32 ifindex, u8 dst,
+		      u32 portid, u32 seq, int event)
 {
 	struct rtmsg *rtm;
 	struct nlmsghdr *nlh;
@@ -190,8 +190,7 @@ static int fill_route(struct sk_buff *skb, struct net_device *dev, u8 dst,
 	rtm->rtm_scope = RT_SCOPE_UNIVERSE;
 	rtm->rtm_type = RTN_UNICAST;
 	rtm->rtm_flags = 0;
-	if (nla_put_u8(skb, RTA_DST, dst) ||
-	    nla_put_u32(skb, RTA_OIF, READ_ONCE(dev->ifindex)))
+	if (nla_put_u8(skb, RTA_DST, dst) || nla_put_u32(skb, RTA_OIF, ifindex))
 		goto nla_put_failure;
 	nlmsg_end(skb, nlh);
 	return 0;
@@ -210,7 +209,8 @@ void rtm_phonet_notify(int event, struct net_device *dev, u8 dst)
 			nla_total_size(1) + nla_total_size(4), GFP_KERNEL);
 	if (skb == NULL)
 		goto errout;
-	err = fill_route(skb, dev, dst, 0, 0, event);
+
+	err = fill_route(skb, dev->ifindex, dst, 0, 0, event);
 	if (err < 0) {
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
@@ -286,7 +286,7 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 		if (!dev)
 			continue;
 
-		err = fill_route(skb, dev, addr << 2,
+		err = fill_route(skb, READ_ONCE(dev->ifindex), addr << 2,
 				 NETLINK_CB(cb->skb).portid,
 				 cb->nlh->nlmsg_seq, RTM_NEWROUTE);
 		if (err < 0)
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 7/9] phonet: Pass net and ifindex to rtm_phonet_notify().

[Kuniyuki Iwashima]

Currently, rtm_phonet_notify() fetches netns and ifindex from dev.

Once route_doit() is converted to RCU, rtm_phonet_notify() will be
called outside of RCU due to GFP_KERNEL, and dev will be unavailable
there.

Let's pass net and ifindex to rtm_phonet_notify().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/phonet/pn_dev.h |  2 +-
 net/phonet/pn_dev.c         | 10 +++++++---
 net/phonet/pn_netlink.c     | 16 +++++++++-------
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
index ac0331d83a81..021e524fd20a 100644
--- a/include/net/phonet/pn_dev.h
+++ b/include/net/phonet/pn_dev.h
@@ -43,7 +43,7 @@ void phonet_address_notify(struct net *net, int event, u32 ifindex, u8 addr);
 
 int phonet_route_add(struct net_device *dev, u8 daddr);
 int phonet_route_del(struct net_device *dev, u8 daddr);
-void rtm_phonet_notify(int event, struct net_device *dev, u8 dst);
+void rtm_phonet_notify(struct net *net, int event, u32 ifindex, u8 dst);
 struct net_device *phonet_route_get_rcu(struct net *net, u8 daddr);
 struct net_device *phonet_route_output(struct net *net, u8 daddr);
 
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
index 545279ef5910..6ded0d347b9f 100644
--- a/net/phonet/pn_dev.c
+++ b/net/phonet/pn_dev.c
@@ -263,9 +263,13 @@ static int phonet_device_autoconf(struct net_device *dev)
 
 static void phonet_route_autodel(struct net_device *dev)
 {
-	struct phonet_net *pnn = phonet_pernet(dev_net(dev));
-	unsigned int i;
+	struct net *net = dev_net(dev);
 	DECLARE_BITMAP(deleted, 64);
+	u32 ifindex = dev->ifindex;
+	struct phonet_net *pnn;
+	unsigned int i;
+
+	pnn = phonet_pernet(net);
 
 	/* Remove left-over Phonet routes */
 	bitmap_zero(deleted, 64);
@@ -281,7 +285,7 @@ static void phonet_route_autodel(struct net_device *dev)
 		return; /* short-circuit RCU */
 	synchronize_rcu();
 	for_each_set_bit(i, deleted, 64) {
-		rtm_phonet_notify(RTM_DELROUTE, dev, i);
+		rtm_phonet_notify(net, RTM_DELROUTE, ifindex, i);
 		dev_put(dev);
 	}
 }
diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index c9a4215ec560..bfec5bd639b6 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -200,7 +200,7 @@ static int fill_route(struct sk_buff *skb, u32 ifindex, u8 dst,
 	return -EMSGSIZE;
 }
 
-void rtm_phonet_notify(int event, struct net_device *dev, u8 dst)
+void rtm_phonet_notify(struct net *net, int event, u32 ifindex, u8 dst)
 {
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
@@ -210,17 +210,17 @@ void rtm_phonet_notify(int event, struct net_device *dev, u8 dst)
 	if (skb == NULL)
 		goto errout;
 
-	err = fill_route(skb, dev->ifindex, dst, 0, 0, event);
+	err = fill_route(skb, ifindex, dst, 0, 0, event);
 	if (err < 0) {
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
 		goto errout;
 	}
-	rtnl_notify(skb, dev_net(dev), 0,
-			  RTNLGRP_PHONET_ROUTE, NULL, GFP_KERNEL);
+
+	rtnl_notify(skb, net, 0, RTNLGRP_PHONET_ROUTE, NULL, GFP_KERNEL);
 	return;
 errout:
-	rtnl_set_sk_err(dev_net(dev), RTNLGRP_PHONET_ROUTE, err);
+	rtnl_set_sk_err(net, RTNLGRP_PHONET_ROUTE, err);
 }
 
 static const struct nla_policy rtm_phonet_policy[RTA_MAX+1] = {
@@ -235,6 +235,7 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct nlattr *tb[RTA_MAX+1];
 	struct net_device *dev;
 	struct rtmsg *rtm;
+	u32 ifindex;
 	int err;
 	u8 dst;
 
@@ -260,7 +261,8 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (dst & 3) /* Phonet addresses only have 6 high-order bits */
 		return -EINVAL;
 
-	dev = __dev_get_by_index(net, nla_get_u32(tb[RTA_OIF]));
+	ifindex = nla_get_u32(tb[RTA_OIF]);
+	dev = __dev_get_by_index(net, ifindex);
 	if (dev == NULL)
 		return -ENODEV;
 
@@ -269,7 +271,7 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else
 		err = phonet_route_del(dev, dst);
 	if (!err)
-		rtm_phonet_notify(nlh->nlmsg_type, dev, dst);
+		rtm_phonet_notify(net, nlh->nlmsg_type, ifindex, dst);
 	return err;
 }
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 8/9] phonet: Convert phonet_routes.lock to spinlock_t.

[Kuniyuki Iwashima]

route_doit() calls phonet_route_add() or phonet_route_del()
for RTM_NEWROUTE or RTM_DELROUTE, respectively.

Both functions only touch phonet_pernet(dev_net(dev))->routes,
which is currently protected by RTNL and its dedicated mutex,
phonet_routes.lock.

We will convert route_doit() to RCU and cannot use mutex inside RCU.

Let's convert the mutex to spinlock_t.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/phonet/pn_dev.h |  1 -
 net/phonet/pn_dev.c         | 23 ++++++++++++++---------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h
index 021e524fd20a..37a3e83531c6 100644
--- a/include/net/phonet/pn_dev.h
+++ b/include/net/phonet/pn_dev.h
@@ -11,7 +11,6 @@
 #define PN_DEV_H
 
 #include <linux/list.h>
-#include <linux/mutex.h>
 #include <linux/spinlock.h>
 
 struct net;
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
index 6ded0d347b9f..19234d664c4f 100644
--- a/net/phonet/pn_dev.c
+++ b/net/phonet/pn_dev.c
@@ -22,7 +22,7 @@
 #include <net/phonet/pn_dev.h>
 
 struct phonet_routes {
-	struct mutex		lock;
+	spinlock_t		lock;
 	struct net_device __rcu	*table[64];
 };
 
@@ -273,13 +273,15 @@ static void phonet_route_autodel(struct net_device *dev)
 
 	/* Remove left-over Phonet routes */
 	bitmap_zero(deleted, 64);
-	mutex_lock(&pnn->routes.lock);
-	for (i = 0; i < 64; i++)
+
+	spin_lock(&pnn->routes.lock);
+	for (i = 0; i < 64; i++) {
 		if (rcu_access_pointer(pnn->routes.table[i]) == dev) {
 			RCU_INIT_POINTER(pnn->routes.table[i], NULL);
 			set_bit(i, deleted);
 		}
-	mutex_unlock(&pnn->routes.lock);
+	}
+	spin_unlock(&pnn->routes.lock);
 
 	if (bitmap_empty(deleted, 64))
 		return; /* short-circuit RCU */
@@ -326,7 +328,7 @@ static int __net_init phonet_init_net(struct net *net)
 
 	INIT_LIST_HEAD(&pnn->pndevs.list);
 	spin_lock_init(&pnn->pndevs.lock);
-	mutex_init(&pnn->routes.lock);
+	spin_lock_init(&pnn->routes.lock);
 	return 0;
 }
 
@@ -376,13 +378,15 @@ int phonet_route_add(struct net_device *dev, u8 daddr)
 	int err = -EEXIST;
 
 	daddr = daddr >> 2;
-	mutex_lock(&routes->lock);
+
+	spin_lock(&routes->lock);
 	if (routes->table[daddr] == NULL) {
 		rcu_assign_pointer(routes->table[daddr], dev);
 		dev_hold(dev);
 		err = 0;
 	}
-	mutex_unlock(&routes->lock);
+	spin_unlock(&routes->lock);
+
 	return err;
 }
 
@@ -392,12 +396,13 @@ int phonet_route_del(struct net_device *dev, u8 daddr)
 	struct phonet_routes *routes = &pnn->routes;
 
 	daddr = daddr >> 2;
-	mutex_lock(&routes->lock);
+
+	spin_lock(&routes->lock);
 	if (rcu_access_pointer(routes->table[daddr]) == dev)
 		RCU_INIT_POINTER(routes->table[daddr], NULL);
 	else
 		dev = NULL;
-	mutex_unlock(&routes->lock);
+	spin_unlock(&routes->lock);
 
 	if (!dev)
 		return -ENOENT;
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 9/9] phonet: Don't hold RTNL for route_doit().

[Kuniyuki Iwashima]

Now only __dev_get_by_index() depends on RTNL in route_doit().

Let's use dev_get_by_index_rcu() and register route_doit() with
RTNL_FLAG_DOIT_UNLOCKED.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/phonet/pn_netlink.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index bfec5bd639b6..ca1f04e4a2d9 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -245,8 +245,6 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!netlink_capable(skb, CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ASSERT_RTNL();
-
 	err = nlmsg_parse_deprecated(nlh, sizeof(*rtm), tb, RTA_MAX,
 				     rtm_phonet_policy, extack);
 	if (err < 0)
@@ -262,16 +260,25 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 
 	ifindex = nla_get_u32(tb[RTA_OIF]);
-	dev = __dev_get_by_index(net, ifindex);
-	if (dev == NULL)
+
+	rcu_read_lock();
+
+	dev = dev_get_by_index_rcu(net, ifindex);
+	if (!dev) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
 	if (nlh->nlmsg_type == RTM_NEWROUTE)
 		err = phonet_route_add(dev, dst);
 	else
 		err = phonet_route_del(dev, dst);
+
+	rcu_read_unlock();
+
 	if (!err)
 		rtm_phonet_notify(net, nlh->nlmsg_type, ifindex, dst);
+
 	return err;
 }
 
@@ -308,9 +315,9 @@ static const struct rtnl_msg_handler phonet_rtnl_msg_handlers[] __initdata_or_mo
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_GETADDR,
 	 .dumpit = getaddr_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_NEWROUTE,
-	 .doit = route_doit},
+	 .doit = route_doit, .flags = RTNL_FLAG_DOIT_UNLOCKED},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_DELROUTE,
-	 .doit = route_doit},
+	 .doit = route_doit, .flags = RTNL_FLAG_DOIT_UNLOCKED},
 	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = RTM_GETROUTE,
 	 .dumpit = route_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
 };
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Rémi Denis-Courmont]

Le torstaina 17. lokakuuta 2024, 21.31.36 EEST Kuniyuki Iwashima a écrit :
> getaddr_dumpit() already relies on RCU and does not need RTNL.
> 
> Let's use READ_ONCE() for ifindex and register getaddr_dumpit()
> with RTNL_FLAG_DUMP_UNLOCKED.
> 
> While at it, the retval of getaddr_dumpit() is changed to combine
> NLMSG_DONE and save recvmsg() as done in 58a4ff5d77b1 ("phonet: no
> longer hold RTNL in route_dumpit()").
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/phonet/pn_netlink.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
> index 5996141e258f..14928fa04675 100644
> --- a/net/phonet/pn_netlink.c
> +++ b/net/phonet/pn_netlink.c
> @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32 ifindex,
> u8 addr,
> 
>  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> {
> +	int addr_idx = 0, addr_start_idx = cb->args[1];
> +	int dev_idx = 0, dev_start_idx = cb->args[0];
>  	struct phonet_device_list *pndevs;
>  	struct phonet_device *pnd;
> -	int dev_idx = 0, dev_start_idx = cb->args[0];
> -	int addr_idx = 0, addr_start_idx = cb->args[1];
> +	int err = 0;
> 
>  	pndevs = phonet_device_list(sock_net(skb->sk));
> +
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
> +		DECLARE_BITMAP(addrs, 64);
>  		u8 addr;
> 
>  		if (dev_idx > dev_start_idx)
> @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb, struct
> netlink_callback *cb) continue;
> 
>  		addr_idx = 0;
> -		for_each_set_bit(addr, pnd->addrs, 64) {
> +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));

Is that really safe? Are we sure that the bit-field writers are atomic w.r.t. 
memcpy() on all platforms? If READ_ONCE is needed for an integer, using 
memcpy() seems sketchy, TBH.

> +
> +		for_each_set_bit(addr, addrs, 64) {
>  			if (addr_idx++ < addr_start_idx)
>  				continue;
> 
> -			if (fill_addr(skb, pnd->netdev->ifindex, addr 
<< 2,
> -					 NETLINK_CB(cb-
>skb).portid,
> -					cb->nlh->nlmsg_seq, 
RTM_NEWADDR) < 0)
> +			err = fill_addr(skb, READ_ONCE(pnd->netdev-
>ifindex),
> +					addr << 2, 
NETLINK_CB(cb->skb).portid,
> +					cb->nlh->nlmsg_seq, 
RTM_NEWADDR);
> +			if (err < 0)
>  				goto out;
>  		}
>  	}
> -
>  out:
>  	rcu_read_unlock();
> +
>  	cb->args[0] = dev_idx;
>  	cb->args[1] = addr_idx;
> 
> -	return skb->len;
> +	return err;
>  }
> 
>  /* Routes handling */
> @@ -298,7 +304,7 @@ static const struct rtnl_msg_handler
> phonet_rtnl_msg_handlers[] __initdata_or_mo {.owner = THIS_MODULE,
> .protocol = PF_PHONET, .msgtype = RTM_DELADDR, .doit = addr_doit, .flags =
> RTNL_FLAG_DOIT_UNLOCKED},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_GETADDR,
> -	 .dumpit = getaddr_dumpit},
> +	 .dumpit = getaddr_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_NEWROUTE,
>  	 .doit = route_doit},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_DELROUTE,


-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Kuniyuki Iwashima]

From: "Rémi Denis-Courmont" <remi@remlab.net>
Date: Thu, 17 Oct 2024 21:49:18 +0300
> > diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
> > index 5996141e258f..14928fa04675 100644
> > --- a/net/phonet/pn_netlink.c
> > +++ b/net/phonet/pn_netlink.c
> > @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32 ifindex,
> > u8 addr,
> > 
> >  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> > {
> > +	int addr_idx = 0, addr_start_idx = cb->args[1];
> > +	int dev_idx = 0, dev_start_idx = cb->args[0];
> >  	struct phonet_device_list *pndevs;
> >  	struct phonet_device *pnd;
> > -	int dev_idx = 0, dev_start_idx = cb->args[0];
> > -	int addr_idx = 0, addr_start_idx = cb->args[1];
> > +	int err = 0;
> > 
> >  	pndevs = phonet_device_list(sock_net(skb->sk));
> > +
> >  	rcu_read_lock();
> >  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
> > +		DECLARE_BITMAP(addrs, 64);
> >  		u8 addr;
> > 
> >  		if (dev_idx > dev_start_idx)
> > @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb, struct
> > netlink_callback *cb) continue;
> > 
> >  		addr_idx = 0;
> > -		for_each_set_bit(addr, pnd->addrs, 64) {
> > +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
> 
> Is that really safe? Are we sure that the bit-field writers are atomic w.r.t. 
> memcpy() on all platforms? If READ_ONCE is needed for an integer, using 
> memcpy() seems sketchy, TBH.

I think bit-field read/write need not be atomic here because even
if a data-race happens, for_each_set_bit() iterates each bit, which
is the real data, regardless of whether data-race happened or not.

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Rémi Denis-Courmont]

Le perjantaina 18. lokakuuta 2024, 20.16.29 EEST Kuniyuki Iwashima a écrit :
> From: "Rémi Denis-Courmont" <remi@remlab.net>
> Date: Thu, 17 Oct 2024 21:49:18 +0300
> 
> > > diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
> > > index 5996141e258f..14928fa04675 100644
> > > --- a/net/phonet/pn_netlink.c
> > > +++ b/net/phonet/pn_netlink.c
> > > @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32
> > > ifindex, u8 addr,
> > > 
> > >  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback
> > >  *cb)
> > > 
> > > {
> > > +	int addr_idx = 0, addr_start_idx = cb->args[1];
> > > +	int dev_idx = 0, dev_start_idx = cb->args[0];
> > > 
> > >  	struct phonet_device_list *pndevs;
> > >  	struct phonet_device *pnd;
> > > 
> > > -	int dev_idx = 0, dev_start_idx = cb->args[0];
> > > -	int addr_idx = 0, addr_start_idx = cb->args[1];
> > > +	int err = 0;
> > > 
> > >  	pndevs = phonet_device_list(sock_net(skb->sk));
> > > 
> > > +
> > > 
> > >  	rcu_read_lock();
> > >  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
> > > 
> > > +		DECLARE_BITMAP(addrs, 64);
> > > 
> > >  		u8 addr;
> > >  		
> > >  		if (dev_idx > dev_start_idx)
> > > 
> > > @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb,
> > > struct
> > > netlink_callback *cb) continue;
> > > 
> > >  		addr_idx = 0;
> > > 
> > > -		for_each_set_bit(addr, pnd->addrs, 64) {
> > > +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
> > 
> > Is that really safe? Are we sure that the bit-field writers are atomic
> > w.r.t. memcpy() on all platforms? If READ_ONCE is needed for an integer,
> > using memcpy() seems sketchy, TBH.
> 
> I think bit-field read/write need not be atomic here because even
> if a data-race happens, for_each_set_bit() iterates each bit, which
> is the real data, regardless of whether data-race happened or not.

Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
needed here.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Kuniyuki Iwashima]

From: "Rémi Denis-Courmont" <remi@remlab.net>
Date: Sat, 19 Oct 2024 10:48:57 +0300
> > > >  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
> > > > 
> > > > +		DECLARE_BITMAP(addrs, 64);
> > > > 
> > > >  		u8 addr;
> > > >  		
> > > >  		if (dev_idx > dev_start_idx)
> > > > 
> > > > @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb,
> > > > struct
> > > > netlink_callback *cb) continue;
> > > > 
> > > >  		addr_idx = 0;
> > > > 
> > > > -		for_each_set_bit(addr, pnd->addrs, 64) {
> > > > +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
> > > 
> > > Is that really safe? Are we sure that the bit-field writers are atomic
> > > w.r.t. memcpy() on all platforms? If READ_ONCE is needed for an integer,
> > > using memcpy() seems sketchy, TBH.
> > 
> > I think bit-field read/write need not be atomic here because even
> > if a data-race happens, for_each_set_bit() iterates each bit, which
> > is the real data, regardless of whether data-race happened or not.
> 
> Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
> and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
> needed here.

Let's say pnd->addrs has 0b00010001 as the lowest in 8 bits
and addr_dumpit() and addr_doit() are executed concurrently,
and addr_doit() tries to change it to 0b00010011.

If we have a lock, there could be two results of dumpit().

  1.
  lock()
  dumpit() -> 0b00010001
  unlock()
                   lock()
                   doit()
                   unlock()

  2.
                   lock()
                   doit()
                   unlock()
  lock()
  dumpit() -> 0b00010011
  unlock()

If we don't have a lock and dumpit()'s read and doit()'s write
are split to upper 4 bits (0b0001) and lower 4 bits (0b0011),

there are 6 patterns of occurences, but the results are only
2 patterns and the same with the locked version.

If you get 0b00010001, you can think dumpit() completes earlier,
and the opposite if you get 0b00010011.

This is how we think with lockless algo.  In this case, we do
not require lock to iterate bitmaps.

  1.
  upper half read of dumpit()
  lower half read of dumpit()
                   upper half write of doit() -> 0b0001
                   lower half write of doit() -> 0b0011
  -> 0b00010001

  2.
  upper half read of dumpit()
                   upper half write of doit() -> 0b0001
  lower half read of dumpit()
                   lower half write of doit() -> 0b0011
  -> 0b00010001

  3.
                   upper half write of doit() -> 0b0001
  upper half read of dumpit()
  lower half read of dumpit()
                   lower half write of doit() -> 0b0011
  -> 0b00010001

  4.
                   upper half write of doit() -> 0b0001
                   lower half write of doit() -> 0b0011
  upper half read of dumpit()
  lower half read of dumpit()
  -> 0b00010011

  5.
                   upper half write of doit() -> 0b0001
  upper half read of dumpit()
                   lower half write of doit() -> 0b0011
  lower half read of dumpit()
  -> 0b00010011

  6.
  upper half read of dumpit()
                   upper half write of doit() -> 0b0001
                   lower half write of doit() -> 0b0011
  lower half read of dumpit()
  -> 0b00010011

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Paolo Abeni]

On 10/19/24 09:48, Rémi Denis-Courmont wrote:
> Le perjantaina 18. lokakuuta 2024, 20.16.29 EEST Kuniyuki Iwashima a écrit :
>> From: "Rémi Denis-Courmont" <remi@remlab.net>
>> Date: Thu, 17 Oct 2024 21:49:18 +0300
>>
>>>> diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
>>>> index 5996141e258f..14928fa04675 100644
>>>> --- a/net/phonet/pn_netlink.c
>>>> +++ b/net/phonet/pn_netlink.c
>>>> @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32
>>>> ifindex, u8 addr,
>>>>
>>>>  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback
>>>>  *cb)
>>>>
>>>> {
>>>> +	int addr_idx = 0, addr_start_idx = cb->args[1];
>>>> +	int dev_idx = 0, dev_start_idx = cb->args[0];
>>>>
>>>>  	struct phonet_device_list *pndevs;
>>>>  	struct phonet_device *pnd;
>>>>
>>>> -	int dev_idx = 0, dev_start_idx = cb->args[0];
>>>> -	int addr_idx = 0, addr_start_idx = cb->args[1];
>>>> +	int err = 0;
>>>>
>>>>  	pndevs = phonet_device_list(sock_net(skb->sk));
>>>>
>>>> +
>>>>
>>>>  	rcu_read_lock();
>>>>  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
>>>>
>>>> +		DECLARE_BITMAP(addrs, 64);
>>>>
>>>>  		u8 addr;
>>>>  		
>>>>  		if (dev_idx > dev_start_idx)
>>>>
>>>> @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb,
>>>> struct
>>>> netlink_callback *cb) continue;
>>>>
>>>>  		addr_idx = 0;
>>>>
>>>> -		for_each_set_bit(addr, pnd->addrs, 64) {
>>>> +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
>>>
>>> Is that really safe? Are we sure that the bit-field writers are atomic
>>> w.r.t. memcpy() on all platforms? If READ_ONCE is needed for an integer,
>>> using memcpy() seems sketchy, TBH.
>>
>> I think bit-field read/write need not be atomic here because even
>> if a data-race happens, for_each_set_bit() iterates each bit, which
>> is the real data, regardless of whether data-race happened or not.
> 
> Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
> and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
> needed here.

To wrap-up Kuniyuki's reply: addresses can't be repeated in dump. They
can be 'skipped' meaning the dump can race with writer reading an 'old'
address bitmask, still not containing the 'new' address. Exactly as
could happen with racing dump/writer both protected by the lock.

The bottom line is that this code looks safe to me.

Thanks,

Paolo

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Paolo Abeni]

On 10/23/24 13:04, Paolo Abeni wrote:
> On 10/19/24 09:48, Rémi Denis-Courmont wrote:
>>> I think bit-field read/write need not be atomic here because even
>>> if a data-race happens, for_each_set_bit() iterates each bit, which
>>> is the real data, regardless of whether data-race happened or not.
>>
>> Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
>> and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
>> needed here.
> 
> To wrap-up Kuniyuki's reply: addresses can't be repeated in dump. They
> can be 'skipped' meaning the dump can race with writer reading an 'old'
> address bitmask, still not containing the 'new' address. Exactly as
> could happen with racing dump/writer both protected by the lock.
> 
> The bottom line is that this code looks safe to me.

I'm sorry, I forgot to ask the obvious question: @Rémi, are you ok with
this explanation and patch as-is?

Thanks!

Paolo

Re: [PATCH v1 net-next 1/9] phonet: Pass ifindex to fill_addr().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> We will convert addr_doit() and getaddr_dumpit() to RCU, both
> of which call fill_addr().
>
> The former will call phonet_address_notify() outside of RCU
> due to GFP_KERNEL, so dev will not be available in fill_addr().
>
> Let's pass ifindex directly to fill_addr().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 2/9] phonet: Pass net and ifindex to phonet_address_notify().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Currently, phonet_address_notify() fetches netns and ifindex from dev.
>
> Once addr_doit() is converted to RCU, phonet_address_notify() will be
> called outside of RCU due to GFP_KERNEL, and dev will be unavailable
> there.
>
> Let's pass net and ifindex to phonet_address_notify().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 3/9] phonet: Convert phonet_device_list.lock to spinlock_t.

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:32 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> addr_doit() calls phonet_address_add() or phonet_address_del()
> for RTM_NEWADDR or RTM_DELADDR, respectively.
>
> Both functions only touch phonet_device_list(dev_net(dev)),
> which is currently protected by RTNL and its dedicated mutex,
> phonet_device_list.lock.
>
> We will convert addr_doit() to RCU and cannot use mutex inside RCU.
>
> Let's convert the mutex to spinlock_t.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 4/9] phonet: Don't hold RTNL for addr_doit().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:33 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Now only __dev_get_by_index() depends on RTNL in addr_doit().
>
> Let's use dev_get_by_index_rcu() and register addr_doit() with
> RTNL_FLAG_DOIT_UNLOCKED.
>
> While at it, I changed phonet_rtnl_msg_handlers[]'s init to C99
> style like other core networking code.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:33 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> getaddr_dumpit() already relies on RCU and does not need RTNL.
>
> Let's use READ_ONCE() for ifindex and register getaddr_dumpit()
> with RTNL_FLAG_DUMP_UNLOCKED.
>
> While at it, the retval of getaddr_dumpit() is changed to combine
> NLMSG_DONE and save recvmsg() as done in 58a4ff5d77b1 ("phonet: no
> longer hold RTNL in route_dumpit()").
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 6/9] phonet: Pass ifindex to fill_route().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:33 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> We will convert route_doit() to RCU.
>
> route_doit() will call rtm_phonet_notify() outside of RCU due
> to GFP_KERNEL, so dev will not be available in fill_route().
>
> Let's pass ifindex directly to fill_route().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 7/9] phonet: Pass net and ifindex to rtm_phonet_notify().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:34 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Currently, rtm_phonet_notify() fetches netns and ifindex from dev.
>
> Once route_doit() is converted to RCU, rtm_phonet_notify() will be
> called outside of RCU due to GFP_KERNEL, and dev will be unavailable
> there.
>
> Let's pass net and ifindex to rtm_phonet_notify().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 8/9] phonet: Convert phonet_routes.lock to spinlock_t.

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:34 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> route_doit() calls phonet_route_add() or phonet_route_del()
> for RTM_NEWROUTE or RTM_DELROUTE, respectively.
>
> Both functions only touch phonet_pernet(dev_net(dev))->routes,
> which is currently protected by RTNL and its dedicated mutex,
> phonet_routes.lock.
>
> We will convert route_doit() to RCU and cannot use mutex inside RCU.
>
> Let's convert the mutex to spinlock_t.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 9/9] phonet: Don't hold RTNL for route_doit().

[Eric Dumazet]

On Thu, Oct 17, 2024 at 8:34 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Now only __dev_get_by_index() depends on RTNL in route_doit().
>
> Let's use dev_get_by_index_rcu() and register route_doit() with
> RTNL_FLAG_DOIT_UNLOCKED.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 net-next 0/9] phonet: Convert all doit() and dumpit() to RCU.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 17 Oct 2024 11:31:31 -0700 you wrote:
> addr_doit() and route_doit() access only phonet_device_list(dev_net(dev))
> and phonet_pernet(dev_net(dev))->routes, respectively.
> 
> Each per-netns struct has its dedicated mutex, and RTNL also protects
> the structs.  __dev_change_net_namespace() has synchronize_net(), so
> we have two options to convert addr_doit() and route_doit().
> 
> [...]

Here is the summary with links:
  - [v1,net-next,1/9] phonet: Pass ifindex to fill_addr().
    https://git.kernel.org/netdev/net-next/c/08a9572be368
  - [v1,net-next,2/9] phonet: Pass net and ifindex to phonet_address_notify().
    https://git.kernel.org/netdev/net-next/c/68ed5c38b512
  - [v1,net-next,3/9] phonet: Convert phonet_device_list.lock to spinlock_t.
    https://git.kernel.org/netdev/net-next/c/42f5fe1dc4ba
  - [v1,net-next,4/9] phonet: Don't hold RTNL for addr_doit().
    https://git.kernel.org/netdev/net-next/c/8786e98dd0eb
  - [v1,net-next,5/9] phonet: Don't hold RTNL for getaddr_dumpit().
    https://git.kernel.org/netdev/net-next/c/b7d2fc9ad7fe
  - [v1,net-next,6/9] phonet: Pass ifindex to fill_route().
    https://git.kernel.org/netdev/net-next/c/302fc6bbcba4
  - [v1,net-next,7/9] phonet: Pass net and ifindex to rtm_phonet_notify().
    https://git.kernel.org/netdev/net-next/c/de51ad08b117
  - [v1,net-next,8/9] phonet: Convert phonet_routes.lock to spinlock_t.
    https://git.kernel.org/netdev/net-next/c/3deec3b4afb4
  - [v1,net-next,9/9] phonet: Don't hold RTNL for route_doit().
    https://git.kernel.org/netdev/net-next/c/17a1ac0018ae

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