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");
+
-------------------------------------------------------
This SF.Net email is sponsored by the new InstallShield X.
>From Windows to Linux, servers to mobile, InstallShield X is the one
installation-authoring solution that does it all. Learn more and
evaluate today! http://www.installshield.com/Dev2Dev/0504
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
