NetBSD Problem Report #51356

From www@NetBSD.org  Mon Jul 25 04:52:08 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id DF58D7AAAE
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 25 Jul 2016 04:52:08 +0000 (UTC)
Message-Id: <20160725045207.9B5A87AAB7@mollari.NetBSD.org>
Date: Mon, 25 Jul 2016 04:52:07 +0000 (UTC)
From: ozaki-r@netbsd.org
Reply-To: ozaki-r@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: Repeating ifconfig <ip>/ifconfig <ip> delete under network load causes a panic
X-Send-Pr-Version: www-1.0

>Number:         51356
>Category:       kern
>Synopsis:       Repeating ifconfig <ip>/ifconfig <ip> delete under network load causes a panic
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jul 25 04:55:00 +0000 2016
>Closed-Date:    Sun May 15 07:19:36 +0000 2022
>Last-Modified:  Sun May 15 07:19:36 +0000 2022
>Originator:     Ryota Ozaki
>Release:        -current, netbsd-7 (and maybe netbsd-6)
>Organization:
>Environment:
NetBSD kvm 7.99.34 NetBSD 7.99.34 (KVM) #215: Mon Jul 25 12:42:10 JST 2016  ozaki-r@rangeley:(hidden) amd64
NetBSD netbsd7 7.0.0_PATCH NetBSD 7.0.0_PATCH (GENERIC.201512132100Z) amd64
>Description:
Adding and deleting an IP address is executed without softnet_lock, but that's unsafe
and causes a panic on rtcache or rtentry manipulations. We have to hold softnet_lock
during such operations.
>How-To-Repeat:
Setup a machine with IP forwarding enabled:

  sysctl -w net.inet.ip.forwarding=1
  ifconfig wm0 10.0.1.1/24
  ifconfig wm1 10.0.2.1/24

Sending massive UDP packets over the machine and run the following script on the machine:

  while true; do ifconfig wm0 10.0.1.1/24 >/dev/null 2>&1; ifconfig wm0 10.0.1.1/24 delete >/dev/null 2>&1; done &
  while true; do ifconfig wm1 10.0.2.1/24 >/dev/null 2>&1; ifconfig wm1 10.0.2.1/24 delete >/dev/null 2>&1; done &

And wait several minutes.
>Fix:
Hold softnet_lock in in_control (and in6_control). We also need to tweak callout_halt for DAD.

diff --git a/sys/netinet/if_arp.c b/sys/netinet/if_arp.c
index 3ae4e4d..e347315 100644
--- a/sys/netinet/if_arp.c
+++ b/sys/netinet/if_arp.c
@@ -1502,7 +1502,7 @@ static void
 arp_dad_stoptimer(struct dadq *dp)
 {

-	callout_halt(&dp->dad_timer_ch, NULL);
+	callout_halt(&dp->dad_timer_ch, softnet_lock);
 }

 static void
diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index 7619d6b..90354f8 100644
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -360,8 +360,8 @@ in_len2mask(struct in_addr *mask, u_int len)
  * Ifp is 0 if not an interface-specific ioctl.
  */
 /* ARGSUSED */
-int
-in_control(struct socket *so, u_long cmd, void *data, struct ifnet *ifp)
+static int
+in_control0(struct socket *so, u_long cmd, void *data, struct ifnet *ifp)
 {
 	struct ifreq *ifr = (struct ifreq *)data;
 	struct in_ifaddr *ia = NULL;
@@ -667,6 +667,18 @@ in_control(struct socket *so, u_long cmd, void *data, struct ifnet *ifp)
 	return error;
 }

+int
+in_control(struct socket *so, u_long cmd, void *data, struct ifnet *ifp)
+{
+	int error;
+
+	mutex_enter(softnet_lock);
+	error = in_control0(so, cmd, data, ifp);
+	mutex_exit(softnet_lock);
+
+	return error;
+}
+
 /* Add ownaddr as loopback rtentry. */
 static void
 in_ifaddlocal(struct ifaddr *ifa)
diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c
index ead9154..54e5823 100644
--- a/sys/netinet6/in6.c
+++ b/sys/netinet6/in6.c
@@ -751,7 +751,9 @@ in6_control(struct socket *so, u_long cmd, void *data, struct ifnet *ifp)
 	}

 	s = splnet();
+	mutex_enter(softnet_lock);
 	error = in6_control1(so , cmd, data, ifp);
+	mutex_exit(softnet_lock);
 	splx(s);
 	return error;
 }
diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index a3ee9b9..6066460 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -1074,7 +1074,7 @@ static void
 nd6_dad_stoptimer(struct dadq *dp)
 {

-	callout_halt(&dp->dad_timer_ch, NULL);
+	callout_halt(&dp->dad_timer_ch, softnet_lock);
 }

 /*

>Release-Note:

>Audit-Trail:
From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51356 CVS commit: src/sys
Date: Thu, 28 Jul 2016 09:03:51 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Thu Jul 28 09:03:51 UTC 2016

 Modified Files:
 	src/sys/netinet: if_arp.c in.c
 	src/sys/netinet6: in6.c nd6_nbr.c

 Log Message:
 Fix panic on adding/deleting IP addresses under network load

 Adding and deleting IP addresses aren't serialized with other network
 opeartions, e.g., forwarding packets. So if we add or delete an IP
 address under network load, a kernel panic may happen on manipulating
 network-related shared objects such as rtentry and rtcache.

 To avoid such panicks, we still need to hold softnet_lock in in_control
 and in6_control that are called via ioctl and do network-related operations
 including IP address additions/deletions.

 Fix PR kern/51356


 To generate a diff of this commit:
 cvs rdiff -u -r1.219 -r1.220 src/sys/netinet/if_arp.c
 cvs rdiff -u -r1.176 -r1.177 src/sys/netinet/in.c
 cvs rdiff -u -r1.211 -r1.212 src/sys/netinet6/in6.c
 cvs rdiff -u -r1.125 -r1.126 src/sys/netinet6/nd6_nbr.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: dholland@NetBSD.org
State-Changed-When: Sun, 31 Jul 2016 19:46:53 +0000
State-Changed-Why:
should get into -7


From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51356 CVS commit: src/sys
Date: Sat, 24 Feb 2018 07:37:09 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Sat Feb 24 07:37:09 UTC 2018

 Modified Files:
 	src/sys/netinet: in.c ip_input.c wqinput.c
 	src/sys/netinet6: in6.c ip6_input.c
 	src/sys/rump/net/lib/libnetinet: netinet_component.c

 Log Message:
 Avoid a deadlock between softnet_lock and IFNET_LOCK

 A deadlock occurs because there is a violation of the rule of lock ordering;
 softnet_lock is held with hodling IFNET_LOCK, which violates the rule.
 To avoid the deadlock, replace softnet_lock in in_control and in6_control
 with KERNEL_LOCK.

 We also need to add some KERNEL_LOCKs to protect the network stack surely.
 This is required, for example, for PR kern/51356.

 Fix PR kern/53043


 To generate a diff of this commit:
 cvs rdiff -u -r1.218 -r1.219 src/sys/netinet/in.c
 cvs rdiff -u -r1.375 -r1.376 src/sys/netinet/ip_input.c
 cvs rdiff -u -r1.3 -r1.4 src/sys/netinet/wqinput.c
 cvs rdiff -u -r1.259 -r1.260 src/sys/netinet6/in6.c
 cvs rdiff -u -r1.192 -r1.193 src/sys/netinet6/ip6_input.c
 cvs rdiff -u -r1.10 -r1.11 \
     src/sys/rump/net/lib/libnetinet/netinet_component.c

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51356 CVS commit: [netbsd-8] src/sys
Date: Mon, 26 Feb 2018 13:32:01 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Feb 26 13:32:01 UTC 2018

 Modified Files:
 	src/sys/netinet [netbsd-8]: in.c ip_input.c wqinput.c
 	src/sys/netinet6 [netbsd-8]: in6.c ip6_input.c
 	src/sys/rump/net/lib/libnetinet [netbsd-8]: netinet_component.c

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #588):
 	sys/netinet6/in6.c: revision 1.260
 	sys/netinet/in.c: revision 1.219
 	sys/netinet/wqinput.c: revision 1.4
 	sys/rump/net/lib/libnetinet/netinet_component.c: revision 1.11
 	sys/netinet/ip_input.c: revision 1.376
 	sys/netinet6/ip6_input.c: revision 1.193
 Avoid a deadlock between softnet_lock and IFNET_LOCK

 A deadlock occurs because there is a violation of the rule of lock ordering;
 softnet_lock is held with hodling IFNET_LOCK, which violates the rule.
 To avoid the deadlock, replace softnet_lock in in_control and in6_control
 with KERNEL_LOCK.

 We also need to add some KERNEL_LOCKs to protect the network stack surely.
 This is required, for example, for PR kern/51356.

 Fix PR kern/53043


 To generate a diff of this commit:
 cvs rdiff -u -r1.203.2.9 -r1.203.2.10 src/sys/netinet/in.c
 cvs rdiff -u -r1.355.2.4 -r1.355.2.5 src/sys/netinet/ip_input.c
 cvs rdiff -u -r1.3 -r1.3.2.1 src/sys/netinet/wqinput.c
 cvs rdiff -u -r1.245.2.7 -r1.245.2.8 src/sys/netinet6/in6.c
 cvs rdiff -u -r1.178.2.5 -r1.178.2.6 src/sys/netinet6/ip6_input.c
 cvs rdiff -u -r1.8.6.1 -r1.8.6.2 \
     src/sys/rump/net/lib/libnetinet/netinet_component.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: dholland@NetBSD.org
State-Changed-When: Sun, 15 May 2022 07:19:36 +0000
State-Changed-Why:
never got into -7, too late now


>Unformatted:

Home	PR Database Search