| Message ID | 20190330112418.15042-12-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: logitech: Handling of non DJ receivers in hid-logitech-dj | expand |
Hi all, So I just found out the hard way that this commit has a small bug, which when using one of the new supported receiver types may result in a locked machine, see inline comment below. On 30-03-19 12:24, Hans de Goede wrote: > dj/HID++ receivers are really a single logical entity, but for BIOS/Windows > compatibility they have multiple USB interfaces. For the upcoming > non-unifying receiver support, we need to listen for events from / bind to > all USB-interfaces of the receiver. > > This commit add support to the logitech-dj code for creating a single > dj_receiver_dev struct for all interfaces belonging to a single > USB-device / receiver, in preparation for adding non-unifying receiver > support. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/hid-logitech-dj.c | 175 ++++++++++++++++++++++++++++------ > 1 file changed, 148 insertions(+), 27 deletions(-) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 9e08b44c76b2..337389c2d6c7 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -119,11 +119,16 @@ struct hidpp_event { > } __packed; > > struct dj_receiver_dev { > + struct hid_device *mouse; > + struct hid_device *keyboard; > struct hid_device *hidpp; > struct dj_device *paired_dj_devices[DJ_MAX_PAIRED_DEVICES + > DJ_DEVICE_INDEX_MIN]; > + struct list_head list; > + struct kref kref; > struct work_struct work; > struct kfifo notif_fifo; > + bool ready; > spinlock_t lock; > }; > > @@ -363,6 +368,110 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = { > static struct hid_ll_driver logi_dj_ll_driver; > > static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev); > +static void delayedwork_callback(struct work_struct *work); > + > +static LIST_HEAD(dj_hdev_list); > +static DEFINE_MUTEX(dj_hdev_list_lock); > + > +/* > + * dj/HID++ receivers are really a single logical entity, but for BIOS/Windows > + * compatibility they have multiple USB interfaces. On HID++ receivers we need > + * to listen for input reports on both interfaces. The functions below are used > + * to create a single struct dj_receiver_dev for all interfaces belonging to > + * a single USB-device / receiver. > + */ > +static struct dj_receiver_dev *dj_find_receiver_dev(struct hid_device *hdev) > +{ > + struct dj_receiver_dev *djrcv_dev; > + > + /* Try to find an already-probed interface from the same device */ > + list_for_each_entry(djrcv_dev, &dj_hdev_list, list) { > + if (djrcv_dev->mouse && > + hid_compare_device_paths(hdev, djrcv_dev->mouse, '/')) { > + kref_get(&djrcv_dev->kref); > + return djrcv_dev; > + } > + if (djrcv_dev->keyboard && > + hid_compare_device_paths(hdev, djrcv_dev->keyboard, '/')) { > + kref_get(&djrcv_dev->kref); > + return djrcv_dev; > + } > + if (djrcv_dev->hidpp && > + hid_compare_device_paths(hdev, djrcv_dev->hidpp, '/')) { > + kref_get(&djrcv_dev->kref); > + return djrcv_dev; > + } > + } > + > + return NULL; > +} > + > +static void dj_release_receiver_dev(struct kref *kref) > +{ > + struct dj_receiver_dev *djrcv_dev = container_of(kref, struct dj_receiver_dev, kref); > + > + list_del(&djrcv_dev->list); > + kfifo_free(&djrcv_dev->notif_fifo); > + kfree(djrcv_dev); > +} > + > +static void dj_put_receiver_dev(struct hid_device *hdev) > +{ > + struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); > + > + mutex_lock(&dj_hdev_list_lock); > + > + if (djrcv_dev->mouse == hdev) > + djrcv_dev->mouse = NULL; > + if (djrcv_dev->keyboard == hdev) > + djrcv_dev->keyboard = NULL; > + if (djrcv_dev->hidpp == hdev) > + djrcv_dev->hidpp = NULL; > + > + kref_put(&djrcv_dev->kref, dj_release_receiver_dev); > + > + mutex_unlock(&dj_hdev_list_lock); > +} > + > +static struct dj_receiver_dev *dj_get_receiver_dev(struct hid_device *hdev, > + unsigned int application, > + bool is_hidpp) > +{ > + struct dj_receiver_dev *djrcv_dev; > + > + mutex_lock(&dj_hdev_list_lock); > + > + djrcv_dev = dj_find_receiver_dev(hdev); > + if (!djrcv_dev) { > + djrcv_dev = kzalloc(sizeof(*djrcv_dev), GFP_KERNEL); > + if (!djrcv_dev) > + goto out; > + > + INIT_WORK(&djrcv_dev->work, delayedwork_callback); > + spin_lock_init(&djrcv_dev->lock); > + if (kfifo_alloc(&djrcv_dev->notif_fifo, > + DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem), > + GFP_KERNEL)) { > + kfree(djrcv_dev); > + djrcv_dev = NULL; > + goto out; > + } > + kref_init(&djrcv_dev->kref); > + list_add_tail(&djrcv_dev->list, &dj_hdev_list); > + } > + > + if (application == HID_GD_KEYBOARD) > + djrcv_dev->keyboard = hdev; > + if (application == HID_GD_MOUSE) > + djrcv_dev->mouse = hdev; > + if (is_hidpp) > + djrcv_dev->hidpp = hdev; > + > + hid_set_drvdata(hdev, djrcv_dev); > +out: > + mutex_unlock(&dj_hdev_list_lock); > + return djrcv_dev; > +} > > static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev, > struct dj_workitem *workitem) > @@ -480,6 +589,16 @@ static void delayedwork_callback(struct work_struct *work) > > spin_lock_irqsave(&djrcv_dev->lock, flags); > > + /* > + * Since we attach to multiple interfaces, we may get scheduled before > + * we are bound to the HID++ interface, catch this. > + */ > + if (!djrcv_dev->ready) { > + hid_err(djrcv_dev->hidpp, "delayedwork queued before hidpp interface was enumerated\n"); So the whole purpose of this is to catch the work being queued while djrcv_dev->hidpp may be NULL, so printing the error using djrcv_dev->hidpp here is not the best of ideas. While trying to add support for the C-UV35 Bluetooth Mini-Receiver in HID proxy mode I manually unbound the driver from the mouse/hidpp interface only and pressed a key causing the work to be queued with a WORKITEM_TYPE_UNKNOWN workitem, then hit this path, resulting in a NULL ptr deref with the spinlock held and after that the whole machine becomes unusable. I've replaced this with: pr_err("%s: delayedwork queued before hidpp interface was enumerated\n"); __func__); For v2 of this patch set, fixing this. Regards, Hans > + spin_unlock_irqrestore(&djrcv_dev->lock, flags); > + return; > + } > + > count = kfifo_out(&djrcv_dev->notif_fifo, &workitem, sizeof(workitem)); > > if (count != sizeof(workitem)) { > @@ -1041,6 +1160,7 @@ static int logi_dj_probe(struct hid_device *hdev, > struct hid_report *rep; > struct dj_receiver_dev *djrcv_dev; > bool has_hidpp = false; > + unsigned long flags; > int retval; > > /* Call to usbhid to fetch the HID descriptors of the current > @@ -1070,27 +1190,15 @@ static int logi_dj_probe(struct hid_device *hdev, > if (!has_hidpp) > return -ENODEV; > > - /* Treat DJ/HID++ interface */ > - > - djrcv_dev = kzalloc(sizeof(struct dj_receiver_dev), GFP_KERNEL); > + /* get the current application attached to the node */ > + rep = list_first_entry(&rep_enum->report_list, struct hid_report, list); > + djrcv_dev = dj_get_receiver_dev(hdev, > + rep->application, has_hidpp); > if (!djrcv_dev) { > dev_err(&hdev->dev, > "%s:failed allocating dj_receiver_dev\n", __func__); > return -ENOMEM; > } > - djrcv_dev->hidpp = hdev; > - INIT_WORK(&djrcv_dev->work, delayedwork_callback); > - spin_lock_init(&djrcv_dev->lock); > - if (kfifo_alloc(&djrcv_dev->notif_fifo, > - DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem), > - GFP_KERNEL)) { > - dev_err(&hdev->dev, > - "%s:failed allocating notif_fifo\n", __func__); > - kfree(djrcv_dev); > - return -ENOMEM; > - } > - hid_set_drvdata(hdev, djrcv_dev); > - > > /* Starts the usb device and connects to upper interfaces hiddev and > * hidraw */ > @@ -1120,6 +1228,9 @@ static int logi_dj_probe(struct hid_device *hdev, > /* Allow incoming packets to arrive: */ > hid_device_io_start(hdev); > > + spin_lock_irqsave(&djrcv_dev->lock, flags); > + djrcv_dev->ready = true; > + spin_unlock_irqrestore(&djrcv_dev->lock, flags); > retval = logi_dj_recv_query_paired_devices(djrcv_dev); > if (retval < 0) { > dev_err(&hdev->dev, "%s:logi_dj_recv_query_paired_devices " > @@ -1137,10 +1248,8 @@ static int logi_dj_probe(struct hid_device *hdev, > hid_hw_stop(hdev); > > hid_hw_start_fail: > - kfifo_free(&djrcv_dev->notif_fifo); > - kfree(djrcv_dev); > + dj_put_receiver_dev(hdev); > return retval; > - > } > > #ifdef CONFIG_PM > @@ -1164,31 +1273,43 @@ static void logi_dj_remove(struct hid_device *hdev) > { > struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); > struct dj_device *dj_dev; > + unsigned long flags; > int i; > > dbg_hid("%s\n", __func__); > > + /* > + * This ensures that if the work gets requeued from another > + * interface of the same receiver it will be a no-op. > + */ > + spin_lock_irqsave(&djrcv_dev->lock, flags); > + djrcv_dev->ready = false; > + spin_unlock_irqrestore(&djrcv_dev->lock, flags); > + > cancel_work_sync(&djrcv_dev->work); > > hid_hw_close(hdev); > hid_hw_stop(hdev); > > - /* I suppose that at this point the only context that can access > - * the djrecv_data is this thread as the work item is guaranteed to > - * have finished and no more raw_event callbacks should arrive after > - * the remove callback was triggered so no locks are put around the > - * code below */ > + /* > + * For proper operation we need access to all interfaces, so we destroy > + * the paired devices when we're unbound from any interface. > + * > + * Note we may still be bound to other interfaces, sharing the same > + * djrcv_dev, so we need locking here. > + */ > for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) { > + spin_lock_irqsave(&djrcv_dev->lock, flags); > dj_dev = djrcv_dev->paired_dj_devices[i]; > + djrcv_dev->paired_dj_devices[i] = NULL; > + spin_unlock_irqrestore(&djrcv_dev->lock, flags); > if (dj_dev != NULL) { > hid_destroy_device(dj_dev->hdev); > kfree(dj_dev); > - djrcv_dev->paired_dj_devices[i] = NULL; > } > } > > - kfifo_free(&djrcv_dev->notif_fifo); > - kfree(djrcv_dev); > + dj_put_receiver_dev(hdev); > } > > static const struct hid_device_id logi_dj_receivers[] = { >
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 9e08b44c76b2..337389c2d6c7 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -119,11 +119,16 @@ struct hidpp_event { } __packed; struct dj_receiver_dev { + struct hid_device *mouse; + struct hid_device *keyboard; struct hid_device *hidpp; struct dj_device *paired_dj_devices[DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN]; + struct list_head list; + struct kref kref; struct work_struct work; struct kfifo notif_fifo; + bool ready; spinlock_t lock; }; @@ -363,6 +368,110 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = { static struct hid_ll_driver logi_dj_ll_driver; static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev); +static void delayedwork_callback(struct work_struct *work); + +static LIST_HEAD(dj_hdev_list); +static DEFINE_MUTEX(dj_hdev_list_lock); + +/* + * dj/HID++ receivers are really a single logical entity, but for BIOS/Windows + * compatibility they have multiple USB interfaces. On HID++ receivers we need + * to listen for input reports on both interfaces. The functions below are used + * to create a single struct dj_receiver_dev for all interfaces belonging to + * a single USB-device / receiver. + */ +static struct dj_receiver_dev *dj_find_receiver_dev(struct hid_device *hdev) +{ + struct dj_receiver_dev *djrcv_dev; + + /* Try to find an already-probed interface from the same device */ + list_for_each_entry(djrcv_dev, &dj_hdev_list, list) { + if (djrcv_dev->mouse && + hid_compare_device_paths(hdev, djrcv_dev->mouse, '/')) { + kref_get(&djrcv_dev->kref); + return djrcv_dev; + } + if (djrcv_dev->keyboard && + hid_compare_device_paths(hdev, djrcv_dev->keyboard, '/')) { + kref_get(&djrcv_dev->kref); + return djrcv_dev; + } + if (djrcv_dev->hidpp && + hid_compare_device_paths(hdev, djrcv_dev->hidpp, '/')) { + kref_get(&djrcv_dev->kref); + return djrcv_dev; + } + } + + return NULL; +} + +static void dj_release_receiver_dev(struct kref *kref) +{ + struct dj_receiver_dev *djrcv_dev = container_of(kref, struct dj_receiver_dev, kref); + + list_del(&djrcv_dev->list); + kfifo_free(&djrcv_dev->notif_fifo); + kfree(djrcv_dev); +} + +static void dj_put_receiver_dev(struct hid_device *hdev) +{ + struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); + + mutex_lock(&dj_hdev_list_lock); + + if (djrcv_dev->mouse == hdev) + djrcv_dev->mouse = NULL; + if (djrcv_dev->keyboard == hdev) + djrcv_dev->keyboard = NULL; + if (djrcv_dev->hidpp == hdev) + djrcv_dev->hidpp = NULL; + + kref_put(&djrcv_dev->kref, dj_release_receiver_dev); + + mutex_unlock(&dj_hdev_list_lock); +} + +static struct dj_receiver_dev *dj_get_receiver_dev(struct hid_device *hdev, + unsigned int application, + bool is_hidpp) +{ + struct dj_receiver_dev *djrcv_dev; + + mutex_lock(&dj_hdev_list_lock); + + djrcv_dev = dj_find_receiver_dev(hdev); + if (!djrcv_dev) { + djrcv_dev = kzalloc(sizeof(*djrcv_dev), GFP_KERNEL); + if (!djrcv_dev) + goto out; + + INIT_WORK(&djrcv_dev->work, delayedwork_callback); + spin_lock_init(&djrcv_dev->lock); + if (kfifo_alloc(&djrcv_dev->notif_fifo, + DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem), + GFP_KERNEL)) { + kfree(djrcv_dev); + djrcv_dev = NULL; + goto out; + } + kref_init(&djrcv_dev->kref); + list_add_tail(&djrcv_dev->list, &dj_hdev_list); + } + + if (application == HID_GD_KEYBOARD) + djrcv_dev->keyboard = hdev; + if (application == HID_GD_MOUSE) + djrcv_dev->mouse = hdev; + if (is_hidpp) + djrcv_dev->hidpp = hdev; + + hid_set_drvdata(hdev, djrcv_dev); +out: + mutex_unlock(&dj_hdev_list_lock); + return djrcv_dev; +} static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev, struct dj_workitem *workitem) @@ -480,6 +589,16 @@ static void delayedwork_callback(struct work_struct *work) spin_lock_irqsave(&djrcv_dev->lock, flags); + /* + * Since we attach to multiple interfaces, we may get scheduled before + * we are bound to the HID++ interface, catch this. + */ + if (!djrcv_dev->ready) { + hid_err(djrcv_dev->hidpp, "delayedwork queued before hidpp interface was enumerated\n"); + spin_unlock_irqrestore(&djrcv_dev->lock, flags); + return; + } + count = kfifo_out(&djrcv_dev->notif_fifo, &workitem, sizeof(workitem)); if (count != sizeof(workitem)) { @@ -1041,6 +1160,7 @@ static int logi_dj_probe(struct hid_device *hdev, struct hid_report *rep; struct dj_receiver_dev *djrcv_dev; bool has_hidpp = false; + unsigned long flags; int retval; /* Call to usbhid to fetch the HID descriptors of the current @@ -1070,27 +1190,15 @@ static int logi_dj_probe(struct hid_device *hdev, if (!has_hidpp) return -ENODEV; - /* Treat DJ/HID++ interface */ - - djrcv_dev = kzalloc(sizeof(struct dj_receiver_dev), GFP_KERNEL); + /* get the current application attached to the node */ + rep = list_first_entry(&rep_enum->report_list, struct hid_report, list); + djrcv_dev = dj_get_receiver_dev(hdev, + rep->application, has_hidpp); if (!djrcv_dev) { dev_err(&hdev->dev, "%s:failed allocating dj_receiver_dev\n", __func__); return -ENOMEM; } - djrcv_dev->hidpp = hdev; - INIT_WORK(&djrcv_dev->work, delayedwork_callback); - spin_lock_init(&djrcv_dev->lock); - if (kfifo_alloc(&djrcv_dev->notif_fifo, - DJ_MAX_NUMBER_NOTIFS * sizeof(struct dj_workitem), - GFP_KERNEL)) { - dev_err(&hdev->dev, - "%s:failed allocating notif_fifo\n", __func__); - kfree(djrcv_dev); - return -ENOMEM; - } - hid_set_drvdata(hdev, djrcv_dev); - /* Starts the usb device and connects to upper interfaces hiddev and * hidraw */ @@ -1120,6 +1228,9 @@ static int logi_dj_probe(struct hid_device *hdev, /* Allow incoming packets to arrive: */ hid_device_io_start(hdev); + spin_lock_irqsave(&djrcv_dev->lock, flags); + djrcv_dev->ready = true; + spin_unlock_irqrestore(&djrcv_dev->lock, flags); retval = logi_dj_recv_query_paired_devices(djrcv_dev); if (retval < 0) { dev_err(&hdev->dev, "%s:logi_dj_recv_query_paired_devices " @@ -1137,10 +1248,8 @@ static int logi_dj_probe(struct hid_device *hdev, hid_hw_stop(hdev); hid_hw_start_fail: - kfifo_free(&djrcv_dev->notif_fifo); - kfree(djrcv_dev); + dj_put_receiver_dev(hdev); return retval; - } #ifdef CONFIG_PM @@ -1164,31 +1273,43 @@ static void logi_dj_remove(struct hid_device *hdev) { struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); struct dj_device *dj_dev; + unsigned long flags; int i; dbg_hid("%s\n", __func__); + /* + * This ensures that if the work gets requeued from another + * interface of the same receiver it will be a no-op. + */ + spin_lock_irqsave(&djrcv_dev->lock, flags); + djrcv_dev->ready = false; + spin_unlock_irqrestore(&djrcv_dev->lock, flags); + cancel_work_sync(&djrcv_dev->work); hid_hw_close(hdev); hid_hw_stop(hdev); - /* I suppose that at this point the only context that can access - * the djrecv_data is this thread as the work item is guaranteed to - * have finished and no more raw_event callbacks should arrive after - * the remove callback was triggered so no locks are put around the - * code below */ + /* + * For proper operation we need access to all interfaces, so we destroy + * the paired devices when we're unbound from any interface. + * + * Note we may still be bound to other interfaces, sharing the same + * djrcv_dev, so we need locking here. + */ for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) { + spin_lock_irqsave(&djrcv_dev->lock, flags); dj_dev = djrcv_dev->paired_dj_devices[i]; + djrcv_dev->paired_dj_devices[i] = NULL; + spin_unlock_irqrestore(&djrcv_dev->lock, flags); if (dj_dev != NULL) { hid_destroy_device(dj_dev->hdev); kfree(dj_dev); - djrcv_dev->paired_dj_devices[i] = NULL; } } - kfifo_free(&djrcv_dev->notif_fifo); - kfree(djrcv_dev); + dj_put_receiver_dev(hdev); } static const struct hid_device_id logi_dj_receivers[] = {
dj/HID++ receivers are really a single logical entity, but for BIOS/Windows compatibility they have multiple USB interfaces. For the upcoming non-unifying receiver support, we need to listen for events from / bind to all USB-interfaces of the receiver. This commit add support to the logitech-dj code for creating a single dj_receiver_dev struct for all interfaces belonging to a single USB-device / receiver, in preparation for adding non-unifying receiver support. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/hid/hid-logitech-dj.c | 175 ++++++++++++++++++++++++++++------ 1 file changed, 148 insertions(+), 27 deletions(-)