[PATCH] xhci: fix memleak on setup address fails.

[PATCH] xhci: fix memleak on setup address fails.

[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

Re: [PATCH] xhci: fix memleak on setup address fails.

[Mathias Nyman]

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

Re: [PATCH] xhci: fix memleak on setup address fails.

[Ikjoon Jang]

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

Re: [PATCH] xhci: fix memleak on setup address fails.

[Mathias Nyman]

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