NetBSD Problem Report #54434

From www@netbsd.org  Sat Aug  3 10:30:48 2019
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 "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 87F5D7A161
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  3 Aug 2019 10:30:48 +0000 (UTC)
Message-Id: <20190803103047.8E3D67A1DB@mollari.NetBSD.org>
Date: Sat,  3 Aug 2019 10:30:47 +0000 (UTC)
From: logix@foobar.franken.de
Reply-To: logix@foobar.franken.de
To: gnats-bugs@NetBSD.org
Subject: ifconfig lo0 destroy leaves lo0 in unusable state
X-Send-Pr-Version: www-1.0

>Number:         54434
>Category:       kern
>Synopsis:       ifconfig lo0 destroy leaves lo0 in unusable state
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Aug 03 10:35:00 +0000 2019
>Closed-Date:    Tue Apr 21 20:08:17 +0000 2020
>Last-Modified:  Tue Apr 21 20:08:17 +0000 2020
>Originator:     Harold Gutch
>Release:        HEAD
>Organization:
>Environment:
NetBSD localhost 9.99.2 NetBSD 9.99.2 (GENERIC) #0: Thu Aug  1 22:23:16 UTC 2019  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
When calling "ifconfig <device> destroy", the generic clone destroy function sets the interface ioctl to if_nullioctl before running the device specific clone destroy function.  If the device specific destroy function fails, the device still exists and works but there is no reference anymore to its ioctl function and the device subsequently cannot be configured anymore until the next reboot.

This can happen for lo0, ppp*, sl*, srt*, strip*.

This was introduced in rev. 1.336 of src/sys/net/if.c.
>How-To-Repeat:
localhost# ifconfig -a
pcn0: flags=0x8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	address: 08:00:27:a8:b0:5a
	inet 192.168.56.101/24 broadcast 192.168.56.255 flags 0x0
	inet6 fe80::a00:27ff:fea8:b05a%pcn0/64 flags 0x0 scopeid 0x1
lo0: flags=0x8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 33624
	inet 127.0.0.1/8 flags 0x0
	inet6 ::1/128 flags 0x20<NODAD>
	inet6 fe80::1%lo0/64 flags 0x0 scopeid 0x2
localhost# ifconfig lo0 destroy
ifconfig: clone_command: Operation not permitted
ifconfig: exec_matches: Operation not permitted
localhost# ifconfig -a
pcn0: flags=0x8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	address: 08:00:27:a8:b0:5a
	inet 192.168.56.101/24 broadcast 192.168.56.255 flags 0x0
	inet6 fe80::a00:27ff:fea8:b05a%pcn0/64 flags 0x0 scopeid 0x1
ifconfig: status: getifinfo: Device not configured
localhost# ifconfig lo0
ifconfig: SIOCGIFFLAGS lo0: Device not configured
localhost# ifconfig -l
pcn0 lo0
>Fix:
This patch seems to fix the issue, but I'm not sure if there are cases where it has side effects.

--- src/sys/net/if.c.orig	2019-07-25 07:45:57.000000000 +0000
+++ src/sys/net/if.c	2019-08-03 12:15:43.318986910 +0000
@@ -1611,6 +1611,8 @@
 	struct if_clone *ifc;
 	struct ifnet *ifp;
 	struct psref psref;
+	int errno;
+	int (*if_ioctl)(struct ifnet *, u_long, void *);

 	KASSERT(mutex_owned(&if_clone_mtx));

@@ -1627,6 +1629,7 @@

 	/* We have to disable ioctls here */
 	IFNET_LOCK(ifp);
+	if_ioctl = ifp->if_ioctl;
 	ifp->if_ioctl = if_nullioctl;
 	IFNET_UNLOCK(ifp);

@@ -1636,7 +1639,13 @@
 	 */
 	if_put(ifp, &psref);

-	return (*ifc->ifc_destroy)(ifp);
+	errno = (*ifc->ifc_destroy)(ifp);
+	if (errno) {
+		IFNET_LOCK(ifp);
+		ifp->if_ioctl = if_ioctl;
+		IFNET_UNLOCK(ifp);
+	}
+	return errno;
 }

 static bool

>Release-Note:

>Audit-Trail:
From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54434 CVS commit: src/sys/net
Date: Thu, 15 Aug 2019 04:20:59 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Thu Aug 15 04:20:59 UTC 2019

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

 Log Message:
 Restore if_ioctl on error of ifc_destroy

 Otherwise subsequence ioctls won't work.

 Patch from Harold Gutch on PR kern/54434 (tweaked a bit by me)


 To generate a diff of this commit:
 cvs rdiff -u -r1.457 -r1.458 src/sys/net/if.c

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

From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54434 CVS commit: src/tests/net/if
Date: Thu, 15 Aug 2019 04:21:33 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Thu Aug 15 04:21:33 UTC 2019

 Modified Files:
 	src/tests/net/if: t_ifconfig.sh

 Log Message:
 tests: check if ifconfig (ioctl) works after a failure of ifconfig destroy

 This is a test for PR kern/54434.


 To generate a diff of this commit:
 cvs rdiff -u -r1.20 -r1.21 src/tests/net/if/t_ifconfig.sh

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

From: Ryota Ozaki <ozaki-r@netbsd.org>
To: logix@foobar.franken.de
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/54434: ifconfig lo0 destroy leaves lo0 in unusable state
Date: Thu, 15 Aug 2019 13:26:16 +0900

 On Sun, Aug 4, 2019 at 4:08 AM <logix@foobar.franken.de> wrote:
 >
 > >Number:         54434
 > >Category:       kern
 > >Synopsis:       ifconfig lo0 destroy leaves lo0 in unusable state
 > >Confidential:   no
 > >Severity:       serious
 > >Priority:       medium
 > >Responsible:    kern-bug-people
 > >State:          open
 > >Class:          sw-bug
 > >Submitter-Id:   net
 > >Arrival-Date:   Sat Aug 03 10:35:00 +0000 2019
 > >Originator:     Harold Gutch
 > >Release:        HEAD
 > >Organization:
 > >Environment:
 > NetBSD localhost 9.99.2 NetBSD 9.99.2 (GENERIC) #0: Thu Aug  1 22:23:16 U=
 TC 2019  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC=
  amd64
 > >Description:
 > When calling "ifconfig <device> destroy", the generic clone destroy funct=
 ion sets the interface ioctl to if_nullioctl before running the device spec=
 ific clone destroy function.  If the device specific destroy function fails=
 , the device still exists and works but there is no reference anymore to it=
 s ioctl function and the device subsequently cannot be configured anymore u=
 ntil the next reboot.
 >
 > This can happen for lo0, ppp*, sl*, srt*, strip*.
 >
 > This was introduced in rev. 1.336 of src/sys/net/if.c.
 > >How-To-Repeat:
 > localhost# ifconfig -a
 > pcn0: flags=3D0x8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
 >         address: 08:00:27:a8:b0:5a
 >         inet 192.168.56.101/24 broadcast 192.168.56.255 flags 0x0
 >         inet6 fe80::a00:27ff:fea8:b05a%pcn0/64 flags 0x0 scopeid 0x1
 > lo0: flags=3D0x8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 33624
 >         inet 127.0.0.1/8 flags 0x0
 >         inet6 ::1/128 flags 0x20<NODAD>
 >         inet6 fe80::1%lo0/64 flags 0x0 scopeid 0x2
 > localhost# ifconfig lo0 destroy
 > ifconfig: clone_command: Operation not permitted
 > ifconfig: exec_matches: Operation not permitted
 > localhost# ifconfig -a
 > pcn0: flags=3D0x8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
 >         address: 08:00:27:a8:b0:5a
 >         inet 192.168.56.101/24 broadcast 192.168.56.255 flags 0x0
 >         inet6 fe80::a00:27ff:fea8:b05a%pcn0/64 flags 0x0 scopeid 0x1
 > ifconfig: status: getifinfo: Device not configured
 > localhost# ifconfig lo0
 > ifconfig: SIOCGIFFLAGS lo0: Device not configured
 > localhost# ifconfig -l
 > pcn0 lo0
 > >Fix:
 > This patch seems to fix the issue, but I'm not sure if there are cases wh=
 ere it has side effects.
 >
 > --- src/sys/net/if.c.orig       2019-07-25 07:45:57.000000000 +0000
 > +++ src/sys/net/if.c    2019-08-03 12:15:43.318986910 +0000
 > @@ -1611,6 +1611,8 @@
 >         struct if_clone *ifc;
 >         struct ifnet *ifp;
 >         struct psref psref;
 > +       int errno;
 > +       int (*if_ioctl)(struct ifnet *, u_long, void *);
 >
 >         KASSERT(mutex_owned(&if_clone_mtx));
 >
 > @@ -1627,6 +1629,7 @@
 >
 >         /* We have to disable ioctls here */
 >         IFNET_LOCK(ifp);
 > +       if_ioctl =3D ifp->if_ioctl;
 >         ifp->if_ioctl =3D if_nullioctl;
 >         IFNET_UNLOCK(ifp);
 >
 > @@ -1636,7 +1639,13 @@
 >          */
 >         if_put(ifp, &psref);
 >
 > -       return (*ifc->ifc_destroy)(ifp);
 > +       errno =3D (*ifc->ifc_destroy)(ifp);
 > +       if (errno) {
 > +               IFNET_LOCK(ifp);
 > +               ifp->if_ioctl =3D if_ioctl;
 > +               IFNET_UNLOCK(ifp);
 > +       }
 > +       return errno;
 >  }
 >
 >  static bool
 >

 Thank you for the fix!  I've committed it (with minor tweaks)
 and will pull it up to NetBSD 8 and 9.

   ozaki-r

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54434 CVS commit: [netbsd-8] src
Date: Mon, 19 Aug 2019 14:27:16 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Aug 19 14:27:16 UTC 2019

 Modified Files:
 	src/sys/net [netbsd-8]: if.c
 	src/tests/net/if [netbsd-8]: t_ifconfig.sh

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #1339):

 	sys/net/if.c: revision 1.458
 	tests/net/if/t_ifconfig.sh: revision 1.21

 Restore if_ioctl on error of ifc_destroy

 Otherwise subsequence ioctls won't work.

 Patch from Harold Gutch on PR kern/54434 (tweaked a bit by me)
 tests: check if ifconfig (ioctl) works after a failure of ifconfig destroy

 This is a test for PR kern/54434.


 To generate a diff of this commit:
 cvs rdiff -u -r1.394.2.16 -r1.394.2.17 src/sys/net/if.c
 cvs rdiff -u -r1.18 -r1.18.4.1 src/tests/net/if/t_ifconfig.sh

 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/54434 CVS commit: [netbsd-9] src
Date: Mon, 19 Aug 2019 16:10:35 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Aug 19 16:10:35 UTC 2019

 Modified Files:
 	src/sys/net [netbsd-9]: if.c
 	src/tests/net/if [netbsd-9]: t_ifconfig.sh

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #98):

 	sys/net/if.c: revision 1.458
 	tests/net/if/t_ifconfig.sh: revision 1.21

 Restore if_ioctl on error of ifc_destroy

 Otherwise subsequence ioctls won't work.

 Patch from Harold Gutch on PR kern/54434 (tweaked a bit by me)
 tests: check if ifconfig (ioctl) works after a failure of ifconfig destroy

 This is a test for PR kern/54434.


 To generate a diff of this commit:
 cvs rdiff -u -r1.457 -r1.457.2.1 src/sys/net/if.c
 cvs rdiff -u -r1.20 -r1.20.2.1 src/tests/net/if/t_ifconfig.sh

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

State-Changed-From-To: open->closed
State-Changed-By: maya@NetBSD.org
State-Changed-When: Tue, 21 Apr 2020 20:08:17 +0000
State-Changed-Why:
Fixed. thanks for the report & patch, and thanks to ozaki-r for working on it.


>Unformatted:

Home	PR Database Search