* [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
* Re: [PATCH iproute2] libnetlink: Fix memory leak in __rtnl_talk_iov()
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
0 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-12-09 18:20 UTC (permalink / raw)
To: Gilad Naaman; +Cc: stephen, netdev, lschlesinger
Hello:
This patch was applied to iproute2/iproute2.git (main)
by Stephen Hemminger <stephen@networkplumber.org>:
On Mon, 5 Dec 2022 10:47:41 +0200 you wrote:
> 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
>
> [...]
Here is the summary with links:
- [iproute2] libnetlink: Fix memory leak in __rtnl_talk_iov()
https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=0faec4d050b6
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [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, 0 replies; 4+ messages in thread
From: Stephen Hemminger @ 2022-12-01 16:00 UTC (permalink / raw)
To: Gilad Naaman; +Cc: netdev, Lahav Daniel Schlesinger
On Wed, 23 Nov 2022 16:15:41 +0000
Gilad Naaman <gnaaman@drivenets.com> wrote:
> 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(-)
The patch got mangled by MS Exchange. Please resubmit with proper formatting.
$ checkpatch.pl /tmp/leak.mbox
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#148:
If `__rtnl_talk_iov()` fails then callers are not expected to free `answer`=
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 55870dfe7f8b ("Improve batch and dump times by caching link lookups")'
#186:
Fixes: 55870df ("Improve batch and dump times by caching link lookups")
ERROR: patch seems to be corrupt (line wrapped?)
#198: FILE: lib/libnetlink.c:1091:
rtnl_talk_error(h, err, errfn);
WARNING: suspect code indent for conditional statements (32, 32)
#204: FILE: lib/libnetlink.c:1093:
+ if (i < iovlen) {
free(buf);
WARNING: space prohibited between function name and open parenthesis '('
#218: FILE: lib/libnetlink.c:1102:
+ *answer =3D (struct nlmsghdr *)buf;
ERROR: spaces required around that '=' (ctx:WxV)
#218: FILE: lib/libnetlink.c:1102:
+ *answer =3D (struct nlmsghdr *)buf;
^
total: 2 errors, 4 warnings, 29 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/tmp/leak.mbox has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
^ permalink raw reply [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, 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).