linux-kernel - Re: 2.6.31-rc5 regression: Oops when USB Serial disconnected while in use

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.0908212214300.31042-100000@netrider.rowland.org>
Date:	Fri, 21 Aug 2009 22:30:23 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
cc:	Bruno Prémont <bonbons@...ux-vserver.org>,
	Greg KH <greg@...ah.com>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: 2.6.31-rc5 regression: Oops when USB Serial disconnected while
 in use

On Sat, 22 Aug 2009, Alan Cox wrote:

> > What about protecting the use counter?  In tty_port.c it's always
> > protected by port->lock, but not in serial_open().  Is that a mistake?
> 
> Ah good an easy question to begin with
> 
> Yes it is in error.

Okay, I'll fix it.

> The core of both hangup and open are still BKL protected against
> one another (ugly - wants fixing), release_one_dev() liekwise. This is
> probably inadequate as they may well sleep in various spots

Would you consider ideas for changing the protection to something else?  
I don't have anything in mind at the moment -- I need to study the code
some more to understand it better.  But eventually a possibility may
suggest itself.

> > > 		clean up resources
> > > 		if (last && test_clear INITIALIZED)
> > 
> > How do you check for "last"?  Doesn't the fact that we are here mean 
> > that there are no remaining open references?
> 
> It means there are no remaining file references to the handle, but you
> may have multiple file handles referencing the same tty

So basically this amounts to testing whether port->count == 0?  But 
isn't that already implicit when tty_port_close_start() returns 0?

It sounds like this is another little thingy needed only by drivers
that don't use the new helpers.

> > P.S.: Consider this code in tty_port_block_til_ready():
> > 
> > 	/* if non-blocking mode is set we can pass directly to open unless
> > 	   the port has just hung up or is in another error state */
> > 	if ((filp->f_flags & O_NONBLOCK) ||
> > 			(tty->flags & (1 << TTY_IO_ERROR))) {
> > 		port->flags |= ASYNC_NORMAL_ACTIVE;
> > 		return 0;
> > 	}
> > 
> > The comment doesn't agree with the logic of the test.  Which is wrong?
> 
> The code and comment were copied from the original drivers (and occur in
> several places ;))
> 
> The intended logic is
> 
> 	if O_NONBLOCK is set
> 		succeed immediately
> 	if there is a hangup (or other pending error)
> 		succeed immediately

Got it -- the comment is wrong.  It should say something like:

	/* If non-blocking mode is set or the port is in an error state
	 * then we can return directly; tty_open() will handle everything.
	 */

Thanks,

Alan Stern



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/