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

[Matthew Auld <matthew.auld@intel.com>]

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?

> +	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.