Re: [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name

From: David Herrmann <dh.herrmann@gmail.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name
Date: Mon, 15 May 2017 12:50:04 +0200	[thread overview]
Message-ID: <CANq1E4T0gzcizrDMYzwC7drPh9YW+BCz2aFNBJDfbDqiopHk+w@mail.gmail.com> (raw)
In-Reply-To: <20170515101055.10266-1-chris@chris-wilson.co.uk>

Hey

On Mon, May 15, 2017 at 12:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Constructing the name takes the majority of the time for allocating a
> sync_file to wrap a fence, and the name is very rarely used (only via
> the sync_file status user interface). To reduce the impact on the common
> path (that of creating sync_file to pass around), defer the construction
> of the name until it is first used.
>
> v2: Update kerneldoc (kbuild test robot)
> v3: sync_debug.c was peeking at the name
> v4: Comment upon the potential race between two users of
> sync_file_get_name() and claim that such a race is below the level of
> notice. However, to prevent any future nuisance, use a global spinlock
> to serialize the assignment of the name.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/dma-buf/sync_debug.c |  3 ++-
>  drivers/dma-buf/sync_file.c  | 54 ++++++++++++++++++++++++++++++++++++++------
>  include/linux/sync_file.h    |  5 ++--
>  3 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 4b1731ee7458..9a93f1085c63 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s,
>  {
>         int i;
>
> -       seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
> +       seq_printf(s, "[%p] %s: %s\n", sync_file,
> +                  sync_file_get_name(sync_file),
>                    sync_status_str(dma_fence_get_status(sync_file->fence)));
>
>         if (dma_fence_is_array(sync_file->fence)) {
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index c9eb4997cfcc..0d54de79ba76 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>
>         sync_file->fence = dma_fence_get(fence);
>
> -       snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> -                fence->ops->get_driver_name(fence),
> -                fence->ops->get_timeline_name(fence), fence->context,
> -                fence->seqno);
> -
>         return sync_file;
>  }
>  EXPORT_SYMBOL(sync_file_create);
> @@ -129,6 +124,51 @@ struct dma_fence *sync_file_get_fence(int fd)
>  }
>  EXPORT_SYMBOL(sync_file_get_fence);
>
> +/**
> + * sync_file_get_name - get the name of the sync_file
> + * @sync_file:         sync_file to get the fence from
> + *
> + * Each sync_file may have a name assigned either by the user (when merging
> + * sync_files together) or created from the fence it contains. However,
> + * construction of the name is deferred until first use.
> + *
> + * Returns: a string representing the name
> + */
> +char *sync_file_get_name(struct sync_file *sync_file)
> +{
> +       static DEFINE_SPINLOCK(name_lock);
> +
> +       if (!sync_file->user_name[0]) {
> +               char buf[sizeof(sync_file->user_name)];
> +               struct dma_fence *fence;
> +               int len;
> +
> +               rcu_read_lock();
> +               fence = sync_file->fence;
> +               len = scnprintf(buf, sizeof(buf), "%s-%s%llu-%d",
> +                               fence->ops->get_driver_name(sync_file->fence),
> +                               fence->ops->get_timeline_name(sync_file->fence),
> +                               fence->context,
> +                               fence->seqno);
> +
> +               /* This is mildly racy, so we stage the printf into a local
> +                * buffer and do the copy with preempt disabled to trim the
> +                * race down to a few cycles. To completely eliminate the
> +                * race, we do the final assignment underneath a global
> +                * spinlock. A global lock should be adequate here for this
> +                * rare path (called from the middle of a user sw_sync status
> +                * ioctl and from debugfs).
> +                */
> +               spin_lock(&name_lock);
> +               if (!sync_file->user_name[0])
> +                       memcpy(sync_file->user_name, buf, len + 1);

A racing reader might get a truncated string here. Note that a racing
reader does not take the lock as soon as the first byte is visible.
Not sure what the name is used for, but with that code it is not
reliable.

Also, please zero-terminate the string... scnprintf() does not
guarantee that, if it follows posix.

> +               spin_unlock(&name_lock);
> +               rcu_read_unlock();
> +       }
> +
> +       return sync_file->user_name;
> +}
> +
>  static int sync_file_set_fence(struct sync_file *sync_file,
>                                struct dma_fence **fences, int num_fences)
>  {
> @@ -266,7 +306,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>                 goto err;
>         }
>
> -       strlcpy(sync_file->name, name, sizeof(sync_file->name));
> +       strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
>         return sync_file;
>
>  err:
> @@ -419,7 +459,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>         }
>
>  no_fences:
> -       strlcpy(info.name, sync_file->name, sizeof(info.name));
> +       strlcpy(info.name, sync_file_get_name(sync_file), sizeof(info.name));

Don't use strlcpy(). It requires your source to be 0-terminated. Which
is not the case for sync_file->user_name. Either make sure user_name
is 0-terminated, or use strcpy().

Thanks
David

>         info.status = dma_fence_is_signaled(sync_file->fence);
>         info.num_fences = num_fences;
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index d37beefdfbd5..0f3f05325c84 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -23,7 +23,7 @@
>  /**
>   * struct sync_file - sync file to export to the userspace
>   * @file:              file representing this fence
> - * @name:              name of sync_file.  Useful for debugging
> + * @user_name:         name of sync_file.  Useful for debugging
>   * @sync_file_list:    membership in global file list
>   * @wq:                        wait queue for fence signaling
>   * @fence:             fence with the fences in the sync_file
> @@ -31,7 +31,7 @@
>   */
>  struct sync_file {
>         struct file             *file;
> -       char                    name[32];
> +       char                    user_name[32];
>  #ifdef CONFIG_DEBUG_FS
>         struct list_head        sync_file_list;
>  #endif
> @@ -46,5 +46,6 @@ struct sync_file {
>
>  struct sync_file *sync_file_create(struct dma_fence *fence);
>  struct dma_fence *sync_file_get_fence(int fd);
> +char *sync_file_get_name(struct sync_file *sync_file);
>
>  #endif /* _LINUX_SYNC_H */
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel