[PATCH 2/2] - usbserial: race-condition fix.

[PATCH 2/2] - usbserial: race-condition fix.

[Luiz Fernando Capitulino]

 Fixes open()/close() race-condition in the access of the port structure.

 When the race happens, the port in use becomes invalid, and the user must try
to get the next free port (repluging the device, for example).

 Described in more detail in this thread:

http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2

Signed-off-by: Luiz Capitulino <lcapitulino@mandriva.com.br>

 drivers/usb/serial/usb-serial.c |   14 +++++++++++++-
 drivers/usb/serial/usb-serial.h |    2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.c a~/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	2005-11-22 10:50:33.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.c	2005-11-22 11:31:46.000000000 -0200
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/smp_lock.h>
 #include <asm/uaccess.h>
+#include <asm/semaphore.h>
 #include <linux/usb.h>
 #include "usb-serial.h"
 #include "pl2303.h"
@@ -190,6 +191,9 @@ static int serial_open (struct tty_struc
 	port = serial->port[portNumber];
 	if (!port)
 		return -ENODEV;
+
+	if (down_interruptible(&port->sem))
+		return -ERESTARTSYS;
 	 
 	++port->open_count;
 
@@ -215,6 +219,7 @@ static int serial_open (struct tty_struc
 			goto bailout_module_put;
 	}
 
+	up(&port->sem);
 	return 0;
 
 bailout_module_put:
@@ -222,6 +227,7 @@ bailout_module_put:
 bailout_kref_put:
 	kref_put(&serial->kref, destroy_serial);
 	port->open_count = 0;
+	up(&port->sem);
 	return retval;
 }
 
