| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4773032b-d103-a636-2d36-e9e19baa284b@suse.de>
Date: Wed, 7 Sep 2022 09:31:48 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Takashi Iwai <tiwai@...e.de>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 09/11] drm/udl: Fix inconsistent urbs.count value
during udl_free_urb_list()
Hi
Am 06.09.22 um 09:39 schrieb Takashi Iwai:
> In the current design, udl_get_urb() may be called asynchronously
> during the driver freeing its URL list via udl_free_urb_list().
> The problem is that the sync is determined by comparing the urbs.count
> and urbs.available fields, while we clear urbs.count field only once
> after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> the state becomes inconsistent.
>
> For fixing this inconsistency and also for hardening the locking
> scheme, this patch does a slight refactoring of the code around
> udl_get_urb() and udl_free_urb_list(). Now urbs.count is updated in
> the same spinlock at extracting a URB from the list in
> udl_free_url_list().
>
> Signed-off-by: Takashi Iwai <tiwai@...e.de>
> ---
> drivers/gpu/drm/udl/udl_drv.h | 8 +------
> drivers/gpu/drm/udl/udl_main.c | 44 +++++++++++++++++++++++-----------
> 2 files changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 5923d2e02bc8..d943684b5bbb 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
> int udl_modeset_init(struct drm_device *dev);
> struct drm_connector *udl_connector_init(struct drm_device *dev);
>
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
> -
> -#define GET_URB_TIMEOUT HZ
> -static inline struct urb *udl_get_urb(struct drm_device *dev)
> -{
> - return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
> -}
> +struct urb *udl_get_urb(struct drm_device *dev);
>
> int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
> int udl_sync_pending_urbs(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index de28eeff3155..df7ebe1fdc90 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -23,6 +23,8 @@
> #define WRITES_IN_FLIGHT (20)
> #define MAX_VENDOR_DESCRIPTOR_SIZE 256
>
> +static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout);
> +
> static int udl_parse_vendor_descriptor(struct udl_device *udl)
> {
> struct usb_device *udev = udl_to_usb_device(udl);
> @@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb)
> udl->urbs.available++;
> spin_unlock_irqrestore(&udl->urbs.lock, flags);
>
> - wake_up(&udl->urbs.sleep);
> + wake_up_all(&udl->urbs.sleep);
> }
>
> static void udl_free_urb_list(struct drm_device *dev)
> {
> struct udl_device *udl = to_udl(dev);
> - int count = udl->urbs.count;
> struct urb_node *unode;
> struct urb *urb;
>
> DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
>
> /* keep waiting and freeing, until we've got 'em all */
> - while (count--) {
> - urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> + while (udl->urbs.count) {
> + spin_lock_irq(&udl->urbs.lock);
> + urb = udl_get_urb_locked(udl, MAX_SCHEDULE_TIMEOUT);
> + udl->urbs.count--;
> + spin_unlock_irq(&udl->urbs.lock);
> if (WARN_ON(!urb))
> break;
> unode = urb->context;
> @@ -164,7 +168,8 @@ static void udl_free_urb_list(struct drm_device *dev)
> usb_free_urb(urb);
> kfree(unode);
> }
> - udl->urbs.count = 0;
> +
> + wake_up(&udl->urbs.sleep);
I meant that we should maybe do a wake_up_all() here and not in the
completion handler udl_urb_completion().
In any case
Acked-by: Thomas Zimmermann <tzimmermann@...e.de>
> }
>
> static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> @@ -228,33 +233,44 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> return udl->urbs.count;
> }
>
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> +static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
> {
> - struct udl_device *udl = to_udl(dev);
> - struct urb_node *unode = NULL;
> + struct urb_node *unode;
>
> - if (!udl->urbs.count)
> - return NULL;
> + assert_spin_locked(&udl->urbs.lock);
>
> /* Wait for an in-flight buffer to complete and get re-queued */
> - spin_lock_irq(&udl->urbs.lock);
> if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> + !udl->urbs.count ||
> !list_empty(&udl->urbs.list),
> udl->urbs.lock, timeout)) {
> DRM_INFO("wait for urb interrupted: available: %d\n",
> udl->urbs.available);
> - goto unlock;
> + return NULL;
> }
>
> + if (!udl->urbs.count)
> + return NULL;
> +
> unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
> list_del_init(&unode->entry);
> udl->urbs.available--;
>
> -unlock:
> - spin_unlock_irq(&udl->urbs.lock);
> return unode ? unode->urb : NULL;
> }
>
> +#define GET_URB_TIMEOUT HZ
> +struct urb *udl_get_urb(struct drm_device *dev)
> +{
> + struct udl_device *udl = to_udl(dev);
> + struct urb *urb;
> +
> + spin_lock_irq(&udl->urbs.lock);
> + urb = udl_get_urb_locked(udl, GET_URB_TIMEOUT);
> + spin_unlock_irq(&udl->urbs.lock);
> + return urb;
> +}
> +
> int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
> {
> struct udl_device *udl = to_udl(dev);
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists