[PATCH] tty: Fix close races in USB serial

[PATCH] tty: Fix close races in USB serial

[Alan Cox]

From: Alan Cox <alan@redhat.com>

USB serial has always had races where the tty port usage count can hit zero
during a receive event. The internal locking is a mutex so we can't use
that in the IRQ handlers.

With krefs we can tackle this differently but we still need to be careful.

Signed-off-by: Alan Cox <alan@redhat.com>
---

 drivers/usb/serial/usb-serial.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)


diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 794b5ff..aafa684 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -269,15 +269,19 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
 		return;
 	}
 
-	--port->port.count;
-	if (port->port.count == 0)
+	if (port->port.count == 1)
 		/* only call the device specific close if this
-		 * port is being closed by the last owner */
+		 * port is being closed by the last owner. Ensure we do
+		 * this before we drop the port count. The call is protected
+		 * by the port mutex
+		 */
 		port->serial->type->close(tty, port, filp);
 
-	if (port->port.count == (port->console? 1 : 0)) {
+	if (port->port.count == (port->console ? 2 : 1)) {
 		struct tty_struct *tty = tty_port_tty_get(&port->port);
 		if (tty) {
+			/* We must do this before we drop the port count to
+			   zero. */
 			if (tty->driver_data)
 				tty->driver_data = NULL;
 			tty_port_tty_set(&port->port, NULL);
@@ -285,13 +289,14 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
 		}
 	}
 
-	if (port->port.count == 0) {
+	if (port->port.count == 1) {
 		mutex_lock(&port->serial->disc_mutex);
 		if (!port->serial->disconnected)
 			usb_autopm_put_interface(port->serial->interface);
 		mutex_unlock(&port->serial->disc_mutex);
 		module_put(port->serial->type->driver.owner);
 	}
+	--port->port.count;
 
 	mutex_unlock(&port->mutex);
 	usb_serial_put(port->serial);

Re: [PATCH] tty: Fix close races in USB serial

[Greg KH]

On Tue, Nov 04, 2008 at 03:29:24PM +0000, Alan Cox wrote:
> From: Alan Cox <alan@redhat.com>
> 
> USB serial has always had races where the tty port usage count can hit zero
> during a receive event. The internal locking is a mutex so we can't use
> that in the IRQ handlers.
> 
> With krefs we can tackle this differently but we still need to be careful.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> ---
> 
>  drivers/usb/serial/usb-serial.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index 794b5ff..aafa684 100644
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -269,15 +269,19 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
>  		return;
>  	}
>  
> -	--port->port.count;
> -	if (port->port.count == 0)
> +	if (port->port.count == 1)
>  		/* only call the device specific close if this
> -		 * port is being closed by the last owner */
> +		 * port is being closed by the last owner. Ensure we do
> +		 * this before we drop the port count. The call is protected
> +		 * by the port mutex
> +		 */
>  		port->serial->type->close(tty, port, filp);
>  
> -	if (port->port.count == (port->console? 1 : 0)) {
> +	if (port->port.count == (port->console ? 2 : 1)) {

Why are you testing for 2 here?

confused,

greg k-h

Re: [PATCH] tty: Fix close races in USB serial

[Alan Cox]

> > -	if (port->port.count == (port->console? 1 : 0)) {
> > +	if (port->port.count == (port->console ? 2 : 1)) {
> 
> Why are you testing for 2 here?

At the point we enter this routine port->port.count is the number of
users of the port. If this is the final close then this is 1 (us), except
if it is the USB console (in which case there will be two references).

The old code dropped the reference count first, the new code drops the
kref and port->tty first, then shuts down the port and then drops the
refcount. That avoids the race where we get the usb serial drivers trying
to stuff bytes into a closed port and the ldisc layer in turn trying to
echo them
- which causes the warnings.

Alan

Re: [PATCH] tty: Fix close races in USB serial

[Greg KH]

On Tue, Nov 04, 2008 at 05:37:26PM +0000, Alan Cox wrote:
> > > -	if (port->port.count == (port->console? 1 : 0)) {
> > > +	if (port->port.count == (port->console ? 2 : 1)) {
> > 
> > Why are you testing for 2 here?
> 
> At the point we enter this routine port->port.count is the number of
> users of the port. If this is the final close then this is 1 (us), except
> if it is the USB console (in which case there will be two references).
> 
> The old code dropped the reference count first, the new code drops the
> kref and port->tty first, then shuts down the port and then drops the
> refcount. That avoids the race where we get the usb serial drivers trying
> to stuff bytes into a closed port and the ldisc layer in turn trying to
> echo them
> - which causes the warnings.

Ah, ok, that makes sense, thanks.

You might want to document that in there so that others understand it as
well.

thanks,

greg k-h