Hi Steven,

On 14.04.2025 10:50, Steven Price wrote:
> Hi Adrián,
>
> On 11/04/2025 16:03, Adrián Larumbe wrote:
> > Add a new character string Panthor BO field, and a function that allows
> > setting it from within the driver.
> >
> > Driver takes care of freeing the string when it's replaced or no longer
> > needed at object destruction time, but allocating it is the responsibility
> > of callers.
> >
> > Signed-off-by: Adrián Larumbe <adrian.laru...@collabora.com>
> > Reviewed-by: Boris Brezillon <boris.brezil...@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c 
> > b/drivers/gpu/drm/panthor/panthor_gem.c
> > index 8244a4e6c2a2..af0ac17f357f 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright 2019 Linaro, Ltd, Rob Herring <r...@kernel.org> */
> >  /* Copyright 2023 Collabora ltd. */
> >
> > +#include <linux/cleanup.h>
> >  #include <linux/dma-buf.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/err.h>
> > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct 
> > drm_gem_object *obj)
> >     struct panthor_gem_object *bo = to_panthor_bo(obj);
> >     struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem;
> >
> > +   /*
> > +    * Label might have been allocated with kstrdup_const(),
> > +    * we need to take that into account when freeing the memory
> > +    */
> > +   kfree_const(bo->label.str);
> > +
> > +   mutex_destroy(&bo->label.lock);
> > +
> >     drm_gem_free_mmap_offset(&bo->base.base);
> >     mutex_destroy(&bo->gpuva_list_lock);
> >     drm_gem_shmem_free(&bo->base);
> > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct 
> > drm_device *ddev, size_t
> >     obj->base.map_wc = !ptdev->coherent;
> >     mutex_init(&obj->gpuva_list_lock);
> >     drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock);
> > +   mutex_init(&obj->label.lock);
> >
> >     return &obj->base.base;
> >  }
> > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file,
> >
> >     return ret;
> >  }
> > +
> > +void
> > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label)
> > +{
> > +   struct panthor_gem_object *bo = to_panthor_bo(obj);
> > +   const char *old_label;
> > +
> > +   scoped_guard(mutex, &bo->label.lock) {
> > +           old_label = bo->label.str;
> > +           bo->label.str = label;
> > +   }
> > +
> > +   kfree(old_label);
>
> Shouldn't this be kfree_const()? I suspect as things stand we can't
> trigger this bug but it's inconsistent.

This could only be called either from the set_label() ioctl, in which case
old_label could be NULL or a pointer to a string obtained from strdup(); or from
panthor_gem_kernel_bo_set_label(). In the latter case, it could only ever be
NULL, since we don't reassign kernel BO labels, so it'd be safe too.

However I do agree that it's not consistent, and then in the future perhaps
relabelling kernel BO's might be justified, so I'll change it to kfree_const().

> > +}
> > +
> > +void
> > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char 
> > *label)
> > +{
> > +   const char *str;
> > +
> > +   str = kstrdup_const(label, GFP_KERNEL);
> > +   if (!str) {
>
> In the next patch you permit user space to clear the label
> (args->size==0) which means label==NULL and AFAICT kstrdup_const() will
> return NULL in this case triggering this warning.

Kernel and UM-exposed BO's should never experience cross-labelling, so in theory
this scenario ought to be impossible. The only way 
panthor_gem_kernel_bo_set_label()
might take NULL in the 'label' argument is that someone called 
panthor_kernel_bo_create()
with NULL for its name 'argument'.

I think as a defensive check, I could do something as follows

void
panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label)
{
        const char *str;

        /* We should never attempt labelling a UM-exposed GEM object */
        if (drm_WARN_ON(bo->obj->dev, &bo->obj->handle_count > 0))
                return;

        if (!label)
                return;

       [...]
}

> Thanks,
> Steve
>
> > +           /* Failing to allocate memory for a label isn't a fatal 
> > condition */
> > +           drm_warn(bo->obj->dev, "Not enough memory to allocate BO 
> > label");
> > +           return;
> > +   }
> > +
> > +   panthor_gem_bo_set_label(bo->obj, str);
> > +}
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h 
> > b/drivers/gpu/drm/panthor/panthor_gem.h
> > index 1a363bb814f4..af0d77338860 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.h
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> > @@ -46,6 +46,20 @@ struct panthor_gem_object {
> >
> >     /** @flags: Combination of drm_panthor_bo_flags flags. */
> >     u32 flags;
> > +
> > +   /**
> > +    * @label: BO tagging fields. The label can be assigned within the
> > +    * driver itself or through a specific IOCTL.
> > +    */
> > +   struct {
> > +           /**
> > +            * @label.str: Pointer to NULL-terminated string,
> > +            */
> > +           const char *str;
> > +
> > +           /** @lock.str: Protects access to the @label.str field. */
> > +           struct mutex lock;
> > +   } label;
> >  };
> >
> >  /**
> > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file,
> >                            struct panthor_vm *exclusive_vm,
> >                            u64 *size, u32 flags, uint32_t *handle);
> >
> > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char 
> > *label);
> > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const 
> > char *label);
> > +
> >  static inline u64
> >  panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo)
> >  {

Adrian Larumbe

Reply via email to