| Message ID | 20180920144904.24911-1-romain.izard.pro@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: cdc_acm: Do not leak URB buffers | expand |
On Do, 2018-09-20 at 16:49 +0200, Romain Izard wrote: > When the ACM TTY port is disconnected, the URBs it uses must be killed, and > then the buffers must be freed. Unfortunately a previous refactor removed > the code freeing the buffers because it looked extremely similar to the > code killing the URBs. > > As a result, there were many new leaks for each plug/unplug cycle of a > CDC-ACM device, that were detected by kmemleak. > > Restore the missing code, and the memory leak is removed. Try as i may, I don't see the difference. Could you put a comment exactly describing the issue into the code itself, lest this problem reappear? Regards Oliver
2018-09-21 15:27 +0200, Oliver Neukum <oneukum@suse.com>: > > On Do, 2018-09-20 at 16:49 +0200, Romain Izard wrote: > > When the ACM TTY port is disconnected, the URBs it uses must be killed, > > and then the buffers must be freed. Unfortunately a previous refactor > > removed the code freeing the buffers because it looked extremely similar > > to the code killing the URBs. > > > > As a result, there were many new leaks for each plug/unplug cycle of a > > CDC-ACM device, that were detected by kmemleak. > > > > Restore the missing code, and the memory leak is removed. > > Try as i may, I don't see the difference. Could you put a comment exactly > describing the issue into the code itself, lest this problem reappear? > The critical point is that on shutdown, the URBs need to be killed with usb_kill_urb, and then released with usb_free_urb. As the code for iterating on all allocated URBs and the parameters are the same for both functions, which also have the same length, the difference is visually subtle. But conceptually, it is not subtle at all. I believe that this does not need a comment. Best regards, -- Romain Izard
On Mo, 2018-09-24 at 10:20 +0200, Romain Izard wrote: > 2018-09-21 15:27 +0200, Oliver Neukum <oneukum@suse.com>: > > > > On Do, 2018-09-20 at 16:49 +0200, Romain Izard wrote: > > > When the ACM TTY port is disconnected, the URBs it uses must be killed, > > > and then the buffers must be freed. Unfortunately a previous refactor > > > removed the code freeing the buffers because it looked extremely similar > > > to the code killing the URBs. > > > > > > As a result, there were many new leaks for each plug/unplug cycle of a > > > CDC-ACM device, that were detected by kmemleak. > > > > > > Restore the missing code, and the memory leak is removed. > > > > Try as i may, I don't see the difference. Could you put a comment exactly > > describing the issue into the code itself, lest this problem reappear? > > > > The critical point is that on shutdown, the URBs need to be killed with > usb_kill_urb, and then released with usb_free_urb. > > As the code for iterating on all allocated URBs and the parameters are the > same for both functions, which also have the same length, the difference is > visually subtle. But conceptually, it is not subtle at all. > > I believe that this does not need a comment. Damn. I am blind. Regards Oliver
On Do, 2018-09-20 at 16:49 +0200, Romain Izard wrote: > When the ACM TTY port is disconnected, the URBs it uses must be killed, and > then the buffers must be freed. Unfortunately a previous refactor removed > the code freeing the buffers because it looked extremely similar to the > code killing the URBs. > > As a result, there were many new leaks for each plug/unplug cycle of a > CDC-ACM device, that were detected by kmemleak. > > Restore the missing code, and the memory leak is removed. > > Fixes: ba8c931ded8d ("cdc-acm: refactor killing urbs") > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> Acked-by: Oliver Neukum <oneukum@suse.com>
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index f9b40a9dc4d3..bc03b0a690b4 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -1514,6 +1514,7 @@ static void acm_disconnect(struct usb_interface *intf) { struct acm *acm = usb_get_intfdata(intf); struct tty_struct *tty; + int i; /* sibling interface is already cleaning up */ if (!acm) @@ -1544,6 +1545,11 @@ static void acm_disconnect(struct usb_interface *intf) tty_unregister_device(acm_tty_driver, acm->minor); + usb_free_urb(acm->ctrlurb); + for (i = 0; i < ACM_NW; i++) + usb_free_urb(acm->wb[i].urb); + for (i = 0; i < acm->rx_buflimit; i++) + usb_free_urb(acm->read_urbs[i]); acm_write_buffers_free(acm); usb_free_coherent(acm->dev, acm->ctrlsize, acm->ctrl_buffer, acm->ctrl_dma); acm_read_buffers_free(acm);
When the ACM TTY port is disconnected, the URBs it uses must be killed, and then the buffers must be freed. Unfortunately a previous refactor removed the code freeing the buffers because it looked extremely similar to the code killing the URBs. As a result, there were many new leaks for each plug/unplug cycle of a CDC-ACM device, that were detected by kmemleak. Restore the missing code, and the memory leak is removed. Fixes: ba8c931ded8d ("cdc-acm: refactor killing urbs") Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> --- drivers/usb/class/cdc-acm.c | 6 ++++++ 1 file changed, 6 insertions(+)