netdev - Re: [PATCH net] inet: bring NLM_DONE out to a separate recv() in inet_dump

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 2 Jun 2024 15:21:02 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: David Ahern <dsahern@...nel.org>, Stephen Hemminger
 <stephen@...workplumber.org>, davem@...emloft.net, netdev@...r.kernel.org,
 pabeni@...hat.com, Jaroslav Pulchart <jaroslav.pulchart@...ddata.com>
Subject: Re: [PATCH net] inet: bring NLM_DONE out to a separate recv() in
 inet_dump_ifaddr()

On Sun, 2 Jun 2024 12:00:09 +0200 Eric Dumazet wrote:
> Main motivation for me was to avoid re-grabbing RTNL again just to get NLM_DONE.
> 
> The avoidance of the two system calls was really secondary.
> 
> I think we could make a generic change in netlink_dump() to force NLM_DONE
> in an empty message _and_ avoiding a useless call to the dump method, which
> might still use RTNL or other contended mutex.
> 
> In a prior feedback I suggested a sysctl that Jakub disliked,
> but really we do not care yet, as long as we avoid RTNL as much as we can.
> 
> Jakub, what about the following generic change, instead of ad-hoc changes ?

Netlink is full of legacy behavior, the only way to make it usable
in modern environments is to let new families not repeat the mistakes.
That's why I'd really rather not add a workaround at the af_netlink
level. Why would ethtool (which correctly coalesced NLM_DONE from day 1)
suddenly start needed another recv(). A lot of the time the entire dump
fits in one skb.

If you prefer to sacrifice all of rtnetlink (some of which, to be clear,
has also been correctly coded from day 1) - we can add a trampoline for
rtnetlink dump handlers?

(just to illustrate, not even compile tested)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 522bbd70c205..c59b39ee544f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -6487,6 +6487,18 @@ static int rtnl_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 /* Process one rtnetlink message. */
 
+static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	rtnl_dumpit_func dumpit = cb->data;
+	int err;
+
+	err = dumpit(skb, cb);
+	if (err < 0 && err != -EMSGSIZE)
+		return err;
+
+	return skb->len;
+}
+
 static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			     struct netlink_ext_ack *extack)
 {
@@ -6546,10 +6558,11 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		rtnl = net->rtnl;
 		if (err == 0) {
 			struct netlink_dump_control c = {
-				.dump		= dumpit,
+				.dump		= rtnl_dumpit,
 				.min_dump_alloc	= min_dump_alloc,
 				.module		= owner,
 				.flags		= flags,
+				.data		= dumpit,
 			};
 			err = netlink_dump_start(rtnl, skb, nlh, &c);
 			/* netlink_dump_start() will keep a reference on