netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH iproute2] libnetlink: Fix memory leak in __rtnl_talk_iov()
@ 2022-12-05  8:47 Gilad Naaman
  2022-12-09 18:20 ` patchwork-bot+netdevbpf
  0 siblings, 1 reply; 4+ messages in thread
From: Gilad Naaman @ 2022-12-05  8:47 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: netdev @ vger . kernel . org, Lahav Schlesinger, Gilad Naaman

From: Lahav Schlesinger <lschlesinger@drivenets.com>

If `__rtnl_talk_iov` fails then callers are not expected to free `answer`.

Currently if `NLMSG_ERROR` was received with an error then the netlink
buffer was stored in `answer`, while still returning an error

This leak can be observed by running this snippet over time.
This triggers an `NLMSG_ERROR` because for each neighbour update, `ip`
will try to query for the name of interface 9999 in the wrong netns.
(which in itself is a separate bug)

 set -e

 ip netns del test-a || true
 ip netns add test-a
 ip netns del test-b || true
 ip netns add test-b

 ip -n test-a netns set test-b auto
 ip -n test-a link add veth_a index 9999 type veth \
  peer name veth_b netns test-b
 ip -n test-b link set veth_b up

 ip -n test-a monitor link address prefix neigh nsid label all-nsid \
  > /dev/null &
 monitor_pid=$!
 clean() {
  kill $monitor_pid
  ip netns del test-a
  ip netns del test-b
 }
 trap clean EXIT

 while true; do
  ip -n test-b neigh add dev veth_b 1.2.3.4 lladdr AA:AA:AA:AA:AA:AA
  ip -n test-b neigh del dev veth_b 1.2.3.4
 done

Fixes: 55870dfe7f8b ("Improve batch and dump times by caching link lookups")
Signed-off-by: Lahav Schlesinger <lschlesinger@drivenets.com>
Signed-off-by: Gilad Naaman <gnaaman@drivenets.com>
---
 lib/libnetlink.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 9af06232..001efc1d 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -1092,14 +1092,19 @@ next:
 						rtnl_talk_error(h, err, errfn);
 				}
 
-				if (answer)
-					*answer = (struct nlmsghdr *)buf;
-				else
+				if (i < iovlen) {
 					free(buf);
-
-				if (i < iovlen)
 					goto next;
-				return error ? -i : 0;
+				}
+
+				if (error) {
+					free(buf);
+					return -i;
+				}
+
+				if (answer)
+					*answer = (struct nlmsghdr *)buf;
+				return 0;
 			}
 
 			if (answer) {
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH iproute2] libnetlink: Fix memory leak in __rtnl_talk_iov()
@ 2022-11-23 16:15 Gilad Naaman
  2022-12-01 16:00 ` Stephen Hemminger
  0 siblings, 1 reply; 4+ messages in thread
From: Gilad Naaman @ 2022-11-23 16:15 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev@vger.kernel.org, Lahav Daniel Schlesinger

If `__rtnl_talk_iov()` fails then callers are not expected to free `answer`.

Currently if `NLMSG_ERROR` was received with an error then the netlink
buffer was stored in `answer`, while still returning an error

This leak can be observed by running this snippet over time.
This triggers an `NLMSG_ERROR` because for each neighbour update, `ip`
will try to query for the name of interface 9999 in the wrong netns.
(which in itself is a separate bug)

	set -e

	ip netns del test-a || true
	ip netns add test-a
	ip netns del test-b || true
	ip netns add test-b

	ip -n test-a netns set test-b auto
	ip -n test-a link add veth_a index 9999 type veth peer name veth_b netns test-b
	ip -n test-b link set veth_b up

	ip -n test-a monitor link address prefix neigh nsid label all-nsid > /dev/null &
	monitor_pid=$!
	clean() {
		kill $monitor_pid
		ip netns del test-a
		ip netns del test-b
	}
	trap clean EXIT

	while true; do
		ip -n test-b neigh add dev veth_b 1.2.3.4 lladdr AA:AA:AA:AA:AA:AA
		ip -n test-b neigh del dev veth_b 1.2.3.4
	done

Fixes: 55870df ("Improve batch and dump times by caching link lookups")
Signed-off-by: Lahav Schlesinger <lschlesinger@drivenets.com>
Signed-off-by: Gilad Naaman <gnaaman@drivenets.com>
---
lib/libnetlink.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 9af06232..001efc1d 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -1092,14 +1092,19 @@ next:
						rtnl_talk_error(h, err, errfn);
				}

-				if (answer)
-					*answer = (struct nlmsghdr *)buf;
-				else
+				if (i < iovlen) {
					free(buf);
-
-				if (i < iovlen)
					goto next;
-				return error ? -i : 0;
+				}
+
+				if (error) {
+					free(buf);
+					return -i;
+				}
+
+				if (answer)
+					*answer = (struct nlmsghdr *)buf;
+				return 0;
			}

			if (answer) {
--
2.37.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-09 18:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-05  8:47 [PATCH iproute2] libnetlink: Fix memory leak in __rtnl_talk_iov() Gilad Naaman
2022-12-09 18:20 ` patchwork-bot+netdevbpf
  -- strict thread matches above, loose matches on Subject: below --
2022-11-23 16:15 Gilad Naaman
2022-12-01 16:00 ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).