On Fri, Jun 14, 2024 at 02:11:52PM +0200, Greg Kroah-Hartman wrote:
> The usb driver structures contain a dynamic id structure that is written
> to, preventing them from being able to be constant structures. To help
> resolve this, move the dynamic id structure out of the driver and into a
> separate local list, indexed off of the driver * so that all USB
> subsystems can use it (i.e. usb-serial).
>
> Overall it moves some duplicated code out of the usb-serial core as it's
> already in the usb core, and now the usb dynamic id structures can be
> private entirely to the usb core itself.
I'm concerned about the locking of the usb_dynid and usb_dynids
structures. There doesn't seem to be anything to prevent these things
from being deallocated while another thread is reading them.
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 3f69b32222f3..8bba102de39f 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -34,17 +34,76 @@
>
> #include "usb.h"
>
> +static struct list_head usb_dynids;
> +static spinlock_t usb_dynids_lock;
Is there any particular reason you decided to make this a spinlock
instead of a mutex?
Why not use static initialization for these things...
> +
> +struct usb_dynids {
> + struct list_head node;
> + const struct device_driver *driver;
> + struct list_head list;
> +};
> +
> +struct usb_dynid {
> + struct list_head node;
> + struct usb_device_id id;
> +};
> +
> +void usb_dynids_init(void)
> +{
> + spin_lock_init(&usb_dynids_lock);
> + INIT_LIST_HEAD(&usb_dynids);
> +}
... instead of this dynamic initialization? Not that it's a big deal.
> +static struct usb_dynids *usb_find_dynids(const struct device_driver *driver)
> +{
> + struct usb_dynids *u;
> +
> + /* Loop through the list to find if this driver has an id list already */
> + guard(spinlock)(&usb_dynids_lock);
> + list_for_each_entry(u, &usb_dynids, node) {
> + if (u->driver == driver)
> + return u;
> + }
So here, for instance. usb_dynids_lock is held while this routine
iterates through the usb_dynids list. But when an entry is found, the
lock is released. What's to prevent another thread from deallocating
right now the structure that u points to?
For instance, do we know that this code will always run under the
protection of some mutex associated with the driver? Not as far as I
can see.
> + return NULL;
> +}
> +
> +static int store_id(const struct device_driver *driver, const struct usb_device_id *id)
> +{
> + struct usb_dynids *u;
> + struct usb_dynid *usb_dynid;
> +
> + u = usb_find_dynids(driver);
> + if (!u) {
> + /* This driver has not stored any ids yet, so make a new entry for it */
> + u = kmalloc(sizeof(*u), GFP_KERNEL);
> + if (!u)
> + return -ENOMEM;
> + u->driver = driver;
> + INIT_LIST_HEAD(&u->list);
> + guard(spinlock)(&usb_dynids_lock);
> + list_add_tail(&u->node, &usb_dynids);
> + }
> +
> + /* Allocate a new entry and add it to the list of driver ids for this driver */
> + usb_dynid = kmalloc(sizeof(*usb_dynid), GFP_KERNEL);
> + if (!usb_dynid)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&usb_dynid->node);
> + memcpy(&usb_dynid->id, id, sizeof(*id));
> + list_add_tail(&usb_dynid->node, &u->list);
Here we see that the spinlock is _not_ held while a new usb_dynid
entry is added to the driver's list. (Yes, the existing code already
has the same problem; it's not something you added.)
> -ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf)
> +ssize_t usb_show_dynids(const struct device_driver *driver, char *buf)
> {
> + struct usb_dynids *dynids;
> struct usb_dynid *dynid;
> size_t count = 0;
>
> + dynids = usb_find_dynids(driver);
> + if (!dynids)
> + return 0;
> +
> list_for_each_entry(dynid, &dynids->list, node)
> if (dynid->id.bInterfaceClass != 0)
> count += scnprintf(&buf[count], PAGE_SIZE - count, "%04x %04x %02x\n",
And here nothing holds the spinlock while we iterate through the list.
> @@ -160,8 +216,12 @@ static ssize_t remove_id_store(struct device_driver *driver, const char *buf,
> if (fields < 2)
> return -EINVAL;
>
> + dynids = usb_find_dynids(driver);
> + if (!dynids)
> + return count;
> +
> guard(spinlock)(&usb_dynids_lock);
> - list_for_each_entry_safe(dynid, n, &usb_driver->dynids.list, node) {
> + list_for_each_entry_safe(dynid, n, &dynids->list, node) {
> struct usb_device_id *id = &dynid->id;
>
> if ((id->idVendor == idVendor) &&
Although here the spinlock is held while an entry is removed. But
that doesn't do any good if readers don't also acquire the spinlock.
Overall, I think it would be better to hold the spinlock throughout the
entire time that the dynamic ids are being accessed: Grab it before
starting the outer search and don't release it until the desired entry
has been found, added, or removed.
> @@ -1100,8 +1173,8 @@ void usb_deregister(struct usb_driver *driver)
> usbcore_name, driver->name);
>
> usb_remove_newid_files(driver);
> + usb_free_dynids(&driver->driver);
> driver_unregister(&driver->driver);
> - usb_free_dynids(driver);
Here's another potential problem. You moved the usb_free_dynids()
call from after driver_unregister() to before it. This means that the
driver is still visible when usb_free_dynids() runs, so another thread
might be iterating through the dynid list while the list is removed.
In fact, that other thread might go ahead and add a new usb_dynids
structure right after the function call here removes the old one!
Alan Stern
