| Message ID | 20190811082259.48176-1-ikjn@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 9334367cda850fadc3b0ed2ff26b85e29226c1c4 |
| Headers | show |
| Series | xhci: fix memleak on setup address fails. | expand |
On 11.8.2019 11.22, Ikjoon Jang wrote: > Xhci re-enables a slot on transaction error in set_address using > xhci_disable_slot() + xhci_alloc_dev(). > > But in this case, xhci_alloc_dev() creates debugfs entries upon an > existing device without cleaning up old entries, thus memory leaks. > > So this patch simply moves calling xhci_debugfs_free_dev() from > xhci_free_dev() to xhci_disable_slot(). > Othwerwise this looks good, but xhci_alloc_dev() will call xhci_disable_slot() in some failure cases before the slot debugfs entry is created. In these cases xhci_debugfs_remove_slot() will be called without xhci_debugfs_create_slot() ever being called. This might not be an issue as xhci_debugfs_remove_slot() checks if (!dev || !dev->debugfs_private) before doing anything, but should be checked out. -Mathias
On Wed, Aug 14, 2019 at 9:57 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 11.8.2019 11.22, Ikjoon Jang wrote: > > Xhci re-enables a slot on transaction error in set_address using > > xhci_disable_slot() + xhci_alloc_dev(). > > > > But in this case, xhci_alloc_dev() creates debugfs entries upon an > > existing device without cleaning up old entries, thus memory leaks. > > > > So this patch simply moves calling xhci_debugfs_free_dev() from > > xhci_free_dev() to xhci_disable_slot(). > > > > Othwerwise this looks good, but xhci_alloc_dev() will call xhci_disable_slot() > in some failure cases before the slot debugfs entry is created. > > In these cases xhci_debugfs_remove_slot() will be called without > xhci_debugfs_create_slot() ever being called. > > This might not be an issue as xhci_debugfs_remove_slot() checks > if (!dev || !dev->debugfs_private) before doing anything, but should > be checked out. > I checked out the case by adding simple fault injection on xhci_alloc_dev(), to simulate xhci_debugfs_remove_slot() can be called without xhci_debugfs_create_slot() being called. Here is the test codes used in a test: --- drivers/usb/host/xhci-debugfs.c | 11 ++++++++++- drivers/usb/host/xhci.c | 4 ++++ drivers/usb/host/xhci.h | 3 +++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c index 7ba6afc7ef23..4dd3873856e7 100644 --- a/drivers/usb/host/xhci-debugfs.c +++ b/drivers/usb/host/xhci-debugfs.c @@ -13,6 +13,8 @@ #include "xhci.h" #include "xhci-debugfs.h" +static DECLARE_FAULT_ATTR(fail_default_attr); + static const struct debugfs_reg32 xhci_cap_regs[] = { dump_register(CAPLENGTH), dump_register(HCSPARAMS1), @@ -500,8 +502,10 @@ void xhci_debugfs_remove_slot(struct xhci_hcd *xhci, int slot_id) struct xhci_slot_priv *priv; struct xhci_virt_device *dev = xhci->devs[slot_id]; - if (!dev || !dev->debugfs_private) + if (!dev || !dev->debugfs_private) { + xhci_warn(xhci, "trying to remove a non-existent debugfs on slot %d.\n", slot_id); return; + } priv = dev->debugfs_private; @@ -585,6 +589,11 @@ void xhci_debugfs_init(struct xhci_hcd *xhci) xhci->debugfs_slots = debugfs_create_dir("devices", xhci->debugfs_root); xhci_debugfs_create_ports(xhci, xhci->debugfs_root); + + xhci->fail_alloc_dev = fail_default_attr; + + fault_create_debugfs_attr("fail_alloc_dev", xhci->debugfs_root, + &xhci->fail_alloc_dev); } void xhci_debugfs_exit(struct xhci_hcd *xhci) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 8c5cbd065edd..b01f2a2e7b91 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/dmi.h> #include <linux/dma-mapping.h> +#include <linux/fault-inject.h> #include "xhci.h" #include "xhci-trace.h" @@ -3880,6 +3881,9 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) xhci_free_command(xhci, command); + if (should_fail(&xhci->fail_alloc_dev, 1)) + goto disable_slot; + if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) { spin_lock_irqsave(&xhci->lock, flags); ret = xhci_reserve_host_control_ep_resources(xhci); diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 5dad11d223e0..2ab4d2b5e935 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -17,6 +17,7 @@ #include <linux/kernel.h> #include <linux/usb/hcd.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/fault-inject.h> /* Code sharing between pci-quirks and xhci hcd */ #include "xhci-ext-caps.h" @@ -1895,6 +1896,8 @@ struct xhci_hcd { struct dentry *debugfs_slots; struct list_head regset_list; + struct fault_attr fail_alloc_dev; + void *dbc; /* platform-specific data -- must come last */ unsigned long priv[0] __aligned(sizeof(s64));
On 26.8.2019 9.41, Ikjoon Jang wrote: > On Wed, Aug 14, 2019 at 9:57 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> >> On 11.8.2019 11.22, Ikjoon Jang wrote: >>> Xhci re-enables a slot on transaction error in set_address using >>> xhci_disable_slot() + xhci_alloc_dev(). >>> >>> But in this case, xhci_alloc_dev() creates debugfs entries upon an >>> existing device without cleaning up old entries, thus memory leaks. >>> >>> So this patch simply moves calling xhci_debugfs_free_dev() from >>> xhci_free_dev() to xhci_disable_slot(). >>> >> >> Othwerwise this looks good, but xhci_alloc_dev() will call xhci_disable_slot() >> in some failure cases before the slot debugfs entry is created. >> >> In these cases xhci_debugfs_remove_slot() will be called without >> xhci_debugfs_create_slot() ever being called. >> >> This might not be an issue as xhci_debugfs_remove_slot() checks >> if (!dev || !dev->debugfs_private) before doing anything, but should >> be checked out. >> > > I checked out the case by adding simple fault injection on xhci_alloc_dev(), > to simulate xhci_debugfs_remove_slot() can be called without > xhci_debugfs_create_slot() being called. > Thanks, patch sent forward -Mathias
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 03d1e552769b..c24c5bf9ef9c 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3814,7 +3814,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) virt_dev->eps[i].ep_state &= ~EP_STOP_CMD_PENDING; del_timer_sync(&virt_dev->eps[i].stop_cmd_timer); } - xhci_debugfs_remove_slot(xhci, udev->slot_id); virt_dev->udev = NULL; ret = xhci_disable_slot(xhci, udev->slot_id); if (ret) @@ -3832,6 +3831,8 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) if (!command) return -ENOMEM; + xhci_debugfs_remove_slot(xhci, slot_id); + spin_lock_irqsave(&xhci->lock, flags); /* Don't disable the slot if the host controller is dead. */ state = readl(&xhci->op_regs->status);
Xhci re-enables a slot on transaction error in set_address using xhci_disable_slot() + xhci_alloc_dev(). But in this case, xhci_alloc_dev() creates debugfs entries upon an existing device without cleaning up old entries, thus memory leaks. So this patch simply moves calling xhci_debugfs_free_dev() from xhci_free_dev() to xhci_disable_slot(). Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- drivers/usb/host/xhci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)