[PATCH v2 net 0/6] rtnetlink: Handle error of rtnl_register_module().

[PATCH v2 net 0/6] rtnetlink: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

While converting phonet to per-netns RTNL, I found a weird comment

  /* Further rtnl_register_module() cannot fail */

that was true but no longer true after commit addf9b90de22 ("net:
rtnetlink: use rcu to free rtnl message handlers").

Many callers of rtnl_register_module() just ignore the returned
value but should handle them properly.

This series introduces two helpers, rtnl_register_module_many() and
rtnl_unregister_module_many(), to do that easily and fix such callers.

All rtnl_register() and rtnl_register_module() will be converted
to _many() variant and some rtnl_lock() will be saved in _many()
later in net-next.


Changes:
  v2: Remove __exit from mctp_neigh_exit().

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


Kuniyuki Iwashima (6):
  rtnetlink: Add bulk registration helpers for rtnetlink message
    handlers.
  vxlan: Handle error of rtnl_register_module().
  bridge: Handle error of rtnl_register_module().
  mctp: Handle error of rtnl_register_module().
  mpls: Handle error of rtnl_register_module().
  phonet: Handle error of rtnl_register_module().

 drivers/net/vxlan/vxlan_core.c      |  6 ++++-
 drivers/net/vxlan/vxlan_private.h   |  2 +-
 drivers/net/vxlan/vxlan_vnifilter.c | 19 ++++++++--------
 include/net/mctp.h                  |  2 +-
 include/net/rtnetlink.h             | 19 ++++++++++++++++
 net/bridge/br_netlink.c             |  6 ++++-
 net/bridge/br_private.h             |  5 +++--
 net/bridge/br_vlan.c                | 19 ++++++++--------
 net/core/rtnetlink.c                | 30 +++++++++++++++++++++++++
 net/mctp/af_mctp.c                  |  6 ++++-
 net/mctp/device.c                   | 30 +++++++++++++++----------
 net/mctp/neigh.c                    | 31 ++++++++++++++++----------
 net/mctp/route.c                    | 34 ++++++++++++++++++++---------
 net/mpls/af_mpls.c                  | 32 +++++++++++++++++----------
 net/phonet/pn_netlink.c             | 27 +++++++++--------------
 15 files changed, 179 insertions(+), 89 deletions(-)

-- 
2.30.2

[PATCH v2 net 1/6] rtnetlink: Add bulk registration helpers for rtnetlink message handlers.

[Kuniyuki Iwashima]

Before commit addf9b90de22 ("net: rtnetlink: use rcu to free rtnl message
handlers"), once rtnl_msg_handlers[protocol] was allocated, the following
rtnl_register_module() for the same protocol never failed.

However, after the commit, rtnl_msg_handler[protocol][msgtype] needs to
be allocated in each rtnl_register_module(), so each call could fail.

Many callers of rtnl_register_module() do not handle the returned error,
and we need to add many error handlings.

To handle that easily, let's add wrapper functions for bulk registration
of rtnetlink message handlers.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/rtnetlink.h | 19 +++++++++++++++++++
 net/core/rtnetlink.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index b45d57b5968a..b6b91898dc13 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -29,6 +29,14 @@ static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
 	return msgtype & RTNL_KIND_MASK;
 }
 
+struct rtnl_msg_handler {
+	int protocol;
+	int msgtype;
+	rtnl_doit_func doit;
+	rtnl_dumpit_func dumpit;
+	int flags;
+};
+
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_register_module(struct module *owner, int protocol, int msgtype,
@@ -36,6 +44,17 @@ int rtnl_register_module(struct module *owner, int protocol, int msgtype,
 int rtnl_unregister(int protocol, int msgtype);
 void rtnl_unregister_all(int protocol);
 
+int __rtnl_register_many(struct module *owner,
+			 struct rtnl_msg_handler *handlers, int n);
+void __rtnl_unregister_many(struct rtnl_msg_handler *handlers, int n);
+
+#define rtnl_register_many(handlers)						\
+	__rtnl_register_many(NULL, handlers, ARRAY_SIZE(handlers))
+#define rtnl_register_module_many(handlers)					\
+	__rtnl_register_many(THIS_MODULE, handlers, ARRAY_SIZE(handlers))
+#define rtnl_unregister_many(handlers)						\
+	__rtnl_unregister_many(handlers, ARRAY_SIZE(handlers))
+
 static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
 {
 	if (nlmsg_len(nlh) >= sizeof(struct rtgenmsg))
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index f0a520987085..1fac9e786315 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -384,6 +384,36 @@ void rtnl_unregister_all(int protocol)
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister_all);
 
+int __rtnl_register_many(struct module *owner,
+			 struct rtnl_msg_handler *handlers, int n)
+{
+	struct rtnl_msg_handler *handler;
+	int i, err;
+
+	for (i = 0, handler = handlers; i < n; i++, handler++) {
+		err = rtnl_register_internal(owner, handler->protocol,
+					     handler->msgtype, handler->doit,
+					     handler->dumpit, handler->flags);
+		if (err) {
+			__rtnl_unregister_many(handlers, i);
+			break;
+		}
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(__rtnl_register_many);
+
+void __rtnl_unregister_many(struct rtnl_msg_handler *handlers, int n)
+{
+	struct rtnl_msg_handler *handler;
+	int i;
+
+	for (i = n - 1, handler = handlers + n - 1; i >= 0; i--, handler--)
+		rtnl_unregister(handler->protocol, handler->msgtype);
+}
+EXPORT_SYMBOL_GPL(__rtnl_unregister_many);
+
 static LIST_HEAD(link_ops);
 
 static const struct rtnl_link_ops *rtnl_link_ops_get(const char *kind)
-- 
2.30.2

[PATCH v2 net 2/6] vxlan: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

Since introduced, vxlan_vnifilter_init() has been ignoring the
returned value of rtnl_register_module(), which could fail.

Let's handle the errors by rtnl_register_module_many().

Fixes: f9c4bb0b245c ("vxlan: vni filtering support on collect metadata device")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Roopa Prabhu <roopa@nvidia.com>
---
 drivers/net/vxlan/vxlan_core.c      |  6 +++++-
 drivers/net/vxlan/vxlan_private.h   |  2 +-
 drivers/net/vxlan/vxlan_vnifilter.c | 19 +++++++++----------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 53dcb9fffc04..6e9a3795846a 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -4913,9 +4913,13 @@ static int __init vxlan_init_module(void)
 	if (rc)
 		goto out4;
 
-	vxlan_vnifilter_init();
+	rc = vxlan_vnifilter_init();
+	if (rc)
+		goto out5;
 
 	return 0;
+out5:
+	rtnl_link_unregister(&vxlan_link_ops);
 out4:
 	unregister_switchdev_notifier(&vxlan_switchdev_notifier_block);
 out3:
diff --git a/drivers/net/vxlan/vxlan_private.h b/drivers/net/vxlan/vxlan_private.h
index b35d96b78843..76a351a997d5 100644
--- a/drivers/net/vxlan/vxlan_private.h
+++ b/drivers/net/vxlan/vxlan_private.h
@@ -202,7 +202,7 @@ int vxlan_vni_in_use(struct net *src_net, struct vxlan_dev *vxlan,
 int vxlan_vnigroup_init(struct vxlan_dev *vxlan);
 void vxlan_vnigroup_uninit(struct vxlan_dev *vxlan);
 
-void vxlan_vnifilter_init(void);
+int vxlan_vnifilter_init(void);
 void vxlan_vnifilter_uninit(void);
 void vxlan_vnifilter_count(struct vxlan_dev *vxlan, __be32 vni,
 			   struct vxlan_vni_node *vninode,
diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
index 9c59d0bf8c3d..27b4d179a703 100644
--- a/drivers/net/vxlan/vxlan_vnifilter.c
+++ b/drivers/net/vxlan/vxlan_vnifilter.c
@@ -992,19 +992,18 @@ static int vxlan_vnifilter_process(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-void vxlan_vnifilter_init(void)
+static struct rtnl_msg_handler vxlan_vnifilter_rtnl_msg_handlers[] = {
+	{PF_BRIDGE, RTM_GETTUNNEL, NULL, vxlan_vnifilter_dump, 0},
+	{PF_BRIDGE, RTM_NEWTUNNEL, vxlan_vnifilter_process, NULL, 0},
+	{PF_BRIDGE, RTM_DELTUNNEL, vxlan_vnifilter_process, NULL, 0},
+};
+
+int vxlan_vnifilter_init(void)
 {
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETTUNNEL, NULL,
-			     vxlan_vnifilter_dump, 0);
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_NEWTUNNEL,
-			     vxlan_vnifilter_process, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_DELTUNNEL,
-			     vxlan_vnifilter_process, NULL, 0);
+	return rtnl_register_module_many(vxlan_vnifilter_rtnl_msg_handlers);
 }
 
 void vxlan_vnifilter_uninit(void)
 {
-	rtnl_unregister(PF_BRIDGE, RTM_GETTUNNEL);
-	rtnl_unregister(PF_BRIDGE, RTM_NEWTUNNEL);
-	rtnl_unregister(PF_BRIDGE, RTM_DELTUNNEL);
+	rtnl_unregister_many(vxlan_vnifilter_rtnl_msg_handlers);
 }
-- 
2.30.2

[PATCH v2 net 3/6] bridge: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

Since introduced, br_vlan_rtnl_init() has been ignoring the returned
value of rtnl_register_module(), which could fail.

Let's handle the errors by rtnl_register_module_many().

Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump support")
Fixes: f26b296585dc ("net: bridge: vlan: add new rtm message support")
Fixes: adb3ce9bcb0f ("net: bridge: vlan: add del rtm message support")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Roopa Prabhu <roopa@nvidia.com>
Cc: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_netlink.c |  6 +++++-
 net/bridge/br_private.h |  5 +++--
 net/bridge/br_vlan.c    | 19 +++++++++----------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index f17dbac7d828..6b97ae47f855 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1920,7 +1920,10 @@ int __init br_netlink_init(void)
 {
 	int err;
 
-	br_vlan_rtnl_init();
+	err = br_vlan_rtnl_init();
+	if (err)
+		goto out;
+
 	rtnl_af_register(&br_af_ops);
 
 	err = rtnl_link_register(&br_link_ops);
@@ -1931,6 +1934,7 @@ int __init br_netlink_init(void)
 
 out_af:
 	rtnl_af_unregister(&br_af_ops);
+out:
 	return err;
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index d4bedc87b1d8..041f6e571a20 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1571,7 +1571,7 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
 			 void *ptr);
-void br_vlan_rtnl_init(void);
+int br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,
 		    const struct net_bridge_port *p,
@@ -1802,8 +1802,9 @@ static inline int br_vlan_bridge_event(struct net_device *dev,
 	return 0;
 }
 
-static inline void br_vlan_rtnl_init(void)
+static inline int br_vlan_rtnl_init(void)
 {
+	return 0;
 }
 
 static inline void br_vlan_rtnl_uninit(void)
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 9c2fffb827ab..a9f00d72fe18 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -2296,19 +2296,18 @@ static int br_vlan_rtm_process(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-void br_vlan_rtnl_init(void)
+static struct rtnl_msg_handler br_vlan_rtnl_msg_handlers[] = {
+	{PF_BRIDGE, RTM_NEWVLAN, br_vlan_rtm_process, NULL, 0},
+	{PF_BRIDGE, RTM_DELVLAN, br_vlan_rtm_process, NULL, 0},
+	{PF_BRIDGE, RTM_GETVLAN, NULL, br_vlan_rtm_dump, 0},
+};
+
+int br_vlan_rtnl_init(void)
 {
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETVLAN, NULL,
-			     br_vlan_rtm_dump, 0);
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_NEWVLAN,
-			     br_vlan_rtm_process, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_DELVLAN,
-			     br_vlan_rtm_process, NULL, 0);
+	return rtnl_register_module_many(br_vlan_rtnl_msg_handlers);
 }
 
 void br_vlan_rtnl_uninit(void)
 {
-	rtnl_unregister(PF_BRIDGE, RTM_GETVLAN);
-	rtnl_unregister(PF_BRIDGE, RTM_NEWVLAN);
-	rtnl_unregister(PF_BRIDGE, RTM_DELVLAN);
+	rtnl_unregister_many(br_vlan_rtnl_msg_handlers);
 }
-- 
2.30.2

[PATCH v2 net 4/6] mctp: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

Since introduced, mctp has been ignoring the returned value
of rtnl_register_module(), which could fail.

Let's handle the errors by rtnl_register_module_many().

Fixes: 583be982d934 ("mctp: Add device handling and netlink interface")
Fixes: 831119f88781 ("mctp: Add neighbour netlink interface")
Fixes: 06d2f4c583a7 ("mctp: Add netlink route management")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Matt Johnston <matt@codeconstruct.com.au>
---
 include/net/mctp.h |  2 +-
 net/mctp/af_mctp.c |  6 +++++-
 net/mctp/device.c  | 30 ++++++++++++++++++------------
 net/mctp/neigh.c   | 31 +++++++++++++++++++------------
 net/mctp/route.c   | 34 ++++++++++++++++++++++++----------
 5 files changed, 67 insertions(+), 36 deletions(-)

diff --git a/include/net/mctp.h b/include/net/mctp.h
index 7b17c52e8ce2..28d59ae94ca3 100644
--- a/include/net/mctp.h
+++ b/include/net/mctp.h
@@ -295,7 +295,7 @@ void mctp_neigh_remove_dev(struct mctp_dev *mdev);
 int mctp_routes_init(void);
 void mctp_routes_exit(void);
 
-void mctp_device_init(void);
+int mctp_device_init(void);
 void mctp_device_exit(void);
 
 #endif /* __NET_MCTP_H */
diff --git a/net/mctp/af_mctp.c b/net/mctp/af_mctp.c
index 43288b408fde..f6de136008f6 100644
--- a/net/mctp/af_mctp.c
+++ b/net/mctp/af_mctp.c
@@ -756,10 +756,14 @@ static __init int mctp_init(void)
 	if (rc)
 		goto err_unreg_routes;
 
-	mctp_device_init();
+	rc = mctp_device_init();
+	if (rc)
+		goto err_unreg_neigh;
 
 	return 0;
 
+err_unreg_neigh:
+	mctp_neigh_exit();
 err_unreg_routes:
 	mctp_routes_exit();
 err_unreg_proto:
diff --git a/net/mctp/device.c b/net/mctp/device.c
index acb97b257428..d70e688ac886 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -524,25 +524,31 @@ static struct notifier_block mctp_dev_nb = {
 	.priority = ADDRCONF_NOTIFY_PRIORITY,
 };
 
-void __init mctp_device_init(void)
+static struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
+	{PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
+	{PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
+	{PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
+};
+
+int __init mctp_device_init(void)
 {
-	register_netdevice_notifier(&mctp_dev_nb);
+	int err;
 
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETADDR,
-			     NULL, mctp_dump_addrinfo, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWADDR,
-			     mctp_rtm_newaddr, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELADDR,
-			     mctp_rtm_deladdr, NULL, 0);
+	register_netdevice_notifier(&mctp_dev_nb);
 	rtnl_af_register(&mctp_af_ops);
+
+	err = rtnl_register_module_many(mctp_device_rtnl_msg_handlers);
+	if (err) {
+		rtnl_af_unregister(&mctp_af_ops);
+		unregister_netdevice_notifier(&mctp_dev_nb);
+	}
+
+	return err;
 }
 
 void __exit mctp_device_exit(void)
 {
+	rtnl_unregister_many(mctp_device_rtnl_msg_handlers);
 	rtnl_af_unregister(&mctp_af_ops);
-	rtnl_unregister(PF_MCTP, RTM_DELADDR);
-	rtnl_unregister(PF_MCTP, RTM_NEWADDR);
-	rtnl_unregister(PF_MCTP, RTM_GETADDR);
-
 	unregister_netdevice_notifier(&mctp_dev_nb);
 }
diff --git a/net/mctp/neigh.c b/net/mctp/neigh.c
index ffa0f9e0983f..245f8242929b 100644
--- a/net/mctp/neigh.c
+++ b/net/mctp/neigh.c
@@ -322,22 +322,29 @@ static struct pernet_operations mctp_net_ops = {
 	.exit = mctp_neigh_net_exit,
 };
 
+static struct rtnl_msg_handler mctp_neigh_rtnl_msg_handlers[] = {
+	{PF_MCTP, RTM_NEWNEIGH, mctp_rtm_newneigh, NULL, 0},
+	{PF_MCTP, RTM_DELNEIGH, mctp_rtm_delneigh, NULL, 0},
+	{PF_MCTP, RTM_GETNEIGH, NULL, mctp_rtm_getneigh, 0},
+};
+
 int __init mctp_neigh_init(void)
 {
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWNEIGH,
-			     mctp_rtm_newneigh, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELNEIGH,
-			     mctp_rtm_delneigh, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETNEIGH,
-			     NULL, mctp_rtm_getneigh, 0);
-
-	return register_pernet_subsys(&mctp_net_ops);
+	int err;
+
+	err = register_pernet_subsys(&mctp_net_ops);
+	if (err)
+		return err;
+
+	err = rtnl_register_module_many(mctp_neigh_rtnl_msg_handlers);
+	if (err)
+		unregister_pernet_subsys(&mctp_net_ops);
+
+	return err;
 }
 
-void __exit mctp_neigh_exit(void)
+void mctp_neigh_exit(void)
 {
+	rtnl_unregister_many(mctp_neigh_rtnl_msg_handlers);
 	unregister_pernet_subsys(&mctp_net_ops);
-	rtnl_unregister(PF_MCTP, RTM_GETNEIGH);
-	rtnl_unregister(PF_MCTP, RTM_DELNEIGH);
-	rtnl_unregister(PF_MCTP, RTM_NEWNEIGH);
 }
diff --git a/net/mctp/route.c b/net/mctp/route.c
index eefd7834d9a0..083f9933eb81 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -1474,26 +1474,40 @@ static struct pernet_operations mctp_net_ops = {
 	.exit = mctp_routes_net_exit,
 };
 
+static struct rtnl_msg_handler mctp_route_rtnl_msg_handlers[] = {
+	{PF_MCTP, RTM_NEWROUTE, mctp_newroute, NULL, 0},
+	{PF_MCTP, RTM_DELROUTE, mctp_delroute, NULL, 0},
+	{PF_MCTP, RTM_GETROUTE, NULL, mctp_dump_rtinfo, 0},
+};
+
 int __init mctp_routes_init(void)
 {
+	int err;
+
 	dev_add_pack(&mctp_packet_type);
 
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETROUTE,
-			     NULL, mctp_dump_rtinfo, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWROUTE,
-			     mctp_newroute, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELROUTE,
-			     mctp_delroute, NULL, 0);
+	err = register_pernet_subsys(&mctp_net_ops);
+	if (err)
+		goto fail_pernet;
+
+	err = rtnl_register_module_many(mctp_route_rtnl_msg_handlers);
+	if (err)
+		goto fail_rtnl;
 
-	return register_pernet_subsys(&mctp_net_ops);
+out:
+	return err;
+
+fail_rtnl:
+	unregister_pernet_subsys(&mctp_net_ops);
+fail_pernet:
+	dev_remove_pack(&mctp_packet_type);
+	goto out;
 }
 
 void mctp_routes_exit(void)
 {
+	rtnl_unregister_many(mctp_route_rtnl_msg_handlers);
 	unregister_pernet_subsys(&mctp_net_ops);
-	rtnl_unregister(PF_MCTP, RTM_DELROUTE);
-	rtnl_unregister(PF_MCTP, RTM_NEWROUTE);
-	rtnl_unregister(PF_MCTP, RTM_GETROUTE);
 	dev_remove_pack(&mctp_packet_type);
 }
 
-- 
2.30.2

[PATCH v2 net 5/6] mpls: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

Since introduced, mpls_init() has been ignoring the returned
value of rtnl_register_module(), which could fail.

Let's handle the errors by rtnl_register_module_many().

Fixes: 03c0566542f4 ("mpls: Netlink commands to add, remove, and dump routes")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/mpls/af_mpls.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index aba983531ed3..653897327165 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -2728,6 +2728,15 @@ static struct rtnl_af_ops mpls_af_ops __read_mostly = {
 	.get_stats_af_size = mpls_get_stats_af_size,
 };
 
+static struct rtnl_msg_handler mpls_rtnl_msg_handlers[] = {
+	{PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0},
+	{PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0},
+	{PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0},
+	{PF_MPLS, RTM_GETNETCONF,
+	 mpls_netconf_get_devconf, mpls_netconf_dump_devconf,
+	 RTNL_FLAG_DUMP_UNLOCKED},
+};
+
 static int __init mpls_init(void)
 {
 	int err;
@@ -2746,24 +2755,25 @@ static int __init mpls_init(void)
 
 	rtnl_af_register(&mpls_af_ops);
 
-	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_NEWROUTE,
-			     mpls_rtm_newroute, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_DELROUTE,
-			     mpls_rtm_delroute, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETROUTE,
-			     mpls_getroute, mpls_dump_routes, 0);
-	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETNETCONF,
-			     mpls_netconf_get_devconf,
-			     mpls_netconf_dump_devconf,
-			     RTNL_FLAG_DUMP_UNLOCKED);
-	err = ipgre_tunnel_encap_add_mpls_ops();
+	err = rtnl_register_module_many(mpls_rtnl_msg_handlers);
 	if (err)
+		goto out_unregister_rtnl_af;
+
+	err = ipgre_tunnel_encap_add_mpls_ops();
+	if (err) {
 		pr_err("Can't add mpls over gre tunnel ops\n");
+		goto out_unregister_rtnl;
+	}
 
 	err = 0;
 out:
 	return err;
 
+out_unregister_rtnl:
+	rtnl_unregister_many(mpls_rtnl_msg_handlers);
+out_unregister_rtnl_af:
+	rtnl_af_unregister(&mpls_af_ops);
+	dev_remove_pack(&mpls_packet_type);
 out_unregister_pernet:
 	unregister_pernet_subsys(&mpls_net_ops);
 	goto out;
-- 
2.30.2

[PATCH v2 net 6/6] phonet: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

Before commit addf9b90de22 ("net: rtnetlink: use rcu to free rtnl
message handlers"), once the first rtnl_register_module() allocated
rtnl_msg_handlers[PF_PHONET], the following calls never failed.

However, after the commit, rtnl_register_module() could fail to allocate
rtnl_msg_handlers[PF_PHONET][msgtype] and requires error handling for
each call.

Let's use rtnl_register_module_many() to handle the errors easily.

Fixes: addf9b90de22 ("net: rtnetlink: use rcu to free rtnl message handlers")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Acked-by: Rémi Denis-Courmont <courmisch@gmail.com>
---
Cc: Florian Westphal <fw@strlen.de>
---
 net/phonet/pn_netlink.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 7008d402499d..d39e6983926b 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -285,23 +285,16 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	return err;
 }
 
+static struct rtnl_msg_handler phonet_rtnl_msg_handlers[] __initdata_or_module = {
+	{PF_PHONET, RTM_NEWADDR, addr_doit, NULL, 0},
+	{PF_PHONET, RTM_DELADDR, addr_doit, NULL, 0},
+	{PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0},
+	{PF_PHONET, RTM_NEWROUTE, route_doit, NULL, 0},
+	{PF_PHONET, RTM_DELROUTE, route_doit, NULL, 0},
+	{PF_PHONET, RTM_GETROUTE, NULL, route_dumpit, RTNL_FLAG_DUMP_UNLOCKED},
+};
+
 int __init phonet_netlink_register(void)
 {
-	int err = rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWADDR,
-				       addr_doit, NULL, 0);
-	if (err)
-		return err;
-
-	/* Further rtnl_register_module() cannot fail */
-	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELADDR,
-			     addr_doit, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETADDR,
-			     NULL, getaddr_dumpit, 0);
-	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWROUTE,
-			     route_doit, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELROUTE,
-			     route_doit, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETROUTE,
-			     NULL, route_dumpit, RTNL_FLAG_DUMP_UNLOCKED);
-	return 0;
+	return rtnl_register_module_many(phonet_rtnl_msg_handlers);
 }
