NetBSD Problem Report #48243

From Wolfgang.Stukenbrock@nagler-company.com  Wed Sep 25 10:43:24 2013
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id F375972656
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 25 Sep 2013 10:43:23 +0000 (UTC)
Message-Id: <20130925104312.74F7E123B93@test-s0.nagler-company.com>
Date: Wed, 25 Sep 2013 12:43:12 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: inconsistant usage of 'up->parent' in usb_subr.c
X-Send-Pr-Version: 3.95

>Number:         48243
>Category:       kern
>Synopsis:       inconsistant usage of 'up->parent' in usb_subr.c
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 25 10:45:00 +0000 2013
>Closed-Date:    Wed Oct 26 07:53:12 +0000 2016
>Last-Modified:  Wed Oct 26 07:53:12 +0000 2016
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 6.1 / HEAD
>Organization:
Dr. Nagler & Company GmbH
>Environment:


System: NetBSD test-s0 5.1.2 NetBSD 5.1.2 (NSW-WS) #3: Fri Dec 21 15:15:43 CET 2012 wgstuken@test-s0:/usr/src/sys/arch/amd64/compile/NSW-WS amd64
Architecture: x86_64
Machine: amd64
>Description:
	In the function usbd_new_device() the port-reference is passed as "up" parameter.
	The component "parent" in this structure is used in an inconsistant way.

	1. it is passed to "myhub" in the dev-structure without any check (line 1117)
	2. it is used to call usbd_reset_port() without any check during initial description retrieval (line 1160)
	3. it is checked for not-NULL prior doing the "Windows-like" reset after
initial desc-retrival

	I'm not shure if the up->parent can ever be NULL.
	If the answer is NO, than the check at point 3 makes no sence and should be eliminated.
	If the answer is YES, than the check for NULL at point 2 is missing.
	remark: not shure if setting "myhup" to NULL will make sence.
>How-To-Repeat:
	Found by a look into the sources.
>Fix:
	Either add check for NULL prior calling usb_port_reset() or remove
	useless check for NULL, depending on the call semantics of usbd_new_device()

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: gnats-admin->kern-bug-people
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Sat, 28 Sep 2013 07:18:02 +0000
Responsible-Changed-Why:
rescue this from the pending queue (came in with mistyped category)


State-Changed-From-To: open->feedback
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Mon, 03 Oct 2016 06:50:49 +0000
State-Changed-Why:
The only time up->up_parent is non-NULL is for a root hub. Here the
usbd_get_initial_ddsec can (should?) never fail and therefore the
usbd_reset_port won't happen.

Is the code as-is really a problem?


From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: <gnats-bugs@NetBSD.org>
Cc: <skrll@NetBSD.org>, <kern-bug-people@netbsd.org>, <netbsd-bugs@netbsd.org>,
        <gnats-admin@netbsd.org>, <Wolfgang.Stukenbrock@nagler-company.com>
Subject: Re: kern/48243 (inconsistant usage of 'up->parent' in usb_subr.c)
Date: Fri, 14 Oct 2016 10:56:12 +0200

 Hi, sorry for the delay, but I was offline for a while.

 If a pointer can be NULL and not NULL then it always make sence to check th=
 is (at least in debug-builds) to catch
 any situation where the expected state is not the expected one.
 And if a call to a subroutine 'can (should?)' never fail, this should to be=
  checked to=F6.

 If you think the code is stable enought that it will never fail, you may ad=
 d checks only for debug-builds to validate this.
 This wouldn't harm performance in normal kernels.

 This is my understanding from a 'stable' source that allows searching for e=
 rrors if any happen.
 And perhaps it would be a good idea to add a short comment when the pointer=
  is NULL or not ..

 As stated in the initial report, I found this by analysing a problem I've w=
 ith something in the USB stack.
 It is along time ago and I don't remember the cause why I've a look at it a=
 nymore - sorry.
 As far as I remember, it was never the cause of a kernel crash - in that ca=
 se I haven't classified the report as non-critical/medium.

 On Mon, 3 Oct 2016 06:50:49 +0000
 <skrll@NetBSD.org> wrote:

 > Synopsis: inconsistant usage of 'up->parent' in usb_subr.c
 >=20
 > State-Changed-From-To: open->feedback
 > State-Changed-By: skrll@NetBSD.org
 > State-Changed-When: Mon, 03 Oct 2016 06:50:49 +0000
 > State-Changed-Why:
 > The only time up->up_parent is non-NULL is for a root hub. Here the
 > usbd_get_initial_ddsec can (should?) never fail and therefore the
 > usbd_reset_port won't happen.
 >=20
 > Is the code as-is really a problem?
 >=20
 >=20
 >=20


 --=20


 Dr. Nagler & Company GmbH
 Hauptstra_e 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Gesch_ftsf_hrer: Dr. Martin Nagler

From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48243 CVS commit: [nick-nhusb] src/sys/dev/usb
Date: Wed, 26 Oct 2016 07:31:24 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Wed Oct 26 07:31:24 UTC 2016

 Modified Files:
 	src/sys/dev/usb [nick-nhusb]: usb_subr.c

 Log Message:
 PR kern/48243 (inconsistant usage of 'up->parent' in usb_subr.c)

 Add a KASSERT and comment to explain what's going on.


 To generate a diff of this commit:
 cvs rdiff -u -r1.198.2.34 -r1.198.2.35 src/sys/dev/usb/usb_subr.c

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

State-Changed-From-To: feedback->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Wed, 26 Oct 2016 07:53:12 +0000
State-Changed-Why:
A KASSERT and comment have been added.


>Unformatted:

Home	PR Database Search