Note that I have not verified any of the below on
hardware associated with drivers/usb/serial/ftdi_sio.c,
only with drivers/usb/serial/visor.c. If anyone has
hardware for this device, I would appreciate your comments.
A memory leak occurs in both drivers/usb/serial/ftdi_sio.c
and drivers/usb/serial/visor.c when the usb device is
unplugged while data is being written to the device. This
patch should clear that up.
This was prepared against 2.6.7-rc2.
Signed-off-by: Joe Nardelli <[email protected]>
diff -uprN -X dontdiff linux-2.6.7-rc2.orig/drivers/usb/serial/ftdi_sio.c linux-2.6.7-rc2/drivers/usb/serial/ftdi_sio.c
--- linux-2.6.7-rc2.orig/drivers/usb/serial/ftdi_sio.c 2004-06-04 10:10:21.112743024 -0400
+++ linux-2.6.7-rc2/drivers/usb/serial/ftdi_sio.c 2004-06-04 09:53:27.000000000 -0400
@@ -1504,6 +1504,7 @@ static int ftdi_write (struct usb_serial
if (status) {
err("%s - failed submitting write urb, error %d", __FUNCTION__, status);
count = status;
+ kfree (buffer);
}
/* we are done with this urb, so let the host driver
diff -uprN -X dontdiff linux-2.6.7-rc2.orig/drivers/usb/serial/visor.c linux-2.6.7-rc2/drivers/usb/serial/visor.c
--- linux-2.6.7-rc2.orig/drivers/usb/serial/visor.c 2004-06-04 10:10:21.210728128 -0400
+++ linux-2.6.7-rc2/drivers/usb/serial/visor.c 2004-06-04 10:13:10.214035720 -0400
@@ -515,6 +515,7 @@ static int visor_write (struct usb_seria
dev_err(&port->dev, "%s - usb_submit_urb(write bulk) failed with status = %d\n",
__FUNCTION__, status);
count = status;
+ kfree (buffer);
} else {
bytes_out += count;
}
--
Joe Nardelli
[email protected]
On 04/06/2004 15:59, nardelli wrote:
> Note that I have not verified any of the below on
> hardware associated with drivers/usb/serial/ftdi_sio.c,
> only with drivers/usb/serial/visor.c. If anyone has
> hardware for this device, I would appreciate your comments.
>
> A memory leak occurs in both drivers/usb/serial/ftdi_sio.c
> and drivers/usb/serial/visor.c when the usb device is
> unplugged while data is being written to the device. This
> patch should clear that up.
The change to ftdi_sio.c looks correct to me.
I made the original change to ftdi_sio.c to allocate the write urbs
and their transfer buffers dynamically (instead of using a
preallocated pool) and I copied that technique from visor.c!
A related problem with the current implementation is that is easy to
run out of memory by running something similar to this:
# cat /dev/zero > /dev/ttyUSB0
That affects both the ftdi_sio and visor drivers.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
Ian Abbott wrote:
> On 04/06/2004 15:59, nardelli wrote:
>
>> Note that I have not verified any of the below on
>> hardware associated with drivers/usb/serial/ftdi_sio.c,
>> only with drivers/usb/serial/visor.c. If anyone has
>> hardware for this device, I would appreciate your comments.
>>
>> A memory leak occurs in both drivers/usb/serial/ftdi_sio.c
>> and drivers/usb/serial/visor.c when the usb device is
>> unplugged while data is being written to the device. This
>> patch should clear that up.
>
>
> The change to ftdi_sio.c looks correct to me.
>
> I made the original change to ftdi_sio.c to allocate the write urbs and
> their transfer buffers dynamically (instead of using a preallocated
> pool) and I copied that technique from visor.c!
>
> A related problem with the current implementation is that is easy to run
> out of memory by running something similar to this:
>
> # cat /dev/zero > /dev/ttyUSB0
>
> That affects both the ftdi_sio and visor drivers.
>
I believe that I have seen something similiar, but possibly not identical.
When writing alot (I used /dev/urandom instead of /dev/zero), it looks
like a very large percentage of buffers that are being allocated during
writes are not being freed. One test I did indicated that 95% of buffers
were not being freed! I briefly mention some info in
http://lkml.org/lkml/2004/5/25/72, but I wasn't going to go into detail
until I'd found out more info, and verified that my test procedure was
adequate.
My test is pretty simple, print out the address every time a buffer is
allocated, and print out the address every time a buffer is freed. In
this case there is only one location where it is being allocated, but 3
(4 with patch I submitted) where it is being freed. It's simply a matter
of running a command like the one below, and looking to see which addresses
are in there an odd number of times (i.e. allocated, but not freed). Not a
perfect test, but hopefully not embarrassingly bad either.
cat /var/log/messages | egrep 'Allocated|Freed' | tr -s ' ' | cut -d ' ' -f 7 | sort | uniq -c | sort -n
I'm not sure why such a high percentage would not be freed, but hopefully I'll
figure it out in the next week or two, time permitting.
--
Joe Nardelli
[email protected]
On Fri, 04 Jun 2004 17:34:41 +0100
Ian Abbott <[email protected]> wrote:
> I made the original change to ftdi_sio.c to allocate the write urbs
> and their transfer buffers dynamically (instead of using a
> preallocated pool) and I copied that technique from visor.c!
>
> A related problem with the current implementation is that is easy to
> run out of memory by running something similar to this:
>
> # cat /dev/zero > /dev/ttyUSB0
This begs the question why in the world you discarded the
perfectly good code and went into the trouble of programming
the dynamic allocation scheme (with the leak we just plugged).
-- Pete
On Fri, Jun 04, 2004 at 10:59:57AM -0400, nardelli wrote:
> Note that I have not verified any of the below on
> hardware associated with drivers/usb/serial/ftdi_sio.c,
> only with drivers/usb/serial/visor.c. If anyone has
> hardware for this device, I would appreciate your comments.
>
> A memory leak occurs in both drivers/usb/serial/ftdi_sio.c
> and drivers/usb/serial/visor.c when the usb device is
> unplugged while data is being written to the device. This
> patch should clear that up.
>
> This was prepared against 2.6.7-rc2.
This patch has all of the tabs stripped out of it and can not be applied
:(
Care to try it again?
thanks,
greg k-h
^*(&*(# editor not copying tabs - I should have caught that.
Here's another try:
This was prepared against 2.6.7-rc2.
Signed-off-by: Joe Nardelli <[email protected]>
diff -uprN -X dontdiff linux-2.6.7-rc2.orig/drivers/usb/serial/ftdi_sio.c linux-2.6.7-rc2/drivers/usb/serial/ftdi_sio.c
--- linux-2.6.7-rc2.orig/drivers/usb/serial/ftdi_sio.c 2004-06-04 10:10:21.112743024 -0400
+++ linux-2.6.7-rc2/drivers/usb/serial/ftdi_sio.c 2004-06-04 09:53:27.000000000 -0400
@@ -1504,6 +1504,7 @@ static int ftdi_write (struct usb_serial
if (status) {
err("%s - failed submitting write urb, error %d", __FUNCTION__, status);
count = status;
+ kfree (buffer);
}
/* we are done with this urb, so let the host driver
diff -uprN -X dontdiff linux-2.6.7-rc2.orig/drivers/usb/serial/visor.c linux-2.6.7-rc2/drivers/usb/serial/visor.c
--- linux-2.6.7-rc2.orig/drivers/usb/serial/visor.c 2004-06-04 10:10:21.210728128 -0400
+++ linux-2.6.7-rc2/drivers/usb/serial/visor.c 2004-06-04 10:13:10.214035720 -0400
@@ -515,6 +515,7 @@ static int visor_write (struct usb_seria
dev_err(&port->dev, "%s - usb_submit_urb(write bulk) failed with status = %d\n",
__FUNCTION__, status);
count = status;
+ kfree (buffer);
} else {
bytes_out += count;
}
Greg KH wrote:
>
>
> This patch has all of the tabs stripped out of it and can not be applied
> :(
>
> Care to try it again?
>
> thanks,
>
> greg k-h
>
--
Joe Nardelli
[email protected]
On Fri, Jun 04, 2004 at 02:47:51PM -0400, nardelli wrote:
> ^*(&*(# editor not copying tabs - I should have caught that.
> Here's another try:
>
>
> This was prepared against 2.6.7-rc2.
Heh, much better. Applied, thanks.
greg k-h
On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
>
> A related problem with the current implementation is that is easy to
> run out of memory by running something similar to this:
>
> # cat /dev/zero > /dev/ttyUSB0
>
I got bitten by this a couple of days ago. There is effectively no write
flow control at all, it just sucks up memory ...
P.
On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
> On 04/06/2004 15:59, nardelli wrote:
> >Note that I have not verified any of the below on
> >hardware associated with drivers/usb/serial/ftdi_sio.c,
> >only with drivers/usb/serial/visor.c. If anyone has
> >hardware for this device, I would appreciate your comments.
> >
> >A memory leak occurs in both drivers/usb/serial/ftdi_sio.c
> >and drivers/usb/serial/visor.c when the usb device is
> >unplugged while data is being written to the device. This
> >patch should clear that up.
>
> The change to ftdi_sio.c looks correct to me.
>
> I made the original change to ftdi_sio.c to allocate the write urbs
> and their transfer buffers dynamically (instead of using a
> preallocated pool) and I copied that technique from visor.c!
>
> A related problem with the current implementation is that is easy to
> run out of memory by running something similar to this:
>
> # cat /dev/zero > /dev/ttyUSB0
>
> That affects both the ftdi_sio and visor drivers.
Care to try out the following (build test only) patch to the visor
driver to see if it prevents this from happening? I don't have a
working visor right now to test it out myself :(
Oops, ignore the fact that we never free the structure on disconnect, I
see that now...
thanks,
greg k-h
===== drivers/usb/serial/visor.c 1.114 vs edited =====
--- 1.114/drivers/usb/serial/visor.c Fri Jun 4 07:13:10 2004
+++ edited/drivers/usb/serial/visor.c Fri Jun 4 17:12:53 2004
@@ -387,10 +387,17 @@
.read_bulk_callback = visor_read_bulk_callback,
};
+struct visor_private {
+ spinlock_t lock;
+ int bytes_in;
+ int bytes_out;
+ int outstanding_urbs;
+};
-static int bytes_in;
-static int bytes_out;
+/* number of outstanding urbs to prevent userspace DoS from happening */
+#define URB_UPPER_LIMIT 42
+static int stats;
/******************************************************************************
* Handspring Visor specific driver functions
@@ -398,6 +405,8 @@
static int visor_open (struct usb_serial_port *port, struct file *filp)
{
struct usb_serial *serial = port->serial;
+ struct visor_private *priv = usb_get_serial_port_data(port);
+ unsigned long flags;
int result = 0;
dbg("%s - port %d", __FUNCTION__, port->number);
@@ -408,8 +417,11 @@
return -ENODEV;
}
- bytes_in = 0;
- bytes_out = 0;
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->bytes_in = 0;
+ priv->bytes_out = 0;
+ priv->outstanding_urbs = 0;
+ spin_unlock_irqrestore(&priv->lock, flags);
/*
* Force low_latency on so that our tty_push actually forces the data
@@ -447,6 +459,7 @@
static void visor_close (struct usb_serial_port *port, struct file * filp)
{
+ struct visor_private *priv = usb_get_serial_port_data(port);
unsigned char *transfer_buffer;
dbg("%s - port %d", __FUNCTION__, port->number);
@@ -467,20 +480,32 @@
kfree (transfer_buffer);
}
- /* Uncomment the following line if you want to see some statistics in your syslog */
- /* dev_info (&port->dev, "Bytes In = %d Bytes Out = %d\n", bytes_in, bytes_out); */
+ if (stats)
+ dev_info(&port->dev, "Bytes In = %d Bytes Out = %d\n",
+ priv->bytes_in, priv->bytes_out);
}
static int visor_write (struct usb_serial_port *port, int from_user, const unsigned char *buf, int count)
{
+ struct visor_private *priv = usb_get_serial_port_data(port);
struct usb_serial *serial = port->serial;
struct urb *urb;
unsigned char *buffer;
+ unsigned long flags;
int status;
dbg("%s - port %d", __FUNCTION__, port->number);
+ spin_lock_irqsave(&priv->lock, flags);
+ if (priv->outstanding_urbs > URB_UPPER_LIMIT) {
+ spin_unlock_irqrestore(&priv->lock, flags);
+ dev_dbg(&port->dev, "write limit hit\n");
+ return 0;
+ }
+ ++priv->outstanding_urbs;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
buffer = kmalloc (count, GFP_ATOMIC);
if (!buffer) {
dev_err(&port->dev, "out of memory\n");
@@ -520,7 +545,10 @@
count = status;
kfree (buffer);
} else {
- bytes_out += count;
+ spin_lock_irqsave(&priv->lock, flags);
+ ++priv->outstanding_urbs;
+ priv->bytes_out += count;
+ spin_unlock_irqrestore(&priv->lock, flags);
}
/* we are done with this urb, so let the host driver
@@ -561,6 +589,8 @@
static void visor_write_bulk_callback (struct urb *urb, struct pt_regs *regs)
{
struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
+ struct visor_private *priv = usb_get_serial_port_data(port);
+ unsigned long flags;
/* free up the transfer buffer, as usb_free_urb() does not do this */
kfree (urb->transfer_buffer);
@@ -571,6 +601,10 @@
dbg("%s - nonzero write bulk status received: %d",
__FUNCTION__, urb->status);
+ spin_lock_irqsave(&priv->lock, flags);
+ --priv->outstanding_urbs;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
schedule_work(&port->work);
}
@@ -578,8 +612,10 @@
static void visor_read_bulk_callback (struct urb *urb, struct pt_regs *regs)
{
struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
- struct tty_struct *tty;
+ struct visor_private *priv = usb_get_serial_port_data(port);
unsigned char *data = urb->transfer_buffer;
+ struct tty_struct *tty;
+ unsigned long flags;
int i;
int result;
@@ -604,7 +640,9 @@
}
tty_flip_buffer_push(tty);
}
- bytes_in += urb->actual_length;
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->bytes_in += urb->actual_length;
+ spin_unlock_irqrestore(&priv->lock, flags);
/* Continue trying to always read */
usb_fill_bulk_urb (port->read_urb, port->serial->dev,
@@ -837,6 +875,22 @@
return num_ports;
}
+static int generic_startup(struct usb_serial *serial)
+{
+ struct visor_private *priv;
+ int i;
+
+ for (i = 0; i < serial->num_ports; ++i) {
+ priv = kmalloc (sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ memset (priv, 0x00, sizeof(*priv));
+ spin_lock_init(&priv->lock);
+ usb_set_serial_port_data(serial->port[i], priv);
+ }
+ return 0;
+}
+
static int clie_3_5_startup (struct usb_serial *serial)
{
struct device *dev = &serial->dev->dev;
@@ -876,7 +930,7 @@
return -EIO;
}
- return 0;
+ return generic_startup(serial);
}
static int treo_attach (struct usb_serial *serial)
@@ -915,7 +969,7 @@
COPY_PORT(serial->port[1], swap_port);
kfree(swap_port);
- return 0;
+ return generic_startup(serial);
}
static int clie_5_attach (struct usb_serial *serial)
@@ -936,7 +990,7 @@
/* port 0 now uses the modified endpoint Address */
serial->port[0]->bulk_out_endpointAddress = serial->port[1]->bulk_out_endpointAddress;
- return 0;
+ return generic_startup(serial);
}
static void visor_shutdown (struct usb_serial *serial)
@@ -1092,8 +1146,11 @@
module_param(debug, bool, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(debug, "Debug enabled or not");
+module_param(stats, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(stats, "Enables statistics or not");
module_param(vendor, ushort, 0);
MODULE_PARM_DESC(vendor, "User specified vendor ID");
module_param(product, ushort, 0);
MODULE_PARM_DESC(product, "User specified product ID");
+
On 05/06/2004 01:18, Greg KH wrote:
> On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
>>A related problem with the current implementation is that is easy to
>>run out of memory by running something similar to this:
>>
>> # cat /dev/zero > /dev/ttyUSB0
>>
>>That affects both the ftdi_sio and visor drivers.
>
>
> Care to try out the following (build test only) patch to the visor
> driver to see if it prevents this from happening? I don't have a
> working visor right now to test it out myself :(
I could try something similar on ftdi_sio, but am a bit pushed for
time right now.
One comment about the test patch in case it turns into a real patch:
I think it would be better to check the number of outstanding write
urbs in visor_write_room instead of visor_write, otherwise some
stuff written by the TTY line discipline will go missing.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
Greg KH wrote:
> On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
>
>>On 04/06/2004 15:59, nardelli wrote:
>>
>>A related problem with the current implementation is that is easy to
>>run out of memory by running something similar to this:
>>
>> # cat /dev/zero > /dev/ttyUSB0
>>
>>That affects both the ftdi_sio and visor drivers.
>
>
> Care to try out the following (build test only) patch to the visor
> driver to see if it prevents this from happening? I don't have a
> working visor right now to test it out myself :(
>
> Oops, ignore the fact that we never free the structure on disconnect, I
> see that now...
>
> thanks,
>
> greg k-h
>
>
> ===== drivers/usb/serial/visor.c 1.114 vs edited =====
> --- 1.114/drivers/usb/serial/visor.c Fri Jun 4 07:13:10 2004
> +++ edited/drivers/usb/serial/visor.c Fri Jun 4 17:12:53 2004
...
Just curious - is there something special about 42? Grepping wasn't
very useful, as numbers like this are scattered all over the place.
> +/* number of outstanding urbs to prevent userspace DoS from happening */
> +#define URB_UPPER_LIMIT 42
...
>
> static int visor_write (struct usb_serial_port *port, int from_user, const unsigned char *buf, int count)
> {
> + struct visor_private *priv = usb_get_serial_port_data(port);
> struct usb_serial *serial = port->serial;
> struct urb *urb;
> unsigned char *buffer;
> + unsigned long flags;
> int status;
>
> dbg("%s - port %d", __FUNCTION__, port->number);
>
> + spin_lock_irqsave(&priv->lock, flags);
> + if (priv->outstanding_urbs > URB_UPPER_LIMIT) {
> + spin_unlock_irqrestore(&priv->lock, flags);
> + dev_dbg(&port->dev, "write limit hit\n");
> + return 0;
> + }
> + ++priv->outstanding_urbs;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> buffer = kmalloc (count, GFP_ATOMIC);
> if (!buffer) {
> dev_err(&port->dev, "out of memory\n");
> @@ -520,7 +545,10 @@
> count = status;
> kfree (buffer);
> } else {
> - bytes_out += count;
> + spin_lock_irqsave(&priv->lock, flags);
> + ++priv->outstanding_urbs;
> + priv->bytes_out += count;
> + spin_unlock_irqrestore(&priv->lock, flags);
> }
>
> /* we are done with this urb, so let the host driver
Removing the first of two priv->outstanding_urbs increments in
visor_write (I assume that was your intention) produced very nice
results ;-)
1) When being flooded, after the initial bunch of URBs were sent,
only 1 per second was sent, and it appeared that all of them were
being freed.
2) Even after the flood, the driver survived, and backups were
possible after the device was reconnected.
I'm fairly ignorant of most of the lower level usb infrastructure
(hcd, hub, ehci, etc), so I'm not sure what the root cause (ignoring
the patch above) of the completion handler not being called might be.
I would suspect overflow of some callback list, but that's just a
guess. Do you have any ideas on what this might be, and could this
be a problem in other devices?
--
Joe Nardelli
[email protected]
On Mon, 7 Jun 2004, nardelli wrote:
> Greg KH wrote:
> > On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
> >
> >>On 04/06/2004 15:59, nardelli wrote:
[SNIPPED...]
> >
> >
> > ===== drivers/usb/serial/visor.c 1.114 vs edited =====
> > --- 1.114/drivers/usb/serial/visor.c Fri Jun 4 07:13:10 2004
> > +++ edited/drivers/usb/serial/visor.c Fri Jun 4 17:12:53 2004
>
> ...
>
> Just curious - is there something special about 42? Grepping wasn't
> very useful, as numbers like this are scattered all over the place.
>
> > +/* number of outstanding urbs to prevent userspace DoS from happening */
> > +#define URB_UPPER_LIMIT 42
See Hitchiker's Guide to the Galaxy. ;^
Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
Note 96.31% of all statistics are fiction.
Richard B. Johnson wrote:
> On Mon, 7 Jun 2004, nardelli wrote:
>
>
>>Greg KH wrote:
>>
>>>On Fri, Jun 04, 2004 at 05:34:41PM +0100, Ian Abbott wrote:
>>>
>>>
>>>>On 04/06/2004 15:59, nardelli wrote:
>
> [SNIPPED...]
>
>
>>>
>>>===== drivers/usb/serial/visor.c 1.114 vs edited =====
>>>--- 1.114/drivers/usb/serial/visor.c Fri Jun 4 07:13:10 2004
>>>+++ edited/drivers/usb/serial/visor.c Fri Jun 4 17:12:53 2004
>>
>>...
>>
>>Just curious - is there something special about 42? Grepping wasn't
>>very useful, as numbers like this are scattered all over the place.
>>
>>
>>>+/* number of outstanding urbs to prevent userspace DoS from happening */
>>>+#define URB_UPPER_LIMIT 42
>
>
> See Hitchiker's Guide to the Galaxy. ;^
>
LOL - I knew I shouldn't have asked :-)
--
Joe Nardelli
[email protected]