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 <[email protected]>
---
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
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
<[email protected]> 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
On 26.8.2019 9.41, Ikjoon Jang wrote:
> On Wed, Aug 14, 2019 at 9:57 PM Mathias Nyman
> <[email protected]> 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