* [PATCH] xhci: fix memleak on setup address fails. @ 2019-08-11 8:22 Ikjoon Jang 2019-08-14 13:59 ` Mathias Nyman 0 siblings, 1 reply; 4+ messages in thread From: Ikjoon Jang @ 2019-08-11 8:22 UTC (permalink / raw) To: Mathias Nyman, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Ikjoon Jang 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(-) 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); -- 2.23.0.rc1.153.gdeed80330f-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xhci: fix memleak on setup address fails. 2019-08-11 8:22 [PATCH] xhci: fix memleak on setup address fails Ikjoon Jang @ 2019-08-14 13:59 ` Mathias Nyman 2019-08-26 6:41 ` Ikjoon Jang 0 siblings, 1 reply; 4+ messages in thread From: Mathias Nyman @ 2019-08-14 13:59 UTC (permalink / raw) To: Ikjoon Jang, Mathias Nyman, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xhci: fix memleak on setup address fails. 2019-08-14 13:59 ` Mathias Nyman @ 2019-08-26 6:41 ` Ikjoon Jang 2019-08-30 13:40 ` Mathias Nyman 0 siblings, 1 reply; 4+ messages in thread From: Ikjoon Jang @ 2019-08-26 6:41 UTC (permalink / raw) To: Mathias Nyman; +Cc: Mathias Nyman, Greg Kroah-Hartman, linux-usb, linux-kernel 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)); -- and here is the test result: [ 117.528523] FAULT_INJECTION: forcing a failure. [ 117.528523] name fail_alloc_dev, interval 1, probability 100, space 0, times 1 ... [ 117.600764] xxxx.xhci: trying to remove a non-existent debugfs on slot 0. [ 117.608943] usb 1-1.2-port2: couldn't allocate usb_device > -Mathias ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xhci: fix memleak on setup address fails. 2019-08-26 6:41 ` Ikjoon Jang @ 2019-08-30 13:40 ` Mathias Nyman 0 siblings, 0 replies; 4+ messages in thread From: Mathias Nyman @ 2019-08-30 13:40 UTC (permalink / raw) To: Ikjoon Jang; +Cc: Mathias Nyman, Greg Kroah-Hartman, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-30 13:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-11 8:22 [PATCH] xhci: fix memleak on setup address fails Ikjoon Jang 2019-08-14 13:59 ` Mathias Nyman 2019-08-26 6:41 ` Ikjoon Jang 2019-08-30 13:40 ` Mathias Nyman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).