| 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
| ||
|
Message-ID: <1262692881.23577.15.camel@wall-e>
Date: Tue, 05 Jan 2010 13:01:21 +0100
From: Stefani Seibold <stefani@...bold.net>
To: Johan Hovold <jhovold@...il.com>
Cc: Pete Zaitcev <zaitcev@...hat.com>, Greg KH <greg@...ah.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: USB: serial: kfifo_len locking
Am Dienstag, den 05.01.2010, 12:35 +0100 schrieb Johan Hovold:
> > > > > Which does not make any sense at all. Bad merge? What do you say Greg?
> > > >
> > > > I don't know where is your problem? This are two different functions
> > > > usb_serial_generic_write_room() and usb_serial_generic_chars_in_buffer()
> > >
> > > Exactly my point.
> > >
> > > The drain patch needed to modify chars_in_buffer, but the patch in Greg's
> > > tree modifies write_room instead (which does not make sense and was
> > > neither part of the submitted patch).
> > >
> > Sorry, but i am not sure if i the right address about your complains.
> > The only thing i have done in the usb serial driver is the port to the
> > new kfifo API. This is the original patch i had posted:
>
> The drain patch merge was a side-track and you were CC:d as you
> were part of the original thread.
>
> You did however remove the locking on kfifo_len that the original author
> had put there with the exact patch you're quoting:
>
> > diff -u -N -r -p old/drivers/usb/serial/generic.c new/drivers/usb/serial/generic.c
> > --- old/drivers/usb/serial/generic.c 2009-12-23 08:54:06.966476248 +0100
> > +++ new/drivers/usb/serial/generic.c 2009-12-23 09:06:25.778474708 +0100
> > @@ -276,7 +276,7 @@ static int usb_serial_generic_write_star
> > if (port->write_urb_busy)
> > start_io = false;
> > else {
> > - start_io = (kfifo_len(port->write_fifo) != 0);
> > + start_io = (kfifo_len(&port->write_fifo) != 0);
> > port->write_urb_busy = start_io;
> > }
> > spin_unlock_irqrestore(&port->lock, flags);
> > @@ -285,7 +285,7 @@ static int usb_serial_generic_write_star
> > return 0;
> >
> > data = port->write_urb->transfer_buffer;
> > - count = kfifo_out_locked(port->write_fifo, data, port->bulk_out_size, &port->lock);
> > + count = kfifo_out_locked(&port->write_fifo, data, port->bulk_out_size, &port->lock);
> > usb_serial_debug_data(debug, &port->dev, __func__, count, data);
> >
> > /* set up our urb */
> > @@ -345,7 +345,7 @@ int usb_serial_generic_write(struct tty_
> > return usb_serial_multi_urb_write(tty, port,
> > buf, count);
> >
> > - count = kfifo_in_locked(port->write_fifo, buf, count, &port->lock);
> > + count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> > result = usb_serial_generic_write_start(port);
> >
> > if (result >= 0)
> > @@ -370,7 +370,7 @@ int usb_serial_generic_write_room(struct
> > (serial->type->max_in_flight_urbs -
> > port->urbs_in_flight);
> > } else if (serial->num_bulk_out)
> > - room = port->write_fifo->size - kfifo_len(port->write_fifo);
> > + room = kfifo_avail(&port->write_fifo);
> > spin_unlock_irqrestore(&port->lock, flags);
> >
> > dbg("%s - returns %d", __func__, room);
> > @@ -391,7 +391,7 @@ int usb_serial_generic_chars_in_buffer(s
> > chars = port->tx_bytes_flight;
> > spin_unlock_irqrestore(&port->lock, flags);
> > } else if (serial->num_bulk_out)
> > - chars = kfifo_len(port->write_fifo);
> > + chars = kfifo_len(&port->write_fifo);
>
> Here's the change. The fifo used to be protected by a lock, but is no
> longer.
>
I posted yesterday a patch to this thread. It would be great if you read
and check this patch before complaining again!!!!
> Never say you did.
>
Sorry, i had no real idea what is your problem, if this is not what you
want. As i mentioned i posted to you yesterday a fix for the possible
kfifo_len() bug, but i didn't get a response if this is fixing your
problem. Again the patch:
diff -u -N -r -p linux-2.6.33-rc2.orig/drivers/usb/serial/generic.c linux-2.6.33-rc2.new/drivers/usb/serial/generic.c
--- linux-2.6.33-rc2.orig/drivers/usb/serial/generic.c 2009-12-27 23:37:03.566060210 +0100
+++ linux-2.6.33-rc2.new/drivers/usb/serial/generic.c 2010-01-04 20:15:38.023351711 +0100
@@ -386,12 +386,12 @@ int usb_serial_generic_chars_in_buffer(s
dbg("%s - port %d", __func__, port->number);
- if (serial->type->max_in_flight_urbs) {
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ if (serial->type->max_in_flight_urbs)
chars = port->tx_bytes_flight;
- spin_unlock_irqrestore(&port->lock, flags);
- } else if (serial->num_bulk_out)
+ else if (serial->num_bulk_out)
chars = kfifo_len(&port->write_fifo);
+ spin_unlock_irqrestore(&port->lock, flags);
dbg("%s - returns %d", __func__, chars);
return chars;
This patch should solve the possible race (if there is one). With this
patch all kfifo_... access are locked by the port->lock spinlock. If
this is what you want i will posted it as a bug fix to andrew.
Stefani
--
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/
Powered by blists - more mailing lists