NetBSD Problem Report #47538

From campbell@mumble.net  Wed Feb  6 21:04:44 2013
Return-Path: <campbell@mumble.net>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id E2A2863E705
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  6 Feb 2013 21:04:43 +0000 (UTC)
Message-Id: <20130206210409.E9480604E3@jupiter.mumble.net>
Date: Wed,  6 Feb 2013 21:04:09 +0000 (UTC)
From: Taylor R Campbell <campbell+netbsd@mumble.net>
Reply-To: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@gnats.NetBSD.org
Subject: races in uatp(4)
X-Send-Pr-Version: 3.95

>Number:         47538
>Category:       kern
>Synopsis:       races in uatp(4)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Feb 06 21:05:00 +0000 2013
>Last-Modified:  Fri Feb 08 13:28:12 +0000 2013
>Originator:     Taylor R Campbell <campbell+netbsd@mumble.net>
>Release:        NetBSD-current
>Organization:
>Environment:
>Description:

	uatp_detach does nothing to ensure the callout is stopped
	before destroying it or to ensure that uatp_intr will not run
	before clobbering the softc structure.  sc_status, sc_input,
	sc_input_index, and sc_input_size are not protected by any
	mutex.  Also, geyser34_enable_raw_mode has a window between the
	UR_GET_REPORT and UR_SET_REPORT requests with nothing to
	prevent intervening updates to the report.  It's not clear that
	this is a problem, but it makes me nervous.

	The hardware should perhaps be reset when enabling the uhidev,
	not just when attaching the device -- that might help to fix
	the interrupt storms I have seen when starting X which the
	heuristic in uatp_intr doesn't detect.

>How-To-Repeat:

	Code inspection.

>Fix:

	Coming!

	I think that it should suffice to

	(a) extend sc_tap_mutex to cover all the mutable fields (and
	rename it to sc_lock), and

	(b) add some logic to uatp_detach to wait for the callout to
	stop running.

	In particular, I believe calling uhidev_close is enough to
	block further uatp_intr, so it is not necessary to add any
	logic to uatp_intr to check for sc_status & ENABLED.

	Eliminating the window in geyser34_enable_raw_mode might be
	best done with an additional flag in sc_status, say RESETTING,
	and an associated condvar:

static int
uatp_enter(struct uatp_softc *sc)
{
	int error;

	mutex_enter(&sc->sc_lock);
	while (ISSET(sc->sc_status, UATP_RESETTING)) {
		error = cv_timedwait_sig(&sc->sc_cv, &sc->sc_lock,
		    mstohz(1000));
		if (error)
			return error;
	}

	return 0;
}

static void
uatp_exit(struct uatp_softc *sc)
{

	mutex_exit(&sc->sc_lock);
}

static void
geyser34_enable_raw_mode(struct uatp_softc *sc)
{

	if (uatp_enter(sc) != 0)
		return;
	sc->sc_status |= UATP_RESETTING;
	uatp_exit(sc);

	...do the USB requests...

out:	mutex_enter(&sc->sc_lock);
	sc->sc_status &=~ UATP_RESETTING;
	cv_broadcast(&sc->sc_cv);
	mutex_exit(&sc->sc_lock);
}

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Fri, 08 Feb 2013 13:28:12 +0000
Responsible-Changed-Why:
mine


>Unformatted:

Home	PR Database Search