netdev - Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <87d2u25s3o.fsf@nemi.mork.no>
Date:	Wed, 10 Apr 2013 17:21:31 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Dan Williams <dcbw@...hat.com>
Cc:	Ming Lei <tom.leiming@...il.com>,
	Oliver Neukum <oliver@...kum.org>,
	Elina Pasheva <epasheva@...rrawireless.com>,
	Network Development <netdev@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	Rory Filer <rfiler@...rrawireless.com>,
	Phil Sutter <phil@....cc>
Subject: Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active

Dan Williams <dcbw@...hat.com> writes:
> On Wed, 2013-04-10 at 15:25 +0200, Bjørn Mork wrote:
>> Dan Williams <dcbw@...hat.com> writes:
>> 
>> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
>> > +{
>> > +	/* Only drivers that implement a status hook should call this */
>> > +	BUG_ON(dev->interrupt == NULL);
>> > 
>> I still don't think there is any reason to BUG out. See for example
>> http://article.gmane.org/gmane.linux.kernel/52102
>> 
>> > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
>> > +{
>> > +	if (dev->interrupt) {
>> > +		mutex_lock(&dev->interrupt_mutex);
>> > +		if (!force)
>> > +			BUG_ON(dev->interrupt_count == 0);
>> 
>> Same here.  You can deal with this just fine.  Warn once, and go on
>> ignoring the problem.  Why kill the machine because of some minor driver
>> issue?
>
> Actually in the stop case, no, we can't deal with it, because then (due
> to the buggy sub-driver) we'd go on to decrement 0 into UINT_MAX.  It
> really is a bug if, *not* at suspend time, we're told to stop the
> interrupt URB when it's not yet submitted. 

Sure it is a bug.  All I'm saying is that you can deal with it.  Warn
about the bug and give up.  Or continue.  But don't roll over and die.
Let the user unload the buggy driver and email the author instead.

> I'm happy to add another
> if() here though, which would end up looking like this:
>
> if (dev->interrupt_count && --dev->interrupt_count == 0)
>     usb_kill_urb(dev->interrupt);
>
> which seems odd, but fine.

Yes, there are too many decision factors, so it does look odd.  You
could also decide early that the bogus dev->interrupt_count means that
nothing needs to be done, because no interrupt URB was subitted.  Like

static void __usbnet_status_stop(struct usbnet *dev, bool force)
{
	if (dev->interrupt) {
		mutex_lock(&dev->interrupt_mutex);
                if (!force && dev->interrupt_count == 0) {
                     print loud warning blaming the minidriver author;
                     goto out;
                }
		if (force || --dev->interrupt_count == 0)
			usb_kill_urb(dev->interrupt);

		dev_dbg(&dev->udev->dev,
			"decremented interrupt URB count to %d\n",
			dev->interrupt_count);
out:
		mutex_unlock(&dev->interrupt_mutex);
	}
}


But it is pretty much the same.  "force" makes a mess of it.

In any case, I believe any of those solutions are a lot nicer to any
unsuspecting user, who may not agree with you that a failing 3G modem is
important enough to kill the machine.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html