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

[PATCH v3 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 that properly.

This series introduces two helpers, rtnl_register_many() and
rtnl_unregister_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:
  v3
    * Move module *owner to struct rtnl_msg_handler
    * Make struct rtnl_msg_handler args/vars const
    * Update mctp goto labels

  v2: https://lore.kernel.org/netdev/20241004222358.79129-1-kuniyu@amazon.com/
    * 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             | 17 +++++++++++++++
 net/bridge/br_netlink.c             |  6 +++++-
 net/bridge/br_private.h             |  5 +++--
 net/bridge/br_vlan.c                | 19 ++++++++---------
 net/core/rtnetlink.c                | 29 +++++++++++++++++++++++++
 net/mctp/af_mctp.c                  |  6 +++++-
 net/mctp/device.c                   | 30 +++++++++++++++-----------
 net/mctp/neigh.c                    | 31 ++++++++++++++++-----------
 net/mctp/route.c                    | 33 ++++++++++++++++++++---------
 net/mpls/af_mpls.c                  | 32 ++++++++++++++++++----------
 net/phonet/pn_netlink.c             | 28 ++++++++++--------------
 15 files changed, 176 insertions(+), 89 deletions(-)

-- 
2.30.2

[PATCH v3 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 | 17 +++++++++++++++++
 net/core/rtnetlink.c    | 29 +++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index b45d57b5968a..2d3eb7cb4dff 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -29,6 +29,15 @@ static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
 	return msgtype & RTNL_KIND_MASK;
 }
 
+struct rtnl_msg_handler {
+	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 +45,14 @@ 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(const struct rtnl_msg_handler *handlers, int n);
+void __rtnl_unregister_many(const struct rtnl_msg_handler *handlers, int n);
+
+#define rtnl_register_many(handlers)				\
+	__rtnl_register_many(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..e30e7ea0207d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -384,6 +384,35 @@ void rtnl_unregister_all(int protocol)
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister_all);
 
+int __rtnl_register_many(const struct rtnl_msg_handler *handlers, int n)
+{
+	const struct rtnl_msg_handler *handler;
+	int i, err;
+
+	for (i = 0, handler = handlers; i < n; i++, handler++) {
+		err = rtnl_register_internal(handler->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(const struct rtnl_msg_handler *handlers, int n)
+{
+	const 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 v3 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_many().

Fixes: f9c4bb0b245c ("vxlan: vni filtering support on collect metadata device")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
---
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..d2023e7131bd 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 const struct rtnl_msg_handler vxlan_vnifilter_rtnl_msg_handlers[] = {
+	{THIS_MODULE, PF_BRIDGE, RTM_GETTUNNEL, NULL, vxlan_vnifilter_dump, 0},
+	{THIS_MODULE, PF_BRIDGE, RTM_NEWTUNNEL, vxlan_vnifilter_process, NULL, 0},
+	{THIS_MODULE, 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_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 v3 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_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>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
---
Cc: Roopa Prabhu <roopa@nvidia.com>
---
 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..89f51ea4cabe 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 const struct rtnl_msg_handler br_vlan_rtnl_msg_handlers[] = {
+	{THIS_MODULE, PF_BRIDGE, RTM_NEWVLAN, br_vlan_rtm_process, NULL, 0},
+	{THIS_MODULE, PF_BRIDGE, RTM_DELVLAN, br_vlan_rtm_process, NULL, 0},
+	{THIS_MODULE, 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_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 v3 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_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>
Reviewed-by: 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   | 33 +++++++++++++++++++++++----------
 5 files changed, 66 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..85cc5f31f1e7 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 const struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
+	{THIS_MODULE, PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
+	{THIS_MODULE, PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
+	{THIS_MODULE, 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_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..590f642413e4 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 const struct rtnl_msg_handler mctp_neigh_rtnl_msg_handlers[] = {
+	{THIS_MODULE, PF_MCTP, RTM_NEWNEIGH, mctp_rtm_newneigh, NULL, 0},
+	{THIS_MODULE, PF_MCTP, RTM_DELNEIGH, mctp_rtm_delneigh, NULL, 0},
+	{THIS_MODULE, 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_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..597e9cf5aa64 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -1474,26 +1474,39 @@ static struct pernet_operations mctp_net_ops = {
 	.exit = mctp_routes_net_exit,
 };
 
+static const struct rtnl_msg_handler mctp_route_rtnl_msg_handlers[] = {
+	{THIS_MODULE, PF_MCTP, RTM_NEWROUTE, mctp_newroute, NULL, 0},
+	{THIS_MODULE, PF_MCTP, RTM_DELROUTE, mctp_delroute, NULL, 0},
+	{THIS_MODULE, 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 err_pernet;
+
+	err = rtnl_register_many(mctp_route_rtnl_msg_handlers);
+	if (err)
+		goto err_rtnl;
 
-	return register_pernet_subsys(&mctp_net_ops);
+	return 0;
+
+err_rtnl:
+	unregister_pernet_subsys(&mctp_net_ops);
+err_pernet:
+	dev_remove_pack(&mctp_packet_type);
+	return err;
 }
 
 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 v3 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_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..df62638b6498 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 const struct rtnl_msg_handler mpls_rtnl_msg_handlers[] __initdata_or_module = {
+	{THIS_MODULE, PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0},
+	{THIS_MODULE, PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0},
+	{THIS_MODULE, PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0},
+	{THIS_MODULE, 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_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 v3 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_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 | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index 7008d402499d..894e5c72d6bf 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -285,23 +285,17 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	return err;
 }
 
+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},
+};
+
 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_many(phonet_rtnl_msg_handlers);
 }
-- 
2.30.2

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

[Eric W. Biederman]

Kuniyuki Iwashima <kuniyu@amazon.com> writes:

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

As I recall that was deliberate.  The module continues to work if the
rtnetlink handlers don't operate, just some functionality is lost.

I don't strongly care either way, but I want to point out that bailing
out due to a memory allocation failure actually makes the module
initialization more brittle.

> Let's handle the errors by rtnl_register_many().

Can you describe what the benefit is from completely giving up in the
face of a memory allocation failure versus having as much of the module
function as possible?

> Fixes: 03c0566542f4 ("mpls: Netlink commands to add, remove, and dump routes")

A fix?  Not really no.  You are making the code work less well in the
face of adversity.  I really don't think that is a fix.  Certainly not
without a better justification.

I get this as a code cleanup to make things more uniform and easier
to reason about.  Still you are adding more code paths that are going
to go untested so I don't see how this winds up being any better
in practice that deliberately limping along.

Eric


> 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..df62638b6498 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 const struct rtnl_msg_handler mpls_rtnl_msg_handlers[] __initdata_or_module = {
> +	{THIS_MODULE, PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0},
> +	{THIS_MODULE, PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0},
> +	{THIS_MODULE, PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0},
> +	{THIS_MODULE, 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_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;

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

[Kuniyuki Iwashima]

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Mon, 07 Oct 2024 09:56:44 -0500
> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> 
> > Since introduced, mpls_init() has been ignoring the returned
> > value of rtnl_register_module(), which could fail.
> 
> As I recall that was deliberate.  The module continues to work if the
> rtnetlink handlers don't operate, just some functionality is lost.

It's ok if it wasn't a module.  rtnl_register() logs an error message
in syslog, but rtnl_register_module() doesn't.  That's why this series
only changes some rtnl_register_module() calls.


> 
> I don't strongly care either way, but I want to point out that bailing
> out due to a memory allocation failure actually makes the module
> initialization more brittle.
> 
> > Let's handle the errors by rtnl_register_many().
> 
> Can you describe what the benefit is from completely giving up in the
> face of a memory allocation failure versus having as much of the module
> function as possible?

What if the memory pressure happend to be relaxed soon after the module
was loaded incompletely ?

Silent failure is much worse to me.

rtnl_get_link() will return NULL and users will see -EOPNOTSUPP even
though the module was loaded "successfully".

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

[Eric W. Biederman]

Kuniyuki Iwashima <kuniyu@amazon.com> writes:

> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Mon, 07 Oct 2024 09:56:44 -0500
>> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
>> 
>> > Since introduced, mpls_init() has been ignoring the returned
>> > value of rtnl_register_module(), which could fail.
>> 
>> As I recall that was deliberate.  The module continues to work if the
>> rtnetlink handlers don't operate, just some functionality is lost.
>
> It's ok if it wasn't a module.  rtnl_register() logs an error message
> in syslog, but rtnl_register_module() doesn't.  That's why this series
> only changes some rtnl_register_module() calls.

You talk about the series.  Is there an introductory letter I should
lookup on netdev that explains things in more detail?

I have only seen the patch that is sent directly to me.

>> I don't strongly care either way, but I want to point out that bailing
>> out due to a memory allocation failure actually makes the module
>> initialization more brittle.
>> 
>> > Let's handle the errors by rtnl_register_many().
>> 
>> Can you describe what the benefit is from completely giving up in the
>> face of a memory allocation failure versus having as much of the module
>> function as possible?
>
> What if the memory pressure happend to be relaxed soon after the module
> was loaded incompletely ?

Huh?  The module will load completely.  It will just won't have full
functionality.  The rtnetlink functionality simply won't work.

> Silent failure is much worse to me.

My point is from the point of view of the MPLS functionality it isn't
a __failure__.

> rtnl_get_link() will return NULL and users will see -EOPNOTSUPP even
> though the module was loaded "successfully".

Yes.  EOPNOTSUPP makes it clear that the rtnetlink functionality
working.  In most cases modules are autoloaded these days, so the end
user experience is likely to be EOPNOTSUPP in either case.

If you log a message, some time later someone will see that there is
a message in the log that the kernel was very low on memory and could
could not allocate enough memory for rtnetlink.

Short of rebooting or retrying to load the module I don't expect
there is much someone can do in either case.  So this does not look
to me like a case of silent failure or a broken module.

Has anyone actually had this happen and reported this as a problem?
Otherwise we are all just arguing theoretical possibilities.

My only real point is that change is *not* a *fix*.
This change is a *cleanup* to make mpls like other modules.

I am fine with a cleanup, but I really don't think we should describe
this as something it is not.

The flip side is I tried very hard to minimize the amount of code in
af_mpls, to make maintenance simpler, and to reduce the chance of bugs.
You are busy introducing what appears to me to be an untested error
handling path which may result in something worse that not logging a
message.  Especially when someone comes along and makes another change.

It is all such a corner case and I really don't care, but I just don't
want this to be seen as a change that is obviously the right way to go
and that has no downside.

Eric

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

[Kuniyuki Iwashima]

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Mon, 07 Oct 2024 11:28:11 -0500
> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> 
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > Date: Mon, 07 Oct 2024 09:56:44 -0500
> >> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> >> 
> >> > Since introduced, mpls_init() has been ignoring the returned
> >> > value of rtnl_register_module(), which could fail.
> >> 
> >> As I recall that was deliberate.  The module continues to work if the
> >> rtnetlink handlers don't operate, just some functionality is lost.
> >
> > It's ok if it wasn't a module.  rtnl_register() logs an error message
> > in syslog, but rtnl_register_module() doesn't.  That's why this series
> > only changes some rtnl_register_module() calls.
> 
> You talk about the series.  Is there an introductory letter I should
> lookup on netdev that explains things in more detail?
> 
> I have only seen the patch that is sent directly to me.

Some context here.
https://lore.kernel.org/netdev/20241007124459.5727-1-kuniyu@amazon.com/

Before addf9b90de22, rtnl_register_module() didn't actually need
error handling for some callers, but even after the commit, some
modules copy-and-pasted the wrong code.


> 
> >> I don't strongly care either way, but I want to point out that bailing
> >> out due to a memory allocation failure actually makes the module
> >> initialization more brittle.
> >> 
> >> > Let's handle the errors by rtnl_register_many().
> >> 
> >> Can you describe what the benefit is from completely giving up in the
> >> face of a memory allocation failure versus having as much of the module
> >> function as possible?
> >
> > What if the memory pressure happend to be relaxed soon after the module
> > was loaded incompletely ?
> 
> Huh?  The module will load completely.  It will just won't have full
> functionality.  The rtnetlink functionality simply won't work.
> 
> > Silent failure is much worse to me.
> 
> My point is from the point of view of the MPLS functionality it isn't
> a __failure__.

My point is it _is_ failure for those who use MPLS rtnetlink
functionality, it's uAPI.  Not everyone uses the plain MPLS
without rtnetlink.

Also, I don't want to waste resource due to such an issue on QEMU w/ 2GB
RAM where I'm running syzkaller and often see OOM.  syzkaller searches
and loads modules and exercises various uAPIs randomly, and it handles
module-load-failure properly.


> 
> > rtnl_get_link() will return NULL and users will see -EOPNOTSUPP even
> > though the module was loaded "successfully".
> 
> Yes.  EOPNOTSUPP makes it clear that the rtnetlink functionality
> working.  In most cases modules are autoloaded these days, so the end
> user experience is likely to be EOPNOTSUPP in either case.
> 
> If you log a message, some time later someone will see that there is
> a message in the log that the kernel was very low on memory and could
> could not allocate enough memory for rtnetlink.
> 
> Short of rebooting or retrying to load the module I don't expect
> there is much someone can do in either case.  So this does not look
> to me like a case of silent failure or a broken module.
> 
> Has anyone actually had this happen and reported this as a problem?
> Otherwise we are all just arguing theoretical possibilities.
> 
> My only real point is that change is *not* a *fix*.
> This change is a *cleanup* to make mpls like other modules.
> 
> I am fine with a cleanup, but I really don't think we should describe
> this as something it is not.
> 
> The flip side is I tried very hard to minimize the amount of code in
> af_mpls, to make maintenance simpler, and to reduce the chance of bugs.
> You are busy introducing what appears to me to be an untested error
> handling path which may result in something worse that not logging a
> message.  Especially when someone comes along and makes another change.
> 
> It is all such a corner case and I really don't care, but I just don't
> want this to be seen as a change that is obviously the right way to go
> and that has no downside.

I don't see how this small change has downside that affects maintenability.
Someone who wants to add a new code there can just add a new function call
and goto label, that's simple enough.

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

[Eric W. Biederman]

Kuniyuki Iwashima <kuniyu@amazon.com> writes:

> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Mon, 07 Oct 2024 11:28:11 -0500
>> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
>> 
>> > From: "Eric W. Biederman" <ebiederm@xmission.com>
>> > Date: Mon, 07 Oct 2024 09:56:44 -0500
>> >> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
>> >> 
>> >> > Since introduced, mpls_init() has been ignoring the returned
>> >> > value of rtnl_register_module(), which could fail.
>> >> 
>> >> As I recall that was deliberate.  The module continues to work if the
>> >> rtnetlink handlers don't operate, just some functionality is lost.
>> >
>> > It's ok if it wasn't a module.  rtnl_register() logs an error message
>> > in syslog, but rtnl_register_module() doesn't.  That's why this series
>> > only changes some rtnl_register_module() calls.
>> 
>> You talk about the series.  Is there an introductory letter I should
>> lookup on netdev that explains things in more detail?
>> 
>> I have only seen the patch that is sent directly to me.
>
> Some context here.
> https://lore.kernel.org/netdev/20241007124459.5727-1-kuniyu@amazon.com/
>
> Before addf9b90de22, rtnl_register_module() didn't actually need
> error handling for some callers, but even after the commit, some
> modules copy-and-pasted the wrong code.

That is wrong. As far back as commit e284986385b6 ("[RTNL]: Message
handler registration interface") it was possible for rtnl_register to
error.  Of course that dates back to the time when small allocations
were guaranteed to succeed or panic the kernel.  So I expect it was
the change to small allocations that actually made it possible to
see failures from rtnl_register.

That said there is a difference between code that generates an error,
and callers that need to handle the error.  Even today I do not
see that MPLS needs to handle the error.

Other than for policy objectives such as making it harder for
syzkaller to get confused, and making the code more like other
callers I don't see a need for handling such an error in MPLS.

>> > What if the memory pressure happend to be relaxed soon after the module
>> > was loaded incompletely ?
>> 
>> Huh?  The module will load completely.  It will just won't have full
>> functionality.  The rtnetlink functionality simply won't work.
>> 
>> > Silent failure is much worse to me.
>> 
>> My point is from the point of view of the MPLS functionality it isn't
>> a __failure__.
>
> My point is it _is_ failure for those who use MPLS rtnetlink
> functionality, it's uAPI.  Not everyone uses the plain MPLS
> without rtnetlink.

No matter what the code does userspace attempting to use rtnetlink to
configure mpls will fail.  Either the MPLS module will fail to load, and
nothing works, or the module will load and do the best it can with what
it has.  Allowing the folks you don't need the rtnetlink uAPI to
succeed.

It isn't a uAPI failure.  It is a lack of uAPI availability.  There
is nothing else it can be.

In general the kernel tries to limp along the best it can without
completely failing.

> Also, I don't want to waste resource due to such an issue on QEMU w/ 2GB
> RAM where I'm running syzkaller and often see OOM.  syzkaller searches
> and loads modules and exercises various uAPIs randomly, and it handles
> module-load-failure properly.

Then please mention that use case in your change description.  That is
the only real motivation I see to for this change in behavior. Perhaps
something like:

    Handler errors from rtnetlink registration allowing syzkaller to view a
    module as an all or nothing thing in terms of the functionality it
    provides.  This prevents syzkaller from reporting spurious errors
    from it's tests.

That seems like a fine motivation and a fine reason.  But if you don't
say that, people will make changes that don't honor that, and people
won't be able to look into the history and figure out why such a change
was made.

Recording the real reasons for changes in the history really matters.
Because sometimes people need to revisit things, and if you don't
include those reasons people don't have a chance of taking your reaons
into account when the revisit a change for another set of reasons.
Unless somehow someone remembers something when it comes to code review
time.


>> The flip side is I tried very hard to minimize the amount of code in
>> af_mpls, to make maintenance simpler, and to reduce the chance of bugs.
>> You are busy introducing what appears to me to be an untested error
>> handling path which may result in something worse that not logging a
>> message.  Especially when someone comes along and makes another change.
>> 
>> It is all such a corner case and I really don't care, but I just don't
>> want this to be seen as a change that is obviously the right way to go
>> and that has no downside.
>
> I don't see how this small change has downside that affects maintenability.
> Someone who wants to add a new code there can just add a new function call
> and goto label, that's simple enough.

Strictly speaking when testing code both branches of every if statement
need to be tested.  It is the untested code where mistakes slip in.
People being human we don't get it right 100% of the time.

It isn't a large maintenance cost increase, but it is a maintenance cost
increase.

One if statement isn't a big deal but at the end of the day it adds up.

If code becomes simpler and there is noticeably less for it to do the
code winds up having measurably fewer bugs.  If code becomes more
complex, with more cases to handle and more branches after a time code
winds up having measurably more bugs.

When I look at any part of the linux kernel with which I am familiar I
can very much guarantee that most of it has become more complex over
time, leading to measurably more bugs.   Tools like syzkaller help, but
that don't completely compensate for more complex code.



In this case I expect in balance fewer spurious errors from syzkall
is seems like it would be a net win.  But I most definitely see a
tradeoff being made here.

Eric

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

[Kuniyuki Iwashima]

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Mon, 07 Oct 2024 17:18:59 -0500
> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> 
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > Date: Mon, 07 Oct 2024 11:28:11 -0500
> >> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> >> 
> >> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> >> > Date: Mon, 07 Oct 2024 09:56:44 -0500
> >> >> Kuniyuki Iwashima <kuniyu@amazon.com> writes:
> >> >> 
> >> >> > Since introduced, mpls_init() has been ignoring the returned
> >> >> > value of rtnl_register_module(), which could fail.
> >> >> 
> >> >> As I recall that was deliberate.  The module continues to work if the
> >> >> rtnetlink handlers don't operate, just some functionality is lost.
> >> >
> >> > It's ok if it wasn't a module.  rtnl_register() logs an error message
> >> > in syslog, but rtnl_register_module() doesn't.  That's why this series
> >> > only changes some rtnl_register_module() calls.
> >> 
> >> You talk about the series.  Is there an introductory letter I should
> >> lookup on netdev that explains things in more detail?
> >> 
> >> I have only seen the patch that is sent directly to me.
> >
> > Some context here.
> > https://lore.kernel.org/netdev/20241007124459.5727-1-kuniyu@amazon.com/
> >
> > Before addf9b90de22, rtnl_register_module() didn't actually need
> > error handling for some callers, but even after the commit, some
> > modules copy-and-pasted the wrong code.
> 
> That is wrong. As far back as commit e284986385b6 ("[RTNL]: Message
> handler registration interface") it was possible for rtnl_register to
> error.  Of course that dates back to the time when small allocations
> were guaranteed to succeed or panic the kernel.  So I expect it was
> the change to small allocations that actually made it possible to
> see failures from rtnl_register.

I mean as of addf9b90de22~1, once rtnl_msg_handlers[protocol] was
allocated by rtnl_register() or rtnl_register_module(), the following
calls for the same protocol never failed.

For example, after rtnl_register() was called for PF_UNSPEC/PF_BRIDGE
from the core code, rtnl_register_module() for the protocols didn't
fail, thus some callers didn't need error handling.

Another example is PHONET, where only the first call of
rtnl_register_module() handled the error, which was correct before
addf9b90de22 but not after that commit.


> 
> That said there is a difference between code that generates an error,
> and callers that need to handle the error.  Even today I do not
> see that MPLS needs to handle the error.
> 
> Other than for policy objectives such as making it harder for
> syzkaller to get confused, and making the code more like other
> callers I don't see a need for handling such an error in MPLS.
> 
> >> > What if the memory pressure happend to be relaxed soon after the module
> >> > was loaded incompletely ?
> >> 
> >> Huh?  The module will load completely.  It will just won't have full
> >> functionality.  The rtnetlink functionality simply won't work.
> >> 
> >> > Silent failure is much worse to me.
> >> 
> >> My point is from the point of view of the MPLS functionality it isn't
> >> a __failure__.
> >
> > My point is it _is_ failure for those who use MPLS rtnetlink
> > functionality, it's uAPI.  Not everyone uses the plain MPLS
> > without rtnetlink.
> 
> No matter what the code does userspace attempting to use rtnetlink to
> configure mpls will fail.  Either the MPLS module will fail to load, and
> nothing works, or the module will load and do the best it can with what
> it has.  Allowing the folks you don't need the rtnetlink uAPI to
> succeed.
> 
> It isn't a uAPI failure.  It is a lack of uAPI availability.  There
> is nothing else it can be.

The former is much easier to understand what happened and what's
the next action, just retrying.  The latter lets lsmod show the
module is actually loaded, but there's no hint indicating the module
lacks the rtnetlink availability at the load time.

It's even worse if only a part of rtnetlink handlers is loaded, like
RTM_NEWROUTE works but RTM_GET/DELROUTE returns -ENOTSUPP, it's really
confusing.


> 
> In general the kernel tries to limp along the best it can without
> completely failing.
> 
> > Also, I don't want to waste resource due to such an issue on QEMU w/ 2GB
> > RAM where I'm running syzkaller and often see OOM.  syzkaller searches
> > and loads modules and exercises various uAPIs randomly, and it handles
> > module-load-failure properly.
> 
> Then please mention that use case in your change description.  That is
> the only real motivation I see to for this change in behavior. Perhaps
> something like:
> 
>     Handler errors from rtnetlink registration allowing syzkaller to view a
>     module as an all or nothing thing in terms of the functionality it
>     provides.  This prevents syzkaller from reporting spurious errors
>     from it's tests.
> 
> That seems like a fine motivation and a fine reason.  But if you don't
> say that, people will make changes that don't honor that, and people
> won't be able to look into the history and figure out why such a change
> was made.

I see, I'll add that sentences in v4.

pw-bot: cr