aboutsummaryrefslogtreecommitdiffstats
path: root/drivers/usb
diff options
context:
space:
mode:
authorAlan Stern <stern@rowland.harvard.edu>2009-04-14 11:31:02 -0400
committerGreg Kroah-Hartman <gregkh@suse.de>2009-04-23 17:15:26 -0400
commit2d93148ab6988cad872e65d694c95e8944e1b626 (patch)
tree1b2e406958336061cce7b262ccdc5752094a23fc /drivers/usb
parent091069740304c979f957ceacec39c461d0192158 (diff)
USB: serial: fix lifetime and locking problems
This patch (as1229) fixes a few lifetime and locking problems in the usb-serial driver. The main symptom is that an invalid kevent is created when the serial device is unplugged while a connection is active. Ports should be unregistered when device is disconnected, not when the parent usb_serial structure is deallocated. Each open file should hold a reference to the corresponding port structure, and the reference should be released when the file is closed. serial->disc_mutex should be acquired in serial_open(), to resolve the classic race between open and disconnect. serial_close() doesn't need to hold both serial->disc_mutex and port->mutex at the same time. Release the subdriver's module reference only after releasing all the other references, in case one of the release routines needs to invoke some code in the subdriver module. Replace a call to flush_scheduled_work() (which is prone to deadlocks) with cancel_work_sync(). Also, add a call to cancel_work_sync() in the disconnect routine. Reduce the scope of serial->disc_mutex in serial_disconnect(). The only place it really needs to protect is where the "disconnected" flag is set. This fixes the bug reported in http://bugs.freedesktop.org/show_bug.cgi?id=20703 Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Tested-by: Dan Williams <dcbw@redhat.com> Tested-by: Ming Lei <tom.leiming@gmail.com> Reviewed-by: Oliver Neukum <oliver@neukum.org> Acked-by: Alan Cox <alan@linux.intel.com> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Diffstat (limited to 'drivers/usb')
-rw-r--r--drivers/usb/serial/usb-serial.c104
1 files changed, 68 insertions, 36 deletions
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 2a70563bbee1..0a566eea49c0 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -137,22 +137,10 @@ static void destroy_serial(struct kref *kref)
137 137
138 dbg("%s - %s", __func__, serial->type->description); 138 dbg("%s - %s", __func__, serial->type->description);
139 139
140 serial->type->shutdown(serial);
141
142 /* return the minor range that this device had */ 140 /* return the minor range that this device had */
143 if (serial->minor != SERIAL_TTY_NO_MINOR) 141 if (serial->minor != SERIAL_TTY_NO_MINOR)
144 return_serial(serial); 142 return_serial(serial);
145 143
146 for (i = 0; i < serial->num_ports; ++i)
147 serial->port[i]->port.count = 0;
148
149 /* the ports are cleaned up and released in port_release() */
150 for (i = 0; i < serial->num_ports; ++i)
151 if (serial->port[i]->dev.parent != NULL) {
152 device_unregister(&serial->port[i]->dev);
153 serial->port[i] = NULL;
154 }
155
156 /* If this is a "fake" port, we have to clean it up here, as it will 144 /* If this is a "fake" port, we have to clean it up here, as it will
157 * not get cleaned up in port_release() as it was never registered with 145 * not get cleaned up in port_release() as it was never registered with
158 * the driver core */ 146 * the driver core */
@@ -187,7 +175,7 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
187 struct usb_serial *serial; 175 struct usb_serial *serial;
188 struct usb_serial_port *port; 176 struct usb_serial_port *port;
189 unsigned int portNumber; 177 unsigned int portNumber;
190 int retval; 178 int retval = 0;
191 179
192 dbg("%s", __func__); 180 dbg("%s", __func__);
193 181
@@ -198,21 +186,24 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
198 return -ENODEV; 186 return -ENODEV;
199 } 187 }
200 188
189 mutex_lock(&serial->disc_mutex);
201 portNumber = tty->index - serial->minor; 190 portNumber = tty->index - serial->minor;
202 port = serial->port[portNumber]; 191 port = serial->port[portNumber];
203 if (!port) { 192 if (!port || serial->disconnected)
204 retval = -ENODEV;
205 goto bailout_kref_put;
206 }
207
208 if (port->serial->disconnected) {
209 retval = -ENODEV; 193 retval = -ENODEV;
210 goto bailout_kref_put; 194 else
211 } 195 get_device(&port->dev);
196 /*
197 * Note: Our locking order requirement does not allow port->mutex
198 * to be acquired while serial->disc_mutex is held.
199 */
200 mutex_unlock(&serial->disc_mutex);
201 if (retval)
202 goto bailout_serial_put;
212 203
213 if (mutex_lock_interruptible(&port->mutex)) { 204 if (mutex_lock_interruptible(&port->mutex)) {
214 retval = -ERESTARTSYS; 205 retval = -ERESTARTSYS;
215 goto bailout_kref_put; 206 goto bailout_port_put;
216 } 207 }
217 208
218 ++port->port.count; 209 ++port->port.count;
@@ -232,14 +223,20 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
232 goto bailout_mutex_unlock; 223 goto bailout_mutex_unlock;
233 } 224 }
234 225
235 retval = usb_autopm_get_interface(serial->interface); 226 mutex_lock(&serial->disc_mutex);
227 if (serial->disconnected)
228 retval = -ENODEV;
229 else
230 retval = usb_autopm_get_interface(serial->interface);
236 if (retval) 231 if (retval)
237 goto bailout_module_put; 232 goto bailout_module_put;
233
238 /* only call the device specific open if this 234 /* only call the device specific open if this
239 * is the first time the port is opened */ 235 * is the first time the port is opened */
240 retval = serial->type->open(tty, port, filp); 236 retval = serial->type->open(tty, port, filp);
241 if (retval) 237 if (retval)
242 goto bailout_interface_put; 238 goto bailout_interface_put;
239 mutex_unlock(&serial->disc_mutex);
243 } 240 }
244 241
245 mutex_unlock(&port->mutex); 242 mutex_unlock(&port->mutex);
@@ -248,13 +245,16 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
248bailout_interface_put: 245bailout_interface_put:
249 usb_autopm_put_interface(serial->interface); 246 usb_autopm_put_interface(serial->interface);
250bailout_module_put: 247bailout_module_put:
248 mutex_unlock(&serial->disc_mutex);
251 module_put(serial->type->driver.owner); 249 module_put(serial->type->driver.owner);
252bailout_mutex_unlock: 250bailout_mutex_unlock:
253 port->port.count = 0; 251 port->port.count = 0;
254 tty->driver_data = NULL; 252 tty->driver_data = NULL;
255 tty_port_tty_set(&port->port, NULL); 253 tty_port_tty_set(&port->port, NULL);
256 mutex_unlock(&port->mutex); 254 mutex_unlock(&port->mutex);
257bailout_kref_put: 255bailout_port_put:
256 put_device(&port->dev);
257bailout_serial_put:
258 usb_serial_put(serial); 258 usb_serial_put(serial);
259 return retval; 259 return retval;
260} 260}
@@ -262,6 +262,9 @@ bailout_kref_put:
262static void serial_close(struct tty_struct *tty, struct file *filp) 262static void serial_close(struct tty_struct *tty, struct file *filp)
263{ 263{
264 struct usb_serial_port *port = tty->driver_data; 264 struct usb_serial_port *port = tty->driver_data;
265 struct usb_serial *serial;
266 struct module *owner;
267 int count;
265 268
266 if (!port) 269 if (!port)
267 return; 270 return;
@@ -269,6 +272,8 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
269 dbg("%s - port %d", __func__, port->number); 272 dbg("%s - port %d", __func__, port->number);
270 273
271 mutex_lock(&port->mutex); 274 mutex_lock(&port->mutex);
275 serial = port->serial;
276 owner = serial->type->driver.owner;
272 277
273 if (port->port.count == 0) { 278 if (port->port.count == 0) {
274 mutex_unlock(&port->mutex); 279 mutex_unlock(&port->mutex);
@@ -281,7 +286,7 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
281 * this before we drop the port count. The call is protected 286 * this before we drop the port count. The call is protected
282 * by the port mutex 287 * by the port mutex
283 */ 288 */
284 port->serial->type->close(tty, port, filp); 289 serial->type->close(tty, port, filp);
285 290
286 if (port->port.count == (port->console ? 2 : 1)) { 291 if (port->port.count == (port->console ? 2 : 1)) {
287 struct tty_struct *tty = tty_port_tty_get(&port->port); 292 struct tty_struct *tty = tty_port_tty_get(&port->port);
@@ -295,17 +300,23 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
295 } 300 }
296 } 301 }
297 302
298 if (port->port.count == 1) {
299 mutex_lock(&port->serial->disc_mutex);
300 if (!port->serial->disconnected)
301 usb_autopm_put_interface(port->serial->interface);
302 mutex_unlock(&port->serial->disc_mutex);
303 module_put(port->serial->type->driver.owner);
304 }
305 --port->port.count; 303 --port->port.count;
306 304 count = port->port.count;
307 mutex_unlock(&port->mutex); 305 mutex_unlock(&port->mutex);
308 usb_serial_put(port->serial); 306 put_device(&port->dev);
307
308 /* Mustn't dereference port any more */
309 if (count == 0) {
310 mutex_lock(&serial->disc_mutex);
311 if (!serial->disconnected)
312 usb_autopm_put_interface(serial->interface);
313 mutex_unlock(&serial->disc_mutex);
314 }
315 usb_serial_put(serial);
316
317 /* Mustn't dereference serial any more */
318 if (count == 0)
319 module_put(owner);
309} 320}
310 321
311static int serial_write(struct tty_struct *tty, const unsigned char *buf, 322static int serial_write(struct tty_struct *tty, const unsigned char *buf,
@@ -549,7 +560,13 @@ static void kill_traffic(struct usb_serial_port *port)
549 560
550static void port_free(struct usb_serial_port *port) 561static void port_free(struct usb_serial_port *port)
551{ 562{
563 /*
564 * Stop all the traffic before cancelling the work, so that
565 * nobody will restart it by calling usb_serial_port_softint.
566 */
552 kill_traffic(port); 567 kill_traffic(port);
568 cancel_work_sync(&port->work);
569
553 usb_free_urb(port->read_urb); 570 usb_free_urb(port->read_urb);
554 usb_free_urb(port->write_urb); 571 usb_free_urb(port->write_urb);
555 usb_free_urb(port->interrupt_in_urb); 572 usb_free_urb(port->interrupt_in_urb);
@@ -558,7 +575,6 @@ static void port_free(struct usb_serial_port *port)
558 kfree(port->bulk_out_buffer); 575 kfree(port->bulk_out_buffer);
559 kfree(port->interrupt_in_buffer); 576 kfree(port->interrupt_in_buffer);
560 kfree(port->interrupt_out_buffer); 577 kfree(port->interrupt_out_buffer);
561 flush_scheduled_work(); /* port->work */
562 kfree(port); 578 kfree(port);
563} 579}
564 580
@@ -1043,6 +1059,12 @@ void usb_serial_disconnect(struct usb_interface *interface)
1043 usb_set_intfdata(interface, NULL); 1059 usb_set_intfdata(interface, NULL);
1044 /* must set a flag, to signal subdrivers */ 1060 /* must set a flag, to signal subdrivers */
1045 serial->disconnected = 1; 1061 serial->disconnected = 1;
1062 mutex_unlock(&serial->disc_mutex);
1063
1064 /* Unfortunately, many of the sub-drivers expect the port structures
1065 * to exist when their shutdown method is called, so we have to go
1066 * through this awkward two-step unregistration procedure.
1067 */
1046 for (i = 0; i < serial->num_ports; ++i) { 1068 for (i = 0; i < serial->num_ports; ++i) {
1047 port = serial->port[i]; 1069 port = serial->port[i];
1048 if (port) { 1070 if (port) {
@@ -1052,11 +1074,21 @@ void usb_serial_disconnect(struct usb_interface *interface)
1052 tty_kref_put(tty); 1074 tty_kref_put(tty);
1053 } 1075 }
1054 kill_traffic(port); 1076 kill_traffic(port);
1077 cancel_work_sync(&port->work);
1078 device_del(&port->dev);
1079 }
1080 }
1081 serial->type->shutdown(serial);
1082 for (i = 0; i < serial->num_ports; ++i) {
1083 port = serial->port[i];
1084 if (port) {
1085 put_device(&port->dev);
1086 serial->port[i] = NULL;
1055 } 1087 }
1056 } 1088 }
1089
1057 /* let the last holder of this object 1090 /* let the last holder of this object
1058 * cause it to be cleaned up */ 1091 * cause it to be cleaned up */
1059 mutex_unlock(&serial->disc_mutex);
1060 usb_serial_put(serial); 1092 usb_serial_put(serial);
1061 dev_info(dev, "device disconnected\n"); 1093 dev_info(dev, "device disconnected\n");
1062} 1094}