LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	tzimmermann@suse.de, airlied@linux.ie, daniel@ffwll.ch,
	sumit.semwal@linaro.org, christian.koenig@amd.com,
	axboe@kernel.dk, oleg@redhat.com, tglx@linutronix.de,
	dvyukov@google.com, walter-zh.wu@mediatek.com,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	intel-gfx@lists.freedesktop.org, linux-media@vger.kernel.org,
	linaro-mm-sig@lists.linaro.org, skhan@linuxfoundation.org,
	gregkh@linuxfoundation.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v3 5/9] drm: protect magic_map,unique{_len} with master_lookup_lock
Date: Wed, 18 Aug 2021 12:43:46 +0200	[thread overview]
Message-ID: <YRzkYlcDDgrwDT81@phenom.ffwll.local> (raw)
In-Reply-To: <20210818073824.1560124-6-desmondcheongzx@gmail.com>

On Wed, Aug 18, 2021 at 03:38:20PM +0800, Desmond Cheong Zhi Xi wrote:
> Currently, drm_device.master_mutex is used to serialize writes to the
> drm_master.magic_map idr and to protect drm_master.unique{_len}.
> 
> In preparation for converting drm_device.master_mutex into an outer
> rwsem that might be read locked before entering some of these
> functions, we can instead serialize access to drm_master.magic_map and
> drm_master.unique{_len} using drm_device.master_lookup_lock which is
> an inner lock.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  drivers/gpu/drm/drm_auth.c  | 12 +++++++-----
>  drivers/gpu/drm/drm_ioctl.c | 10 ++++++----
>  include/drm/drm_auth.h      |  6 +++---
>  include/drm/drm_device.h    |  7 ++++++-
>  4 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index b7230604496b..0acb444fbbac 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -98,10 +98,10 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  	struct drm_master *master;
>  	int ret = 0;
>  
> -	mutex_lock(&dev->master_mutex);
> +	spin_lock(&dev->master_lookup_lock);
>  	master = file_priv->master;
>  	if (!master) {
> -		mutex_unlock(&dev->master_mutex);
> +		spin_unlock(&dev->master_lookup_lock);
>  		return -EINVAL;
>  	}
>  
> @@ -112,7 +112,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			file_priv->magic = ret;
>  	}
>  	auth->magic = file_priv->magic;
> -	mutex_unlock(&dev->master_mutex);
> +	spin_unlock(&dev->master_lookup_lock);
>  
>  	DRM_DEBUG("%u\n", auth->magic);
>  
> @@ -127,13 +127,13 @@ int drm_authmagic(struct drm_device *dev, void *data,
>  
>  	DRM_DEBUG("%u\n", auth->magic);
>  
> -	mutex_lock(&dev->master_mutex);
> +	spin_lock(&dev->master_lookup_lock);
>  	file = idr_find(&file_priv->master->magic_map, auth->magic);
>  	if (file) {
>  		file->authenticated = 1;
>  		idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
>  	}
> -	mutex_unlock(&dev->master_mutex);
> +	spin_unlock(&dev->master_lookup_lock);
>  
>  	return file ? 0 : -EINVAL;
>  }
> @@ -366,8 +366,10 @@ void drm_master_release(struct drm_file *file_priv)
>  	if (!master)
>  		goto unlock;
>  
> +	spin_lock(&dev->master_lookup_lock);
>  	if (file_priv->magic)
>  		idr_remove(&master->magic_map, file_priv->magic);
> +	spin_unlock(&dev->master_lookup_lock);
>  
>  	if (!drm_is_current_master_locked(file_priv))
>  		goto out;
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 4d029d3061d9..e5c3845b6e62 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -119,21 +119,21 @@ int drm_getunique(struct drm_device *dev, void *data,
>  	struct drm_unique *u = data;
>  	struct drm_master *master;
>  
> -	mutex_lock(&dev->master_mutex);
> +	spin_lock(&dev->master_lookup_lock);
>  	master = file_priv->master;
>  	if (!master) {
> -		mutex_unlock(&dev->master_mutex);
> +		spin_unlock(&dev->master_lookup_lock);
>  		return -EINVAL;
>  	}
>  
>  	if (u->unique_len >= master->unique_len) {
>  		if (copy_to_user(u->unique, master->unique, master->unique_len)) {
> -			mutex_unlock(&dev->master_mutex);
> +			spin_unlock(&dev->master_lookup_lock);

copy_to_user while holding a spinlock isn't going to work well, at least
when we take a fault. The might_fault() annotations (if enabled) should
catch that.

Which is really annoying, because it kinda puts a wrench into this neat
plan here :-/

>  			return -EFAULT;
>  		}
>  	}
>  	u->unique_len = master->unique_len;
> -	mutex_unlock(&dev->master_mutex);
> +	spin_unlock(&dev->master_lookup_lock);
>  
>  	return 0;
>  }
> @@ -405,7 +405,9 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  			 * Version 1.1 includes tying of DRM to specific device
>  			 * Version 1.4 has proper PCI domain support
>  			 */
> +			spin_lock(&dev->master_lookup_lock);
>  			retcode = drm_set_busid(dev, file_priv);
> +			spin_unlock(&dev->master_lookup_lock);

Similar issue with drm_set_busid, calling kmalloc under a spinlock isn't a
good idea. This one here is at least much easier to fix by pushing the
locking down a lot.

I'm wondering a bit whether a better fix for these ioctls wouldn't be to
- drop the DRM_MASTER flag from the ioctl table
- take the rwsem in write mode (which would replace our current
  dev->master_lock) and check for master status while holding that lock

I think that would result in simpler locking or am I missing something?
Maybe this could even work as a replacment for the lookup spinlock, since
we're untangling the nesting quite a bit?

Really just tossing ideas around since I feel like we don't have the best
one yet ...
-Daniel

>  			if (retcode)
>  				goto done;
>  		}
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index ba248ca8866f..f5be73153798 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -67,17 +67,17 @@ struct drm_master {
>  	struct drm_device *dev;
>  	/**
>  	 * @unique: Unique identifier: e.g. busid. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_lookup_lock.
>  	 */
>  	char *unique;
>  	/**
>  	 * @unique_len: Length of unique field. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_lookup_lock.
>  	 */
>  	int unique_len;
>  	/**
>  	 * @magic_map: Map of used authentication tokens. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_lookup_lock.
>  	 */
>  	struct idr magic_map;
>  	void *driver_priv;
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 506eb2784819..cf5d15aeb25f 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -152,7 +152,12 @@ struct drm_device {
>  	 */
>  	struct mutex master_mutex;
>  
> -	/** @master_lookup_lock: Serializes &drm_file.master. */
> +	/**
> +	 * @master_lookup_lock:
> +	 *
> +	 * Serializes &drm_file.master, &drm_master.magic_map,
> +	 * &drm_master.unique, and &drm_master.unique_len.
> +	 */
>  	spinlock_t master_lookup_lock;
>  
>  	/**
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply	other threads:[~2021-08-18 10:43 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-18  7:38 [PATCH v3 0/9] drm, kernel: update locking for DRM Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 1/9] drm: move master_lookup_lock into drm_device Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 2/9] drm: hold master_lookup_lock when releasing a drm_file's master Desmond Cheong Zhi Xi
2021-08-18 10:05   ` Daniel Vetter
2021-08-18 14:50     ` Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 3/9] drm: check for null master in drm_is_current_master_locked Desmond Cheong Zhi Xi
2021-08-18 10:04   ` Daniel Vetter
2021-08-18  7:38 ` [PATCH v3 4/9] drm: fix potential null ptr dereferences in drm_{auth,ioctl} Desmond Cheong Zhi Xi
2021-08-18 10:11   ` Daniel Vetter
2021-08-18 15:37     ` Desmond Cheong Zhi Xi
2021-08-18 16:33       ` [Intel-gfx] [PATCH v3 4/9] drm: fix potential null ptr dereferences in drm_{auth, ioctl} Daniel Vetter
2021-08-19  5:31         ` Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 5/9] drm: protect magic_map,unique{_len} with master_lookup_lock Desmond Cheong Zhi Xi
2021-08-18 10:43   ` Daniel Vetter [this message]
2021-08-18  7:38 ` [PATCH v3 6/9] drm: convert drm_device.master_mutex into a rwsem Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 7/9] drm: update global mutex lock in the ioctl handler Desmond Cheong Zhi Xi
2021-08-18 11:02   ` Daniel Vetter
2021-08-19 10:52     ` Desmond Cheong Zhi Xi
2021-08-19 11:21       ` [Intel-gfx] " Daniel Vetter
2021-08-18  7:38 ` [PATCH v3 8/9] kernel: export task_work_add Desmond Cheong Zhi Xi
2021-08-18 11:06   ` Daniel Vetter
2021-08-19  9:26   ` Christoph Hellwig
2021-08-19  9:40     ` Desmond Cheong Zhi Xi
2021-08-18  7:38 ` [PATCH v3 9/9] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YRzkYlcDDgrwDT81@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=axboe@kernel.dk \
    --cc=christian.koenig@amd.com \
    --cc=desmondcheongzx@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=dvyukov@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=oleg@redhat.com \
    --cc=skhan@linuxfoundation.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tglx@linutronix.de \
    --cc=tzimmermann@suse.de \
    --cc=walter-zh.wu@mediatek.com \
    --subject='Re: [PATCH v3 5/9] drm: protect magic_map,unique{_len} with master_lookup_lock' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).