NetBSD Problem Report #58297

From www@netbsd.org  Tue May 28 21:42:49 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id CEC021A923A
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 28 May 2024 21:42:49 +0000 (UTC)
Message-Id: <20240528214248.8C9791A923C@mollari.NetBSD.org>
Date: Tue, 28 May 2024 21:42:48 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: net/nd.c leaks mbufs pending neighbour discovery
X-Send-Pr-Version: www-1.0

>Number:         58297
>Category:       kern
>Synopsis:       net/nd.c leaks mbufs pending neighbour discovery
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 28 21:45:01 +0000 2024
>Closed-Date:    Thu Sep 12 13:23:54 +0000 2024
>Last-Modified:  Thu Sep 12 13:23:54 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10
>Organization:
The NDBSD Foundation
>Environment:
>Description:
When the network stack needs the link-layer address of the next hop for an outgoing packet, it asks nd_resolve (e.g., via ether_output -> arpresolve).

If nd_resolve doesn't know the link-layer address, it queues up the packet's mbuf in struct llentry::la_hold (a.k.a. ln_hold) for all packets to be transmitted via the same next hop once the link-layer address is resolved (via arp, ipv6nd, ...).  (If the queue exceeds a maximum length -- currently, always 1 -- then older packets are freed.)

From time to time, the still-pending packets are supposed to be freed in lltable_drop_entry_queue, via lltable_drain (from arp_drain, called by arp_fasttimo; XXX no IPv6 callers?) or via llentry_free (from lltable_purge_entries, called by in_if_down, in6_if_down, nd6_purge).

But lltable_drop_entry_queue only frees the queued mbufs if la_numheld is positive.  And nothing ever increments la_numheld.  So whenever an llentry is freed, e.g. when an interface is brought down, it will leak any queued mbufs waiting for their next hop's link-layer address resolution.

Found by Edgar Fuss: https://mail-index.netbsd.org/tech-net/2024/04/23/msg008762.html
>How-To-Repeat:
1. ping a host on the local network that doesn't exist
2. bring the interface down before link-layer resolution has timed out and given up
>Fix:
From 2786a8a8ed3b3223888c8374c5d32162866aec91 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Wed, 24 Apr 2024 14:08:35 +0000
Subject: [PATCH] nd_resolve: Maintain la_numheld.

Otherwise lltable_drop_entry_quene never drops anything.

Addresses mbuf leak, PR kern/XXXXX.
---
 sys/net/nd.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sys/net/nd.c b/sys/net/nd.c
index c9c2ca6fddec..7be385b74693 100644
--- a/sys/net/nd.c
+++ b/sys/net/nd.c
@@ -374,14 +374,25 @@ nd_resolve(struct llentry *ln, const struct rtentry *rt, struct mbuf *m,
 				break;
 			}
 		}
+		KASSERTMSG(ln->la_numheld == i, "la_numheld=%d i=%d",
+		    ln->la_numheld, i);
 		while (i >= nd->nd_maxqueuelen) {
 			m_hold = ln->ln_hold;
 			ln->ln_hold = ln->ln_hold->m_nextpkt;
 			m_freem(m_hold);
 			i--;
+			ln->la_numheld--;
 		}
-	} else
+	} else {
+		KASSERTMSG(ln->la_numheld == 0, "la_numheld=%d",
+		    ln->la_numheld);
 		ln->ln_hold = m;
+	}
+
+	KASSERTMSG(ln->la_numheld < nd->nd_maxqueuelen,
+	    "la_numheld=%d nd_maxqueuelen=%d",
+	    ln->la_numheld, nd->nd_maxqueuelen);
+	ln->la_numheld++;

 	if (ln->ln_asked >= nd->nd_mmaxtries)
 		error = (rt != NULL && rt->rt_flags & RTF_GATEWAY) ?

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58297 CVS commit: src/sys/net
Date: Tue, 28 May 2024 22:15:22 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue May 28 22:15:22 UTC 2024

 Modified Files:
 	src/sys/net: nd.c

 Log Message:
 nd_resolve: Maintain la_numheld.

 Otherwise lltable_drop_entry_queue never drops anything.

 Addresses mbuf leak, PR kern/58297.


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.6 src/sys/net/nd.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58297 CVS commit: src/sys/net
Date: Thu, 30 May 2024 23:00:40 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Thu May 30 23:00:39 UTC 2024

 Modified Files:
 	src/sys/net: nd.c

 Log Message:
 nd_timer: Update la_numheld when we clear la_hold (a.k.a. ln_hold).

 Followup for PR kern/58297 fix.  Patch by mlelstv@.

 PR kern/58301


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/sys/net/nd.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 23 Jul 2024 21:08:03 +0000
State-Changed-Why:
needs pullup-10, but I think this code was new since 9


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58297 CVS commit: [netbsd-10] src/sys/net
Date: Wed, 11 Sep 2024 16:18:36 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Sep 11 16:18:36 UTC 2024

 Modified Files:
 	src/sys/net [netbsd-10]: nd.c

 Log Message:
 Pull up following revision(s) (requested by rin in ticket #827):

 	sys/net/nd.c: revision 1.6
 	sys/net/nd.c: revision 1.7

 nd_resolve: Maintain la_numheld.

 Otherwise lltable_drop_entry_queue never drops anything.

 Addresses mbuf leak, PR kern/58297.

 nd_timer: Update la_numheld when we clear la_hold (a.k.a. ln_hold).

 Followup for PR kern/58297 fix.  Patch by mlelstv@.
 PR kern/58301


 To generate a diff of this commit:
 cvs rdiff -u -r1.5 -r1.5.2.1 src/sys/net/nd.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 12 Sep 2024 13:23:54 +0000
State-Changed-Why:
fixed in HEAD, pulled up to 10, inapplicable <10


>Unformatted:

Home	PR Database Search