-- 
2.30.2

Re: [PATCH v2 net 1/6] rtnetlink: Add bulk registration helpers for rtnetlink message handlers.

[Eric Dumazet]

On Sat, Oct 5, 2024 at 12:24 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Before commit addf9b90de22 ("net: rtnetlink: use rcu to free rtnl message
> handlers"), once rtnl_msg_handlers[protocol] was allocated, the following
> rtnl_register_module() for the same protocol never failed.
>
> However, after the commit, rtnl_msg_handler[protocol][msgtype] needs to
> be allocated in each rtnl_register_module(), so each call could fail.
>
> Many callers of rtnl_register_module() do not handle the returned error,
> and we need to add many error handlings.
>
> To handle that easily, let's add wrapper functions for bulk registration
> of rtnetlink message handlers.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  include/net/rtnetlink.h | 19 +++++++++++++++++++
>  net/core/rtnetlink.c    | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index b45d57b5968a..b6b91898dc13 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -29,6 +29,14 @@ static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
>         return msgtype & RTNL_KIND_MASK;
>  }
>
> +struct rtnl_msg_handler {

Since we add a structure, we could stick here a

            struct module *owner;

> +       int protocol;
> +       int msgtype;
> +       rtnl_doit_func doit;
> +       rtnl_dumpit_func dumpit;
> +       int flags;
> +};
> +
>  void rtnl_register(int protocol, int msgtype,
>                    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
>  int rtnl_register_module(struct module *owner, int protocol, int msgtype,
> @@ -36,6 +44,17 @@ int rtnl_register_module(struct module *owner, int protocol, int msgtype,
>  int rtnl_unregister(int protocol, int msgtype);
>  void rtnl_unregister_all(int protocol);
>
> +int __rtnl_register_many(struct module *owner,
> +                        struct rtnl_msg_handler *handlers, int n);
> +void __rtnl_unregister_many(struct rtnl_msg_handler *handlers, int n);
> +
> +#define rtnl_register_many(handlers)                                           \
> +       __rtnl_register_many(NULL, handlers, ARRAY_SIZE(handlers))
> +#define rtnl_register_module_many(handlers)                                    \
> +       __rtnl_register_many(THIS_MODULE, handlers, ARRAY_SIZE(handlers))


This would allow a simpler api, no need for rtnl_register_module_many()

Re: [PATCH v2 net 3/6] bridge: Handle error of rtnl_register_module().

[Nikolay Aleksandrov]

On 05/10/2024 01:23, Kuniyuki Iwashima wrote:
> Since introduced, br_vlan_rtnl_init() has been ignoring the returned
> value of rtnl_register_module(), which could fail.
> 
> Let's handle the errors by rtnl_register_module_many().
> 
> Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump support")
> Fixes: f26b296585dc ("net: bridge: vlan: add new rtm message support")
> Fixes: adb3ce9bcb0f ("net: bridge: vlan: add del rtm message support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Cc: Roopa Prabhu <roopa@nvidia.com>
> Cc: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  net/bridge/br_netlink.c |  6 +++++-
>  net/bridge/br_private.h |  5 +++--
>  net/bridge/br_vlan.c    | 19 +++++++++----------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 

Oops, I guess copy/paste mistakes.
Thank you for fixing them!

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net 2/6] vxlan: Handle error of rtnl_register_module().

[Nikolay Aleksandrov]

On 05/10/2024 01:23, Kuniyuki Iwashima wrote:
> Since introduced, vxlan_vnifilter_init() has been ignoring the
> returned value of rtnl_register_module(), which could fail.
> 
> Let's handle the errors by rtnl_register_module_many().
> 
> Fixes: f9c4bb0b245c ("vxlan: vni filtering support on collect metadata device")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Cc: Roopa Prabhu <roopa@nvidia.com>
> ---
>  drivers/net/vxlan/vxlan_core.c      |  6 +++++-
>  drivers/net/vxlan/vxlan_private.h   |  2 +-
>  drivers/net/vxlan/vxlan_vnifilter.c | 19 +++++++++----------
>  3 files changed, 15 insertions(+), 12 deletions(-)
> 

FWIW,
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net 1/6] rtnetlink: Add bulk registration helpers for rtnetlink message handlers.

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Sat, 5 Oct 2024 10:25:20 +0200
> On Sat, Oct 5, 2024 at 12:24 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > Before commit addf9b90de22 ("net: rtnetlink: use rcu to free rtnl message
> > handlers"), once rtnl_msg_handlers[protocol] was allocated, the following
> > rtnl_register_module() for the same protocol never failed.
> >
> > However, after the commit, rtnl_msg_handler[protocol][msgtype] needs to
> > be allocated in each rtnl_register_module(), so each call could fail.
> >
> > Many callers of rtnl_register_module() do not handle the returned error,
> > and we need to add many error handlings.
> >
> > To handle that easily, let's add wrapper functions for bulk registration
> > of rtnetlink message handlers.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  include/net/rtnetlink.h | 19 +++++++++++++++++++
> >  net/core/rtnetlink.c    | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> > index b45d57b5968a..b6b91898dc13 100644
> > --- a/include/net/rtnetlink.h
> > +++ b/include/net/rtnetlink.h
> > @@ -29,6 +29,14 @@ static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
> >         return msgtype & RTNL_KIND_MASK;
> >  }
> >
> > +struct rtnl_msg_handler {
> 
> Since we add a structure, we could stick here a
> 
>             struct module *owner;

Will add it and remove _module version in v3.

Thanks!


> 
> > +       int protocol;
> > +       int msgtype;
> > +       rtnl_doit_func doit;
> > +       rtnl_dumpit_func dumpit;
> > +       int flags;
> > +};
> > +
> >  void rtnl_register(int protocol, int msgtype,
> >                    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
> >  int rtnl_register_module(struct module *owner, int protocol, int msgtype,
> > @@ -36,6 +44,17 @@ int rtnl_register_module(struct module *owner, int protocol, int msgtype,
> >  int rtnl_unregister(int protocol, int msgtype);
> >  void rtnl_unregister_all(int protocol);
> >
> > +int __rtnl_register_many(struct module *owner,
> > +                        struct rtnl_msg_handler *handlers, int n);
> > +void __rtnl_unregister_many(struct rtnl_msg_handler *handlers, int n);
> > +
> > +#define rtnl_register_many(handlers)                                           \
> > +       __rtnl_register_many(NULL, handlers, ARRAY_SIZE(handlers))
> > +#define rtnl_register_module_many(handlers)                                    \
> > +       __rtnl_register_many(THIS_MODULE, handlers, ARRAY_SIZE(handlers))
> 
> 
> This would allow a simpler api, no need for rtnl_register_module_many()

Re: [PATCH v2 net 4/6] mctp: Handle error of rtnl_register_module().

[Jeremy Kerr]

Hi Kuniyuki,

> Since introduced, mctp has been ignoring the returned value
> of rtnl_register_module(), which could fail.
> 
> Let's handle the errors by rtnl_register_module_many().

Sounds good!

Just a couple of minor things inline, but regardless:

Reviewed-by: Jeremy Kerr <jk@codeconstruct.com.au>

> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index acb97b257428..d70e688ac886 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -524,25 +524,31 @@ static struct notifier_block mctp_dev_nb = {
>         .priority = ADDRCONF_NOTIFY_PRIORITY,
>  };
>  
> -void __init mctp_device_init(void)
> +static struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
> +       {PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
> +       {PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
> +       {PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
> +};

Can this (and the other handler arrays) be const? And consequently, the
pointer argument that you pass to rtnl_register_module_many() from 1/6?

>  int __init mctp_routes_init(void)
>  {
> +       int err;
> +
>         dev_add_pack(&mctp_packet_type);
>  
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETROUTE,
> -                            NULL, mctp_dump_rtinfo, 0);
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWROUTE,
> -                            mctp_newroute, NULL, 0);
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELROUTE,
> -                            mctp_delroute, NULL, 0);
> +       err = register_pernet_subsys(&mctp_net_ops);
> +       if (err)
> +               goto fail_pernet;
> +
> +       err = rtnl_register_module_many(mctp_route_rtnl_msg_handlers);
> +       if (err)
> +               goto fail_rtnl;
>  
> -       return register_pernet_subsys(&mctp_net_ops);
> +out:
> +       return err;
> +
> +fail_rtnl:
> +       unregister_pernet_subsys(&mctp_net_ops);
> +fail_pernet:
> +       dev_remove_pack(&mctp_packet_type);
> +       goto out;
>  }

Just `return err;` here - no need for the backwards goto to the return.

And only if you end up re-rolling the patch: can these labels be err_*,
so that we're consistent with the rest of the file?

Cheers,


Jeremy

Re: [PATCH v2 net 4/6] mctp: Handle error of rtnl_register_module().

[Kuniyuki Iwashima]

From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: Mon, 07 Oct 2024 18:09:59 +0800
> Hi Kuniyuki,
> 
> > Since introduced, mctp has been ignoring the returned value
> > of rtnl_register_module(), which could fail.
> > 
> > Let's handle the errors by rtnl_register_module_many().
> 
> Sounds good!
> 
> Just a couple of minor things inline, but regardless:
> 
> Reviewed-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> > diff --git a/net/mctp/device.c b/net/mctp/device.c
> > index acb97b257428..d70e688ac886 100644
> > --- a/net/mctp/device.c
> > +++ b/net/mctp/device.c
> > @@ -524,25 +524,31 @@ static struct notifier_block mctp_dev_nb = {
> >         .priority = ADDRCONF_NOTIFY_PRIORITY,
> >  };
> >  
> > -void __init mctp_device_init(void)
> > +static struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
> > +       {PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
> > +       {PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
> > +       {PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
> > +};
> 
> Can this (and the other handler arrays) be const? And consequently, the
> pointer argument that you pass to rtnl_register_module_many() from 1/6?

Nice, will do.


> 
> >  int __init mctp_routes_init(void)
> >  {
> > +       int err;
> > +
> >         dev_add_pack(&mctp_packet_type);
> >  
> > -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETROUTE,
> > -                            NULL, mctp_dump_rtinfo, 0);
> > -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWROUTE,
> > -                            mctp_newroute, NULL, 0);
> > -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELROUTE,
> > -                            mctp_delroute, NULL, 0);
> > +       err = register_pernet_subsys(&mctp_net_ops);
> > +       if (err)
> > +               goto fail_pernet;
> > +
> > +       err = rtnl_register_module_many(mctp_route_rtnl_msg_handlers);
> > +       if (err)
> > +               goto fail_rtnl;
> >  
> > -       return register_pernet_subsys(&mctp_net_ops);
> > +out:
> > +       return err;
> > +
> > +fail_rtnl:
> > +       unregister_pernet_subsys(&mctp_net_ops);
> > +fail_pernet:
> > +       dev_remove_pack(&mctp_packet_type);
> > +       goto out;
> >  }
> 
> Just `return err;` here - no need for the backwards goto to the return.
> 
> And only if you end up re-rolling the patch: can these labels be err_*,
> so that we're consistent with the rest of the file?

Sure, will change the labels.

Thanks!