Re: error handling in tty_open

Hi,
No, I don't see how it can work.

When a device is disconnected serial_open will
fail at the very beginning 
(usb_serial_get_by_index returns NULL in my patch
or lock_serial fails in your earlier patch).
In both cases bailout_module_put is bypassed
and 	++port->cleaning_up; is not executed.

But there may be a more serious problem.
port is stored in tty->driver_data and 
is shared among all files on the same device.

So, I wonder whether the following may happen:

proc A -> serial_open file 1 - success
proc B -> serial_open file 2 - failure 
  - set cleaning_up flag
proc A -> serial_close file 1 
  - aborted because of cleaning_up flag
proc B -> serial_close file 2
  - proceeded to close file 2 even if it was
  never opened.

The only way to differentiate between file 1
and 2 is by filp, but currently filp is
not used by usb-serial.c - it is simply passed
to 
serial->type->open(port, filp);
and
port->serial->type->close(port, filp);
and who knows how it may be used there?



By the way, did you receive my patch,
which I sent on Friday for review?
E-Mail title was called
"[PATCH 1/1] race condition fixes for usb-serial - new
attempt - for review"

Thank you
John

--- Oliver Neukum <[email protected]> wrote:

> Hi,
> 
> it turns out that tty_open() calls close() even if
> open() has failed.
> As this is undesireable for usb serial drivers,
> here's a patch
> to deal with it.
> 
> John, does it fix the issue you reported?
> 
> 	Regards
> 		Oliver
> -- 
> 
> --- a/include/linux/usb/serial.h	2007-03-26
> 09:23:20.000000000 +0200
> +++ b/include/linux/usb/serial.h	2007-03-26
> 09:27:12.000000000 +0200
> @@ -92,6 +92,7 @@
>  	int			open_count;
>  	char			throttled;
>  	char			throttle_req;
> +	int			cleaning_up;
>  	struct device		dev;
>  };
>  #define to_usb_serial_port(d) container_of(d,
> struct usb_serial_port, dev)
> --- a/drivers/usb/serial/usb-serial.c	2007-03-26
> 09:18:08.000000000 +0200
> +++ b/drivers/usb/serial/usb-serial.c	2007-03-26
> 09:28:14.000000000 +0200
> @@ -235,6 +235,7 @@
>  	return 0;
>  
>  bailout_module_put:
> +	++port->cleaning_up;
>  	module_put(serial->type->driver.owner);
>  bailout_mutex_unlock:
>  	port->open_count = 0;
> @@ -257,6 +258,12 @@
>  
>  	mutex_lock(&port->mutex);
>  
> +	if (port->cleaning_up) {
> +		--port->cleaning_up;
> +		mutex_unlock(&port->mutex);
> +		return;
> +	}
> +
>  	if (port->open_count == 0) {
>  		mutex_unlock(&port->mutex);
>  		return;
> 
> 



 
____________________________________________________________________________________
Never miss an email again!
Yahoo! Toolbar alerts you the instant new Mail arrives.
http://tools.search.yahoo.com/toolbar/features/mail/

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel