Re: [PATCH] drm: serialize access to debugs_nodes.list

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Oct 30, 2011 at 11:04:48PM +0100, Marcin Slusarz wrote:
> Nouveau, when configured with debugfs, creates debugfs files for every
> channel, so structure holding list of files needs to be protected from
> simultaneous changes by multiple threads.
> 
> Without this patch it's possible to hit kernel oops in
> drm_debugfs_remove_files just by running a couple of xterms with
> looped glxinfo.
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c |    5 +++++
>  include/drm/drmP.h            |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 9d2668a..1144fbe 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -120,7 +120,9 @@ int drm_debugfs_create_files(struct drm_info_list *files, int count,
>  		tmp->minor = minor;
>  		tmp->dent = ent;
>  		tmp->info_ent = &files[i];
> +		mutex_lock(&minor->debugfs_nodes.mutex);
>  		list_add(&(tmp->list), &(minor->debugfs_nodes.list));
> +		mutex_unlock(&minor->debugfs_nodes.mutex);
>  	}
>  	return 0;
>  
> @@ -149,6 +151,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	int ret;
>  
>  	INIT_LIST_HEAD(&minor->debugfs_nodes.list);
> +	mutex_init(&minor->debugfs_nodes.mutex);
>  	sprintf(name, "%d", minor_id);
>  	minor->debugfs_root = debugfs_create_dir(name, root);
>  	if (!minor->debugfs_root) {
> @@ -194,6 +197,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
>  	struct drm_info_node *tmp;
>  	int i;
>  
> +	mutex_lock(&minor->debugfs_nodes.mutex);
>  	for (i = 0; i < count; i++) {
>  		list_for_each_safe(pos, q, &minor->debugfs_nodes.list) {
>  			tmp = list_entry(pos, struct drm_info_node, list);
> @@ -204,6 +208,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
>  			}
>  		}
>  	}
> +	mutex_unlock(&minor->debugfs_nodes.mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_debugfs_remove_files);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 9b7c2bb..c70c943 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -970,6 +970,7 @@ struct drm_info_list {
>   * debugfs node structure. This structure represents a debugfs file.
>   */
>  struct drm_info_node {
> +	struct mutex mutex;
>  	struct list_head list;
>  	struct drm_minor *minor;
>  	struct drm_info_list *info_ent;

This is just ugly, you're adding a mutex to every drm_info_node, but only
use the one embedded into the minor. On a quick grep we're only ever using
the list in there, so I suggest to
- replace minor->debugfs_node.list with minor->debugfs_list and kill
  ->debugfs_node
- add the mutex as minor->debugfs_lock
That way it's clear what's going on.

Also, you've forgotten to add the locking to i915/i915_debugfs.c

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48