NetBSD Problem Report #48964

From www@NetBSD.org  Fri Jul  4 18:16:24 2014
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(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 0CEA5A6547
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  4 Jul 2014 18:16:24 +0000 (UTC)
Message-Id: <20140704181622.D827AA654A@mollari.NetBSD.org>
Date: Fri,  4 Jul 2014 18:16:22 +0000 (UTC)
From: oshima-ya@yagoto-urayama.jp
Reply-To: oshima-ya@yagoto-urayama.jp
To: gnats-bugs@NetBSD.org
Subject: In urndis(4), initialization of ifnet structure is not enough, cause panic.
X-Send-Pr-Version: www-1.0

>Number:         48964
>Category:       kern
>Synopsis:       In urndis(4), initialization of ifnet structure is not enough, cause panic.
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    skrll
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jul 04 18:20:00 +0000 2014
>Closed-Date:    Sun Apr 05 07:10:50 +0000 2015
>Last-Modified:  Sun Apr 05 07:10:50 +0000 2015
>Originator:     Yasushi Oshima
>Release:        NetBSD-current 6.99.45
>Organization:
>Environment:
NetBSD jaguar 6.99.45 NetBSD 6.99.45 (GENERIC) #3: Sat Jul  5 02:30:41 JST 2014  root@sweety:/export/current/daily/20140705/obj/amd64/sys/arch/amd64/compile/GENERIC amd64
>Description:
When running dhcpcd(4) with urndis(4), ioctl(SIOCINITIFADDR) is called and  kernel panic occurs in ether_ioctl(). 

urndis_ioctl(SIOCINITIFADDR) will pass through to ether_ioctl(),
ether_ioctl(SIOCINITIFADDR) will call ifp->if_init() function.
Howerver, this is NULL, wasn't set when attaching urndis(4).

note:
 This problem appears  with dhcpcd version 6.4.0 (or later?)
 Before 6.4 case, it will call ioctl(SOCSIFFLAGS) in the first. so, ifp->if_init() is not called by ether_ioctl().

This problem will exist in netbsd-6, but not appear.
>How-To-Repeat:
On netbsd-current kernel with fix PR kern/48963
and current's dhcpcd ver. 6.4.0 (after 2014.6.16).

1. Attach urndis(4) device
2. Run 'dhcpcd urndis0'
>Fix:
Set urndis_init() to ifp->if_init,
with  change function type void to int (and also add return code)

--- if_urndis.c.old	2014-07-04 19:03:07.000000000 +0900
+++ if_urndis.c	2014-07-05 02:58:13.000000000 +0900
@@ -76,7 +76,7 @@
 static int urndis_rx_list_init(struct urndis_softc *);
 static int urndis_tx_list_init(struct urndis_softc *);

-static void urndis_init(struct ifnet *);
+static int urndis_init(struct ifnet *);
 static void urndis_stop(struct ifnet *);

 static usbd_status urndis_ctrl_msg(struct urndis_softc *, uint8_t, uint8_t,
@@ -1022,35 +1022,35 @@
 }
 #endif

-static void
+static int
 urndis_init(struct ifnet *ifp)
 {
 	struct urndis_softc	*sc;
 	int			 i, s;
-	usbd_status		 err;
+	usbd_status		 err=0;

 	sc = ifp->if_softc;

 	if (ifp->if_flags & IFF_RUNNING)
-		return;
+		return err;

-	if (urndis_ctrl_init(sc) != RNDIS_STATUS_SUCCESS)
-		return;
+	if ((err=urndis_ctrl_init(sc)) != RNDIS_STATUS_SUCCESS)
+		return err;

 	s = splnet();

-	if (urndis_tx_list_init(sc) == ENOBUFS) {
+	if ((err=urndis_tx_list_init(sc)) == ENOBUFS) {
 		printf("%s: tx list init failed\n",
 		    DEVNAME(sc));
 		splx(s);
-		return;
+		return err;
 	}

-	if (urndis_rx_list_init(sc) == ENOBUFS) {
+	if ((err=urndis_rx_list_init(sc)) == ENOBUFS) {
 		printf("%s: rx list init failed\n",
 		    DEVNAME(sc));
 		splx(s);
-		return;
+		return err;
 	}

 	err = usbd_open_pipe(sc->sc_iface_data, sc->sc_bulkin_no,
@@ -1059,7 +1059,7 @@
 		printf("%s: open rx pipe failed: %s\n", DEVNAME(sc),
 		    usbd_errstr(err));
 		splx(s);
-		return;
+		return err;
 	}

 	err = usbd_open_pipe(sc->sc_iface_data, sc->sc_bulkout_no,
@@ -1068,7 +1068,7 @@
 		printf("%s: open tx pipe failed: %s\n", DEVNAME(sc),
 		    usbd_errstr(err));
 		splx(s);
-		return;
+		return err;
 	}

 	for (i = 0; i < RNDIS_RX_LIST_CNT; i++) {
@@ -1086,6 +1086,7 @@
 	ifp->if_flags &= ~IFF_OACTIVE;

 	splx(s);
+	return err;
 }

 static void
@@ -1440,6 +1441,7 @@
 	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
 	ifp->if_start = urndis_start;
 	ifp->if_ioctl = urndis_ioctl;
+	ifp->if_init = urndis_init;
 #if 0
 	ifp->if_watchdog = urndis_watchdog;
 #endif

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->skrll
Responsible-Changed-By: skrll@NetBSD.org
Responsible-Changed-When: Sat, 05 Jul 2014 09:22:50 +0000
Responsible-Changed-Why:
Take


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48964 CVS commit: src/sys/dev/usb
Date: Sat, 5 Jul 2014 11:13:13 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Sat Jul  5 11:13:13 UTC 2014

 Modified Files:
 	src/sys/dev/usb: if_urndis.c

 Log Message:
 PR/48964: In urndis(4), initialization of ifnet structure is not enough,
 cause panic.

 Update urndis_init to return and error and use as if_init based on the
 patch in the PR with stylistic changes from me.

 XXX IFF_RUNNING should be ignored here?


 To generate a diff of this commit:
 cvs rdiff -u -r1.7 -r1.8 src/sys/dev/usb/if_urndis.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->feedback
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sat, 05 Jul 2014 11:22:52 +0000
State-Changed-Why:
I committed a similar patch. Perhaps you can look at the IFF_RUNNING
check in urndis_init?


From: fukumoto@imasy.or.jp
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/48964
Date: Sun, 06 Jul 2014 22:32:33 +0900 (JST)

 When closing this pr, please close kern/46290 as well.  Thank you.


 					FUKUMOTO Atsushi
 					fukumoto@imasy.or.jp

From: Yasushi Oshima <oshima-ya@yagoto-urayama.jp>
To: gnats-bugs@NetBSD.org, skrll@NetBSD.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Cc: 
Subject: Re: kern/48964 (In urndis(4), initialization of ifnet structure is
 not enough, cause panic.)
Date: Mon, 07 Jul 2014 02:31:40 +0900 (JST)

 >  Perhaps you can look at the IFF_RUNNING
 > check in urndis_init?

 [from diff]:

 	if (ifp->if_flags & IFF_RUNNING)
 -	   return;
 +	   return EBUSY;


 I don't know whether if_init() will be called or not when IFF_RUNNING flag is ON,
 it will depend on the design of network (or ethernet) driver.

 It seems that many nic drivers are stopping I/O, resetting and re-initializing,
 or some drivers skip lator and return with 0 in this case.
 at least I could not find another drivers which returns with any ERROR.

 Thanks.

 -- 
 Yasushi Oshima

From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48964 CVS commit: src/sys/dev/usb
Date: Sat, 19 Jul 2014 08:38:28 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Sat Jul 19 08:38:28 UTC 2014

 Modified Files:
 	src/sys/dev/usb: if_urndis.c

 Log Message:
 Don't return an error in urndis_init if IFF_RUNNING is set on entry.

 That is, fix my mistake in handling PR/48964


 To generate a diff of this commit:
 cvs rdiff -u -r1.8 -r1.9 src/sys/dev/usb/if_urndis.c

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

From: Yasushi Oshima <oshima-ya@yagoto-urayama.jp>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/48964 CVS commit: src/sys/dev/usb
Date: Sat, 26 Jul 2014 00:49:51 +0900 (JST)

 >  Module Name:	src
 >  Committed By:	skrll
 >  Date:		Sat Jul 19 08:38:28 UTC 2014
 >  
 >  Modified Files:
 >  	src/sys/dev/usb: if_urndis.c
 >  
 >  Log Message:
 >  Don't return an error in urndis_init if IFF_RUNNING is set on entry.
 >  
 >  That is, fix my mistake in handling PR/48964
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.8 -r1.9 src/sys/dev/usb/if_urndis.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  

 It works well in my environment.
 please pull-up this to netbsd-6.

 Thanks.
 --
 Yasushi Oshima

From: Yasushi Oshima <oshima-ya@yagoto-urayama.jp>
To: gnats-bugs@gnats.netbsd.org
Cc: 
Subject: Re: kern/48964
Date: Sun, 05 Apr 2015 14:46:32 +0900 (JST)

 > kern/48964 - critical high priority sw-bug
 > 	In urndis(4), initialization of ifnet structure is not enough, cause panic.
 > 	http://gnats.NetBSD.org/cgi-bin/query-pr-single.pl?number=48964

 I found it has been already fixed in current, netbsd-6 include 6.1.5 
 and  netbsd-7, so please close it. 
 I sent a feedback previously but this state is still feedback yet...

State-Changed-From-To: feedback->closed
State-Changed-By: martin@NetBSD.org
State-Changed-When: Sun, 05 Apr 2015 07:10:50 +0000
State-Changed-Why:
Confirmed fixed


>Unformatted:

Home	PR Database Search