Re: [PATCH 13/18] drm/xe: Debug metadata create/destroy ioctls

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

On Tue, 2024-10-01 at 17:25 +0100, Matthew Auld wrote:
> Hi,
> 
> On 01/10/2024 15:43, Mika Kuoppala wrote:
> > From: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
> > 
> > Ad a part of eu debug feature introduce debug metadata objects.
> > These are to be used to pass metadata between client and debugger,
> > by attaching them to vm_bind operations.
> > 
> > todo: WORK_IN_PROGRESS_* defines need to be reworded/refined when
> >        the real usage and need is established by l0+gdb.
> > 
> > v2: - include uapi/drm/xe_drm.h
> >      - metadata behind kconfig (Mika)
> > 
> > Signed-off-by: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/xe/Makefile                  |   3 +-
> >   drivers/gpu/drm/xe/xe_debug_metadata.c       | 108 +++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_debug_metadata.h       |  50 +++++++++
> >   drivers/gpu/drm/xe/xe_debug_metadata_types.h |  28 +++++
> >   drivers/gpu/drm/xe/xe_device.c               |   5 +
> >   drivers/gpu/drm/xe/xe_device.h               |   2 +
> >   drivers/gpu/drm/xe/xe_device_types.h         |   7 ++
> >   drivers/gpu/drm/xe/xe_eudebug.c              |  13 +++
> >   include/uapi/drm/xe_drm.h                    |  53 ++++++++-
> >   9 files changed, 267 insertions(+), 2 deletions(-)
> >   create mode 100644 drivers/gpu/drm/xe/xe_debug_metadata.c
> >   create mode 100644 drivers/gpu/drm/xe/xe_debug_metadata.h
> >   create mode 100644 drivers/gpu/drm/xe/xe_debug_metadata_types.h
> > 
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 660c1dbba8d0..a94f771529a4 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -114,7 +114,8 @@ xe-y += xe_bb.o \
> >   	xe_wa.o \
> >   	xe_wopcm.o
> >   
> > -xe-$(CONFIG_DRM_XE_EUDEBUG) += xe_eudebug.o
> > +xe-$(CONFIG_DRM_XE_EUDEBUG) += xe_eudebug.o \
> > +	xe_debug_metadata.o
> >   
> >   xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
> >   
> > diff --git a/drivers/gpu/drm/xe/xe_debug_metadata.c b/drivers/gpu/drm/xe/xe_debug_metadata.c
> > new file mode 100644
> > index 000000000000..72a00b628475
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_debug_metadata.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +#include "xe_debug_metadata.h"
> > +
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_file.h>
> > +#include <uapi/drm/xe_drm.h>
> > +
> > +#include "xe_device.h"
> > +#include "xe_macros.h"
> > +
> > +static void xe_debug_metadata_release(struct kref *ref)
> > +{
> > +	struct xe_debug_metadata *mdata = container_of(ref, struct xe_debug_metadata, refcount);
> > +
> > +	kvfree(mdata->ptr);
> > +	kfree(mdata);
> > +}
> > +
> > +void xe_debug_metadata_put(struct xe_debug_metadata *mdata)
> > +{
> > +	kref_put(&mdata->refcount, xe_debug_metadata_release);
> > +}
> > +
> > +int xe_debug_metadata_create_ioctl(struct drm_device *dev,
> > +				   void *data,
> > +				   struct drm_file *file)
> > +{
> > +	struct xe_device *xe = to_xe_device(dev);
> > +	struct xe_file *xef = to_xe_file(file);
> > +	struct drm_xe_debug_metadata_create *args = data;
> > +	struct xe_debug_metadata *mdata;
> > +	int err;
> > +	u32 id;
> > +
> > +	if (XE_IOCTL_DBG(xe, args->extensions))
> > +		return -EINVAL;
> > +
> > +	if (XE_IOCTL_DBG(xe, args->type > DRM_XE_DEBUG_METADATA_PROGRAM_MODULE))
> > +		return -EINVAL;
> > +
> > +	if (XE_IOCTL_DBG(xe, !args->user_addr || !args->len))
> > +		return -EINVAL;
> > +
> > +	if (XE_IOCTL_DBG(xe, !access_ok(u64_to_user_ptr(args->user_addr), args->len)))
> > +		return -EFAULT;
> > +
> > +	mdata = kzalloc(sizeof(*mdata), GFP_KERNEL);
> > +	if (!mdata)
> > +		return -ENOMEM;
> > +
> > +	mdata->len = args->len;
> > +	mdata->type = args->type;
> > +
> > +	mdata->ptr = kvmalloc(mdata->len, GFP_KERNEL);
> > +	if (!mdata->ptr) {
> > +		kfree(mdata);
> > +		return -ENOMEM;
> > +	}
> > +	kref_init(&mdata->refcount);
> > +
> > +	err = copy_from_user(mdata->ptr, u64_to_user_ptr(args->user_addr), mdata->len);
> > +	if (err) {
> > +		err = -EFAULT;
> > +		goto put_mdata;
> > +	}
> > +
> > +	mutex_lock(&xef->eudebug.metadata.lock);
> > +	err = xa_alloc(&xef->eudebug.metadata.xa, &id, mdata, xa_limit_32b, GFP_KERNEL);
> > +	mutex_unlock(&xef->eudebug.metadata.lock);
> > +
> > +	args->metadata_id = id;
> 
> Just some drive-by-comments. Potentially this is passing uninitialised 
> data from the stack back to userspace, assuming the xa_alloc here fails? 
> Maybe safer to leave metadata_id zeroed on err?

That's mine over oversight. Thanks for catching that!
> 
> > +	mdata->id = id;
> 
> It looks like mdata can go out of scope since user can technically guess 
> the id and call the destroy ioctl before this ioctl returns. I think 
> publishing the id needs to go last or needs some kind of locking.
> 
> Also are we sure we need to store the user id in mdata in the first 
> place? Usually for user id kmd doesn't need to track it in such a way.

I think we can get rid of it fairly easy, I will to that in next revision. 

Regards,
Dominik