@@ -234,8 +240,10 @@ static void serial_close(struct tty_stru
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
+	down(&port->sem);
+
 	if (port->open_count == 0)
-		return;
+		goto out;
 
 	--port->open_count;
 	if (port->open_count == 0) {
@@ -253,6 +261,9 @@ static void serial_close(struct tty_stru
 	}
 
 	kref_put(&port->serial->kref, destroy_serial);
+
+out:
+	up(&port->sem);
 }
 
 static int serial_write (struct tty_struct * tty, const unsigned char *buf, int count)
@@ -774,6 +785,7 @@ int usb_serial_probe(struct usb_interfac
 		port->number = i + serial->minor;
 		port->serial = serial;
 		spin_lock_init(&port->lock);
+		sema_init(&port->sem, 1);
 		INIT_WORK(&port->work, usb_serial_port_softint, port);
 		serial->port[i] = port;
 	}
diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.h a~/drivers/usb/serial/usb-serial.h
--- a/drivers/usb/serial/usb-serial.h	2005-11-22 10:50:42.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.h	2005-11-22 11:31:46.000000000 -0200
@@ -16,6 +16,7 @@
 
 #include <linux/config.h>
 #include <linux/kref.h>
+#include <asm/semaphore.h>
 
 #define SERIAL_TTY_MAJOR	188	/* Nice legal number now */
 #define SERIAL_TTY_MINORS	255	/* loads of devices :) */
@@ -60,6 +61,7 @@ struct usb_serial_port {
 	struct usb_serial *	serial;
 	struct tty_struct *	tty;
 	spinlock_t		lock;
+	struct semaphore        sem;
 	unsigned char		number;
 
 	unsigned char *		interrupt_in_buffer;


-- 
Luiz Fernando N. Capitulino

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Greg KH]

On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
> @@ -60,6 +61,7 @@ struct usb_serial_port {
>  	struct usb_serial *	serial;
>  	struct tty_struct *	tty;
>  	spinlock_t		lock;
> +	struct semaphore        sem;

You forgot to document what this semaphore is used for.

Hm, can we just use the spinlock already present in the port structure
for this?  Well, drop the spinlock and use the semaphore?  Yeah, that
means grabbing a semaphore for ever write for some devices, but USB data
rates are slow enough it wouldn't matter :)

thanks,

greg k-h

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Luiz Fernando Capitulino]

On Tue, 22 Nov 2005 14:13:53 -0800
Greg KH <gregkh@suse.de> wrote:

| On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
| > @@ -60,6 +61,7 @@ struct usb_serial_port {
| >  	struct usb_serial *	serial;
| >  	struct tty_struct *	tty;
| >  	spinlock_t		lock;
| > +	struct semaphore        sem;
| 
| You forgot to document what this semaphore is used for.

 Okay.

| Hm, can we just use the spinlock already present in the port structure
| for this?  Well, drop the spinlock and use the semaphore?  Yeah, that
| means grabbing a semaphore for ever write for some devices, but USB data
| rates are slow enough it wouldn't matter :)

 As far as I read the code, I found that spinlock is only used by the
generic driver, in the
drivers/usb/serial/generic.c:usb_serial_generic_write() function.

 Can we drop the spinlock there and use our new semaphore? Or should we
create a new spinlock just to use there?

 I ask it because the semaphore will be used to serialize open()/close()
operations in the usb-serial driver, is a bit weird to use the same
semaphore in a write() function of other driver.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Eduardo Pereira Habkost]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Wed, Nov 23, 2005 at 09:36:55AM -0200, Luiz Fernando Capitulino wrote:
> On Tue, 22 Nov 2005 14:13:53 -0800
> Greg KH <gregkh@suse.de> wrote:
> 
> | On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
> | > @@ -60,6 +61,7 @@ struct usb_serial_port {
> | >  	struct usb_serial *	serial;
> | >  	struct tty_struct *	tty;
> | >  	spinlock_t		lock;
> | > +	struct semaphore        sem;
> | 
> | You forgot to document what this semaphore is used for.
> 
>  Okay.
> 
> | Hm, can we just use the spinlock already present in the port structure
> | for this?  Well, drop the spinlock and use the semaphore?  Yeah, that
> | means grabbing a semaphore for ever write for some devices, but USB data
> | rates are slow enough it wouldn't matter :)
> 
>  As far as I read the code, I found that spinlock is only used by the
> generic driver, in the
> drivers/usb/serial/generic.c:usb_serial_generic_write() function.
> 
>  Can we drop the spinlock there and use our new semaphore? Or should we
> create a new spinlock just to use there?

The spin_lock is used only to protect write_urb_busy. An atomic_t seem
to be more appropriate for it. If we do that, I guess we can remove the
(then unused) spinlock.

So we have three proposed changes:

- Add semaphore to serialize close()/open() (properly documented)
- Replace write_urb_busy with an atomic_t
- Remove the spinlock

> 
>  I ask it because the semaphore will be used to serialize open()/close()
> operations in the usb-serial driver, is a bit weird to use the same
> semaphore in a write() function of other driver.

I agree.

-- 
Eduardo

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Luiz Fernando Capitulino]

On Wed, 23 Nov 2005 09:56:33 -0200
Eduardo Pereira Habkost <ehabkost@mandriva.com> wrote:

| On Wed, Nov 23, 2005 at 09:36:55AM -0200, Luiz Fernando Capitulino wrote:
| > On Tue, 22 Nov 2005 14:13:53 -0800
| > Greg KH <gregkh@suse.de> wrote:
| > 
| > | On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
| > | > @@ -60,6 +61,7 @@ struct usb_serial_port {
| > | >  	struct usb_serial *	serial;
| > | >  	struct tty_struct *	tty;
| > | >  	spinlock_t		lock;
| > | > +	struct semaphore        sem;
| > | 
| > | You forgot to document what this semaphore is used for.
| > 
| >  Okay.
| > 
| > | Hm, can we just use the spinlock already present in the port structure
| > | for this?  Well, drop the spinlock and use the semaphore?  Yeah, that
| > | means grabbing a semaphore for ever write for some devices, but USB data
| > | rates are slow enough it wouldn't matter :)
| > 
| >  As far as I read the code, I found that spinlock is only used by the
| > generic driver, in the
| > drivers/usb/serial/generic.c:usb_serial_generic_write() function.
| > 
| >  Can we drop the spinlock there and use our new semaphore? Or should we
| > create a new spinlock just to use there?
| 
| The spin_lock is used only to protect write_urb_busy. An atomic_t seem
| to be more appropriate for it. If we do that, I guess we can remove the
| (then unused) spinlock.

 Ok, but, hmm.. I found the spinklock is actually used by other drivers too.

 I didn't catch this before because I wasn't compiling then. Sorry, my fault.

| So we have three proposed changes:
| 
| - Add semaphore to serialize close()/open() (properly documented)
| - Replace write_urb_busy with an atomic_t
| - Remove the spinlock

 Since the spinlock seems to be only used to protect 'write_urb_busy', I agree
with those changes.

 Greg, do you? If so, I suggested we should add the semaphore first, because
it is a bug fix.

 I can do the 'write_urb_busy' type replace next week (yes, I will replace all
the drivers).

-- 
Luiz Fernando N. Capitulino

[RESEND 2/2] - usbserial: race-condition fix.

[Luiz Fernando Capitulino]

On Tue, 22 Nov 2005 14:13:53 -0800
Greg KH <gregkh@suse.de> wrote:

| On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
| > @@ -60,6 +61,7 @@ struct usb_serial_port {
| >  	struct usb_serial *	serial;
| >  	struct tty_struct *	tty;
| >  	spinlock_t		lock;
| > +	struct semaphore        sem;
| 
| You forgot to document what this semaphore is used for.

 Here goes the second patch again, with the documentation now.

 As I said before, would be good to apply these two patches now, and I
can cleanup the spinlock usage until next week.


 Fixes a race-condition in the access of the port structure, described
in detail at: http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2

Signed-off-by: Luiz Capitulino <lcapitulino@mandriva.com.br>

 drivers/usb/serial/usb-serial.c |   14 +++++++++++++-
 drivers/usb/serial/usb-serial.h |    4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.c a~/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	2005-11-23 15:30:20.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.c	2005-11-23 15:23:19.000000000 -0200
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/smp_lock.h>
 #include <asm/uaccess.h>
+#include <asm/semaphore.h>
 #include <linux/usb.h>
 #include "usb-serial.h"
 #include "pl2303.h"
@@ -190,6 +191,9 @@ static int serial_open (struct tty_struc
 	port = serial->port[portNumber];
 	if (!port)
 		return -ENODEV;
+
+	if (down_interruptible(&port->sem))
+		return -ERESTARTSYS;
 	 
 	++port->open_count;
 
@@ -215,6 +219,7 @@ static int serial_open (struct tty_struc
 			goto bailout_module_put;
 	}
 
+	up(&port->sem);
 	return 0;
 
 bailout_module_put:
@@ -222,6 +227,7 @@ bailout_module_put:
 bailout_kref_put:
 	kref_put(&serial->kref, destroy_serial);
 	port->open_count = 0;
+	up(&port->sem);
 	return retval;
 }
 
@@ -234,8 +240,10 @@ static void serial_close(struct tty_stru
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
+	down(&port->sem);
+
 	if (port->open_count == 0)
-		return;
+		goto out;
 
 	--port->open_count;
 	if (port->open_count == 0) {
@@ -253,6 +261,9 @@ static void serial_close(struct tty_stru
 	}
 
 	kref_put(&port->serial->kref, destroy_serial);
+
+out:
+	up(&port->sem);
 }
 
 static int serial_write (struct tty_struct * tty, const unsigned char *buf, int count)
@@ -774,6 +785,7 @@ int usb_serial_probe(struct usb_interfac
 		port->number = i + serial->minor;
 		port->serial = serial;
 		spin_lock_init(&port->lock);
+		sema_init(&port->sem, 1);
 		INIT_WORK(&port->work, usb_serial_port_softint, port);
 		serial->port[i] = port;
 	}
diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.h a~/drivers/usb/serial/usb-serial.h
--- a/drivers/usb/serial/usb-serial.h	2005-11-23 15:30:20.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.h	2005-11-23 15:23:59.000000000 -0200
@@ -16,6 +16,7 @@
 
 #include <linux/config.h>
 #include <linux/kref.h>
+#include <asm/semaphore.h>
 
 #define SERIAL_TTY_MAJOR	188	/* Nice legal number now */
 #define SERIAL_TTY_MINORS	255	/* loads of devices :) */
@@ -30,6 +31,8 @@
  * @serial: pointer back to the struct usb_serial owner of this port.
  * @tty: pointer to the corresponding tty for this port.
  * @lock: spinlock to grab when updating portions of this structure.
+ * @sem: semaphore used to synchronize serial_open() and serial_close()
+ *	access for this port.
  * @number: the number of the port (the minor number).
  * @interrupt_in_buffer: pointer to the interrupt in buffer for this port.
  * @interrupt_in_urb: pointer to the interrupt in struct urb for this port.
@@ -60,6 +63,7 @@ struct usb_serial_port {
 	struct usb_serial *	serial;
 	struct tty_struct *	tty;
 	spinlock_t		lock;
+	struct semaphore        sem;
 	unsigned char		number;
 
 	unsigned char *		interrupt_in_buffer;


-- 
Luiz Fernando N. Capitulino

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Greg KH]

On Wed, Nov 23, 2005 at 09:36:55AM -0200, Luiz Fernando Capitulino wrote:
> On Tue, 22 Nov 2005 14:13:53 -0800
> Greg KH <gregkh@suse.de> wrote:
> 
> | On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
> | > @@ -60,6 +61,7 @@ struct usb_serial_port {
> | >  	struct usb_serial *	serial;
> | >  	struct tty_struct *	tty;
> | >  	spinlock_t		lock;
> | > +	struct semaphore        sem;
> | 
> | You forgot to document what this semaphore is used for.
> 
>  Okay.
> 
> | Hm, can we just use the spinlock already present in the port structure
> | for this?  Well, drop the spinlock and use the semaphore?  Yeah, that
> | means grabbing a semaphore for ever write for some devices, but USB data
> | rates are slow enough it wouldn't matter :)
> 
>  As far as I read the code, I found that spinlock is only used by the
> generic driver, in the
> drivers/usb/serial/generic.c:usb_serial_generic_write() function.

No, lots of other usb-serial drivers use it.  Increase your grep a bit
wider :)

>  Can we drop the spinlock there and use our new semaphore? Or should we
> create a new spinlock just to use there?

Create a new one for where?

>  I ask it because the semaphore will be used to serialize open()/close()
> operations in the usb-serial driver, is a bit weird to use the same
> semaphore in a write() function of other driver.

I agree, but yet-another-lock isn't the best either.

thanks,

greg k-h

Re: [RESEND 2/2] - usbserial: race-condition fix.

[Greg KH]

On Wed, Nov 23, 2005 at 03:46:50PM -0200, Luiz Fernando Capitulino wrote:
> On Tue, 22 Nov 2005 14:13:53 -0800
> Greg KH <gregkh@suse.de> wrote:
> 
> | On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
> | > @@ -60,6 +61,7 @@ struct usb_serial_port {
> | >  	struct usb_serial *	serial;
> | >  	struct tty_struct *	tty;
> | >  	spinlock_t		lock;
> | > +	struct semaphore        sem;
> | 
> | You forgot to document what this semaphore is used for.
> 
>  Here goes the second patch again, with the documentation now.
> 
>  As I said before, would be good to apply these two patches now, and I
> can cleanup the spinlock usage until next week.

How will you clean it up?

>  Fixes a race-condition in the access of the port structure, described
> in detail at: http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2

Please put the full detail in the email, so I can put it in the
ChangeLog so people have an easy place to get the proper information.

Care to redo it again?

thanks,

greg k-h

Re: [RESEND 2/2] - usbserial: race-condition fix.

[Luiz Fernando Capitulino]

On Wed, 23 Nov 2005 10:01:08 -0800
Greg KH <gregkh@suse.de> wrote:

| On Wed, Nov 23, 2005 at 03:46:50PM -0200, Luiz Fernando Capitulino wrote:
| > On Tue, 22 Nov 2005 14:13:53 -0800
| > Greg KH <gregkh@suse.de> wrote:
| > 
| > | On Tue, Nov 22, 2005 at 07:59:26PM -0200, Luiz Fernando Capitulino wrote:
| > | > @@ -60,6 +61,7 @@ struct usb_serial_port {
| > | >  	struct usb_serial *	serial;
| > | >  	struct tty_struct *	tty;
| > | >  	spinlock_t		lock;
| > | > +	struct semaphore        sem;
| > | 
| > | You forgot to document what this semaphore is used for.
| > 
| >  Here goes the second patch again, with the documentation now.
| > 
| >  As I said before, would be good to apply these two patches now, and I
| > can cleanup the spinlock usage until next week.
| 
| How will you clean it up?

 Replacing the 'write_urb_busy' member type to atomic_t (and using the
atomic functions, of course), since the current spinlock seems to be only
used to have atomic access to 'write_urb_busy'.

 Thanks to Eduardo for noting this.

| >  Fixes a race-condition in the access of the port structure, described
| > in detail at: http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
| 
| Please put the full detail in the email, so I can put it in the
| ChangeLog so people have an easy place to get the proper information.
| 
| Care to redo it again?

 No. But hope the CC'd people doesn't too. :)

 Here goes:


 There is a race-condition in usb-serial driver that can be triggered if
a processes does 'port->tty->driver_data = NULL' in serial_close() while
other processes is in kernel-space about to call serial_ioctl() on the
same port.

 This happens because a process can open the device while there is
another one closing it.

 The patch below fixes that by adding a semaphore to ensure that no
process will open the device while another process is closing it.

 Note that we can't use spinlocks here, since serial_open() and
serial_close() can sleep.


Signed-off-by: Luiz Capitulino <lcapitulino@mandriva.com.br>

 drivers/usb/serial/usb-serial.c |   14 +++++++++++++-
 drivers/usb/serial/usb-serial.h |    4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.c a~/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	2005-11-23 15:30:20.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.c	2005-11-23 15:23:19.000000000 -0200
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/smp_lock.h>
 #include <asm/uaccess.h>
+#include <asm/semaphore.h>
 #include <linux/usb.h>
 #include "usb-serial.h"
 #include "pl2303.h"
@@ -190,6 +191,9 @@ static int serial_open (struct tty_struc
 	port = serial->port[portNumber];
 	if (!port)
 		return -ENODEV;
+
+	if (down_interruptible(&port->sem))
+		return -ERESTARTSYS;
 	 
 	++port->open_count;
 
@@ -215,6 +219,7 @@ static int serial_open (struct tty_struc
 			goto bailout_module_put;
 	}
 
+	up(&port->sem);
 	return 0;
 
 bailout_module_put:
@@ -222,6 +227,7 @@ bailout_module_put:
 bailout_kref_put:
 	kref_put(&serial->kref, destroy_serial);
 	port->open_count = 0;
+	up(&port->sem);
 	return retval;
 }
 
@@ -234,8 +240,10 @@ static void serial_close(struct tty_stru
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
+	down(&port->sem);
+
 	if (port->open_count == 0)
-		return;
+		goto out;
 
 	--port->open_count;
 	if (port->open_count == 0) {
@@ -253,6 +261,9 @@ static void serial_close(struct tty_stru
 	}
 
 	kref_put(&port->serial->kref, destroy_serial);
+
+out:
+	up(&port->sem);
 }
 
 static int serial_write (struct tty_struct * tty, const unsigned char *buf, int count)
@@ -774,6 +785,7 @@ int usb_serial_probe(struct usb_interfac
 		port->number = i + serial->minor;
 		port->serial = serial;
 		spin_lock_init(&port->lock);
+		sema_init(&port->sem, 1);
 		INIT_WORK(&port->work, usb_serial_port_softint, port);
 		serial->port[i] = port;
 	}
diff -Nparu -X /home/lcapitulino/kernels/dontdiff a/drivers/usb/serial/usb-serial.h a~/drivers/usb/serial/usb-serial.h
--- a/drivers/usb/serial/usb-serial.h	2005-11-23 15:30:20.000000000 -0200
+++ a~/drivers/usb/serial/usb-serial.h	2005-11-23 15:23:59.000000000 -0200
@@ -16,6 +16,7 @@
 
 #include <linux/config.h>
 #include <linux/kref.h>
+#include <asm/semaphore.h>
 
 #define SERIAL_TTY_MAJOR	188	/* Nice legal number now */
 #define SERIAL_TTY_MINORS	255	/* loads of devices :) */
@@ -30,6 +31,8 @@
  * @serial: pointer back to the struct usb_serial owner of this port.
  * @tty: pointer to the corresponding tty for this port.
  * @lock: spinlock to grab when updating portions of this structure.
+ * @sem: semaphore used to synchronize serial_open() and serial_close()
+ *	access for this port.
  * @number: the number of the port (the minor number).
  * @interrupt_in_buffer: pointer to the interrupt in buffer for this port.
  * @interrupt_in_urb: pointer to the interrupt in struct urb for this port.
@@ -60,6 +63,7 @@ struct usb_serial_port {
 	struct usb_serial *	serial;
 	struct tty_struct *	tty;
 	spinlock_t		lock;
+	struct semaphore        sem;
 	unsigned char		number;
 
 	unsigned char *		interrupt_in_buffer;



-- 
Luiz Fernando N. Capitulino

Re: [PATCH 2/2] - usbserial: race-condition fix.

[Greg KH]

On Wed, Nov 23, 2005 at 10:07:08AM -0200, Luiz Fernando Capitulino wrote:
>  Since the spinlock seems to be only used to protect 'write_urb_busy', I agree
> with those changes.
> 
>  Greg, do you? If so, I suggested we should add the semaphore first, because
> it is a bug fix.

Yes, I agree.

>  I can do the 'write_urb_busy' type replace next week (yes, I will replace all
> the drivers).

Ok, that sounds fine.

thanks,

greg k-h