Hi Ricardo,
Thank you for the patch.
On Tue, Nov 05, 2024 at 10:53:59AM +0000, Ricardo Ribalda wrote:
> We used the wrong device for the device managed functions. We used the
> usb device, when we should be using the interface device.
>
> If we unbind the driver from the usb interface, the cleanup functions
> are never called. In our case, the IRQ is never disabled.
>
> If an IRQ is triggered, it will try to access memory sections that are
> already free, causing an OOPS.
The commit message should explain why you're switching away from
devm_request_threaded_irq().
>
> Luckily this bug has small impact, as it is only affected by devices
> with gpio units and the user has to unbind the device, a disconnect will
> not trigger this error.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> Changes in v2: Thanks to Laurent.
> - The main structure is not allocated with devres so there is a small
> period of time where we can get an irq with the structure free. Do not
> use devres for the IRQ.
> - Link to v1: https://lore.kernel.org/r/20241031-uvc-crashrmmod-v1-1-059fe593b1e6@xxxxxxxxxxxx
> ---
> drivers/media/usb/uvc/uvc_driver.c | 28 +++++++++++++++++++++-------
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index a96f6ca0889f..af6aec27083c 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1295,14 +1295,14 @@ static int uvc_gpio_parse(struct uvc_device *dev)
> struct gpio_desc *gpio_privacy;
> int irq;
>
> - gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy",
> + gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> GPIOD_IN);
> if (IS_ERR_OR_NULL(gpio_privacy))
> return PTR_ERR_OR_ZERO(gpio_privacy);
>
> irq = gpiod_to_irq(gpio_privacy);
> if (irq < 0)
> - return dev_err_probe(&dev->udev->dev, irq,
> + return dev_err_probe(&dev->intf->dev, irq,
> "No IRQ for privacy GPIO\n");
>
> unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> @@ -1329,15 +1329,28 @@ static int uvc_gpio_parse(struct uvc_device *dev)
> static int uvc_gpio_init_irq(struct uvc_device *dev)
> {
> struct uvc_entity *unit = dev->gpio_unit;
> + int ret;
>
> if (!unit || unit->gpio.irq < 0)
> return 0;
>
> - return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL,
> - uvc_gpio_irq,
> - IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> - IRQF_TRIGGER_RISING,
> - "uvc_privacy_gpio", dev);
> + ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> + IRQF_TRIGGER_RISING,
> + "uvc_privacy_gpio", dev);
> +
> + if (!ret)
> + dev->gpio_unit->gpio.inited = true;
unit->gpio.inited = true;
> +
> + return ret;
> +}
> +
> +static void uvc_gpio_cleanup(struct uvc_device *dev)
> +{
> + if (!dev->gpio_unit || !dev->gpio_unit->gpio.inited)
> + return;
> +
> + free_irq(dev->gpio_unit->gpio.irq, dev);
> }
>
> /* ------------------------------------------------------------------------
> @@ -1880,6 +1893,7 @@ static void uvc_delete(struct kref *kref)
> struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
> struct list_head *p, *n;
>
> + uvc_gpio_cleanup(dev);
This belongs to uvc_unregister_video(), or you'll have a race between
the release of the GPIO happening after .disconnect() returns, and
uvc_gpio_event() calling gpiod_get_value_cansleep().
I understand the desire to get such a fix merged quickly, but taking
time to think about race conditions instead of speeding up to get a
patch out of the door would be better. The alternative where I have to
flag race conditions multiple times during review is slower, and is more
work for everybody.
> uvc_status_cleanup(dev);
> uvc_ctrl_cleanup_device(dev);
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 07f9921d83f2..376cd670539b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -234,6 +234,7 @@ struct uvc_entity {
> u8 *bmControls;
> struct gpio_desc *gpio_privacy;
> int irq;
> + bool inited;
As Sakari, I also prefer "initialized". Another option to save a few
bytes of memory here is to set irq to -1 when request_threaded_irq()
fails and test that in uvc_gpio_cleanup().
> } gpio;
> };
>
>
> ---
> base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
> change-id: 20241031-uvc-crashrmmod-666de3fc9141
--
Regards,
Laurent Pinchart
