[PATCH 0/3] drm/msm: Add comm/cmdline override

[PATCH 0/3] drm/msm: Add comm/cmdline override

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add a way to override comm/cmdline per-drm_file.  This is useful for
VM scenarios where the host process is just a proxy for the actual
guest process.

Rob Clark (3):
  drm/msm: Add support for pointer params
  drm/msm: Split out helper to get comm/cmdline
  drm/msm: Add a way to override processes comm/cmdline

 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 46 +++++++++++++++++++++++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  4 +--
 drivers/gpu/drm/msm/msm_drv.c           |  8 ++---
 drivers/gpu/drm/msm/msm_gpu.c           | 39 ++++++++++++---------
 drivers/gpu/drm/msm/msm_gpu.h           | 10 ++++--
 drivers/gpu/drm/msm/msm_rd.c            |  5 +--
 drivers/gpu/drm/msm/msm_submitqueue.c   |  2 ++
 include/uapi/drm/msm_drm.h              |  4 +++
 8 files changed, 90 insertions(+), 28 deletions(-)

-- 
2.35.1

[PATCH 1/3] drm/msm: Add support for pointer params

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The 64b value field is already suffient to hold a pointer instead of
immediate, but we also need a length field.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 12 ++++++++++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  4 ++--
 drivers/gpu/drm/msm/msm_drv.c           |  8 ++++----
 drivers/gpu/drm/msm/msm_gpu.h           |  4 ++--
 drivers/gpu/drm/msm/msm_rd.c            |  5 +++--
 include/uapi/drm/msm_drm.h              |  2 ++
 6 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 9efc84929be0..3d307b34854d 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -229,10 +229,14 @@ adreno_iommu_create_address_space(struct msm_gpu *gpu,
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-		     uint32_t param, uint64_t *value)
+		     uint32_t param, uint64_t *value, uint32_t *len)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 
+	/* No pointer params yet */
+	if (*len != 0)
+		return -EINVAL;
+
 	switch (param) {
 	case MSM_PARAM_GPU_ID:
 		*value = adreno_gpu->info->revn;
@@ -284,8 +288,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 }
 
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-		     uint32_t param, uint64_t value)
+		     uint32_t param, uint64_t value, uint32_t len)
 {
+	/* No pointer params yet */
+	if (len != 0)
+		return -EINVAL;
+
 	switch (param) {
 	case MSM_PARAM_SYSPROF:
 		if (!capable(CAP_SYS_ADMIN))
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 0490c5fbb780..ab3b5ef80332 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -281,9 +281,9 @@ static inline int adreno_is_a650_family(struct adreno_gpu *gpu)
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-		     uint32_t param, uint64_t *value);
+		     uint32_t param, uint64_t *value, uint32_t *len);
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-		     uint32_t param, uint64_t value);
+		     uint32_t param, uint64_t value, uint32_t len);
 const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu,
 		const char *fwname);
 struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 780f9748aaaf..a5eed5738ac8 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -610,7 +610,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data,
 	/* for now, we just have 3d pipe.. eventually this would need to
 	 * be more clever to dispatch to appropriate gpu module:
 	 */
-	if (args->pipe != MSM_PIPE_3D0)
+	if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0))
 		return -EINVAL;
 
 	gpu = priv->gpu;
@@ -619,7 +619,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void *data,
 		return -ENXIO;
 
 	return gpu->funcs->get_param(gpu, file->driver_priv,
-				     args->param, &args->value);
+				     args->param, &args->value, &args->len);
 }
 
 static int msm_ioctl_set_param(struct drm_device *dev, void *data,
@@ -629,7 +629,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data,
 	struct drm_msm_param *args = data;
 	struct msm_gpu *gpu;
 
-	if (args->pipe != MSM_PIPE_3D0)
+	if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0))
 		return -EINVAL;
 
 	gpu = priv->gpu;
@@ -638,7 +638,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void *data,
 		return -ENXIO;
 
 	return gpu->funcs->set_param(gpu, file->driver_priv,
-				     args->param, args->value);
+				     args->param, args->value, args->len);
 }
 
 static int msm_ioctl_gem_new(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index a84140055920..c28c2ad9f52e 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -44,9 +44,9 @@ struct msm_gpu_config {
  */
 struct msm_gpu_funcs {
 	int (*get_param)(struct msm_gpu *gpu, struct msm_file_private *ctx,
-			 uint32_t param, uint64_t *value);
+			 uint32_t param, uint64_t *value, uint32_t *len);
 	int (*set_param)(struct msm_gpu *gpu, struct msm_file_private *ctx,
-			 uint32_t param, uint64_t value);
+			 uint32_t param, uint64_t value, uint32_t len);
 	int (*hw_init)(struct msm_gpu *gpu);
 	int (*pm_suspend)(struct msm_gpu *gpu);
 	int (*pm_resume)(struct msm_gpu *gpu);
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
index 9d835331f214..a92ffde53f0b 100644
--- a/drivers/gpu/drm/msm/msm_rd.c
+++ b/drivers/gpu/drm/msm/msm_rd.c
@@ -180,6 +180,7 @@ static int rd_open(struct inode *inode, struct file *file)
 	struct msm_gpu *gpu = priv->gpu;
 	uint64_t val;
 	uint32_t gpu_id;
+	uint32_t zero = 0;
 	int ret = 0;
 
 	if (!gpu)
@@ -200,12 +201,12 @@ static int rd_open(struct inode *inode, struct file *file)
 	 *
 	 * Note: These particular params do not require a context
 	 */
-	gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val);
+	gpu->funcs->get_param(gpu, NULL, MSM_PARAM_GPU_ID, &val, &zero);
 	gpu_id = val;
 
 	rd_write_section(rd, RD_GPU_ID, &gpu_id, sizeof(gpu_id));
 
-	gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val);
+	gpu->funcs->get_param(gpu, NULL, MSM_PARAM_CHIP_ID, &val, &zero);
 	rd_write_section(rd, RD_CHIP_ID, &val, sizeof(val));
 
 out:
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 07efc8033492..0aa1a8cb4e0d 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -95,6 +95,8 @@ struct drm_msm_param {
 	__u32 pipe;           /* in, MSM_PIPE_x */
 	__u32 param;          /* in, MSM_PARAM_x */
 	__u64 value;          /* out (get_param) or in (set_param) */
+	__u32 len;            /* zero for non-pointer params */
+	__u32 pad;            /* must be zero */
 };
 
 /*
-- 
2.35.1

[PATCH 2/3] drm/msm: Split out helper to get comm/cmdline

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Deduplicate this from fault_worker and recover_worker.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_gpu.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 8fe4aee96aa9..4ec62b601adc 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -362,6 +362,20 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
 
 static void retire_submits(struct msm_gpu *gpu);
 
+static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
+{
+	struct task_struct *task;
+
+	task = get_pid_task(submit->pid, PIDTYPE_PID);
+	if (!task)
+		return;
+
+	*comm = kstrdup(task->comm, GFP_KERNEL);
+	*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+
+	put_task_struct(task);
+}
+
 static void recover_worker(struct kthread_work *work)
 {
 	struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work);
@@ -378,18 +392,11 @@ static void recover_worker(struct kthread_work *work)
 
 	submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1);
 	if (submit) {
-		struct task_struct *task;
-
 		/* Increment the fault counts */
 		submit->queue->faults++;
 		submit->aspace->faults++;
 
-		task = get_pid_task(submit->pid, PIDTYPE_PID);
-		if (task) {
-			comm = kstrdup(task->comm, GFP_KERNEL);
-			cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
-			put_task_struct(task);
-		}
+		get_comm_cmdline(submit, &comm, &cmd);
 
 		if (comm && cmd) {
 			DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
@@ -478,14 +485,7 @@ static void fault_worker(struct kthread_work *work)
 		goto resume_smmu;
 
 	if (submit) {
-		struct task_struct *task;
-
-		task = get_pid_task(submit->pid, PIDTYPE_PID);
-		if (task) {
-			comm = kstrdup(task->comm, GFP_KERNEL);
-			cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
-			put_task_struct(task);
-		}
+		get_comm_cmdline(submit, &comm, &cmd);
 
 		/*
 		 * When we get GPU iova faults, we can get 1000s of them,
-- 
2.35.1

[PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

In the cause of using the GPU via virtgpu, the host side process is
really a sort of proxy, and not terribly interesting from the PoV of
crash/fault logging.  Add a way to override these per process so that
we can see the guest process's name.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 40 +++++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gpu.c           | 11 +++++--
 drivers/gpu/drm/msm/msm_gpu.h           |  6 ++++
 drivers/gpu/drm/msm/msm_submitqueue.c   |  2 ++
 include/uapi/drm/msm_drm.h              |  2 ++
 5 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 3d307b34854d..c68dc9c722c7 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -290,11 +290,45 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 		     uint32_t param, uint64_t value, uint32_t len)
 {
-	/* No pointer params yet */
-	if (len != 0)
-		return -EINVAL;
+	switch (param) {
+	case MSM_PARAM_COMM:
+	case MSM_PARAM_CMDLINE:
+		/* kstrdup_quotable_cmdline() limits to PAGE_SIZE, so
+		 * that should be a reasonable upper bound
+		 */
+		if (len > PAGE_SIZE)
+			return -EINVAL;
+		break;
+	default:
+		if (len != 0)
+			return -EINVAL;
+	}
 
 	switch (param) {
+	case MSM_PARAM_COMM:
+	case MSM_PARAM_CMDLINE: {
+		char *str, **paramp;
+
+		str = kmalloc(len + 1, GFP_KERNEL);
+		if (copy_from_user(str, u64_to_user_ptr(value), len)) {
+			kfree(str);
+			return -EFAULT;
+		}
+
+		/* Ensure string is null terminated: */
+		str[len] = '\0';
+
+		if (param == MSM_PARAM_COMM) {
+			paramp = &ctx->comm;
+		} else {
+			paramp = &ctx->cmdline;
+		}
+
+		kfree(*paramp);
+		*paramp = str;
+
+		return 0;
+	}
 	case MSM_PARAM_SYSPROF:
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 4ec62b601adc..68f3f8ade76d 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
 
 static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
 {
+	struct msm_file_private *ctx = submit->queue->ctx;
 	struct task_struct *task;
 
+	*comm = kstrdup(ctx->comm, GFP_KERNEL);
+	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
+
 	task = get_pid_task(submit->pid, PIDTYPE_PID);
 	if (!task)
 		return;
 
-	*comm = kstrdup(task->comm, GFP_KERNEL);
-	*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+	if (!*comm)
+		*comm = kstrdup(task->comm, GFP_KERNEL);
+
+	if (!*cmd)
+		*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
 
 	put_task_struct(task);
 }
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index c28c2ad9f52e..2c0203fd6ce3 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -355,6 +355,12 @@ struct msm_file_private {
 	 */
 	int sysprof;
 
+	/** comm: Overridden task comm, see MSM_PARAM_COMM */
+	char *comm;
+
+	/** cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE */
+	char *cmdline;
+
 	/**
 	 * elapsed:
 	 *
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 79b6ccd6ce64..f486a3cd4e55 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -61,6 +61,8 @@ void __msm_file_private_destroy(struct kref *kref)
 	}
 
 	msm_gem_address_space_put(ctx->aspace);
+	kfree(ctx->comm);
+	kfree(ctx->cmdline);
 	kfree(ctx);
 }
 
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 0aa1a8cb4e0d..794ad1948497 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -82,6 +82,8 @@ struct drm_msm_timespec {
 #define MSM_PARAM_FAULTS     0x09  /* RO */
 #define MSM_PARAM_SUSPENDS   0x0a  /* RO */
 #define MSM_PARAM_SYSPROF    0x0b  /* WO: 1 preserves perfcntrs, 2 also disables suspend */
+#define MSM_PARAM_COMM       0x0c  /* WO: override for task->comm */
+#define MSM_PARAM_CMDLINE    0x0d  /* WO: override for task cmdline */
 
 /* For backwards compat.  The original support for preemption was based on
  * a single ring per priority level so # of priority levels equals the #
-- 
2.35.1

Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Dan Carpenter]

On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
>  	switch (param) {
> +	case MSM_PARAM_COMM:
> +	case MSM_PARAM_CMDLINE: {
> +		char *str, **paramp;
> +
> +		str = kmalloc(len + 1, GFP_KERNEL);

if (!str)
	return -ENOMEM;

> +		if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> +			kfree(str);
> +			return -EFAULT;
> +		}
> +
> +		/* Ensure string is null terminated: */
> +		str[len] = '\0';
> +
> +		if (param == MSM_PARAM_COMM) {
> +			paramp = &ctx->comm;
> +		} else {
> +			paramp = &ctx->cmdline;
> +		}
> +
> +		kfree(*paramp);
> +		*paramp = str;
> +
> +		return 0;
> +	}
>  	case MSM_PARAM_SYSPROF:
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 4ec62b601adc..68f3f8ade76d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
>  
>  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
>  {
> +	struct msm_file_private *ctx = submit->queue->ctx;
>  	struct task_struct *task;
>  
> +	*comm = kstrdup(ctx->comm, GFP_KERNEL);
> +	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> +
>  	task = get_pid_task(submit->pid, PIDTYPE_PID);
>  	if (!task)
>  		return;
>  
> -	*comm = kstrdup(task->comm, GFP_KERNEL);
> -	*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> +	if (!*comm)
> +		*comm = kstrdup(task->comm, GFP_KERNEL);

What?

If the first allocation failed, then this one is going to fail as well.
Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
string?

> +
> +	if (!*cmd)
> +		*cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);

Same.

>  
>  	put_task_struct(task);
>  }

regards,
dan carpenter

Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Rob Clark]

On Thu, Mar 17, 2022 at 1:21 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
> >       switch (param) {
> > +     case MSM_PARAM_COMM:
> > +     case MSM_PARAM_CMDLINE: {
> > +             char *str, **paramp;
> > +
> > +             str = kmalloc(len + 1, GFP_KERNEL);
>
> if (!str)
>         return -ENOMEM;
>
> > +             if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> > +                     kfree(str);
> > +                     return -EFAULT;
> > +             }
> > +
> > +             /* Ensure string is null terminated: */
> > +             str[len] = '\0';
> > +
> > +             if (param == MSM_PARAM_COMM) {
> > +                     paramp = &ctx->comm;
> > +             } else {
> > +                     paramp = &ctx->cmdline;
> > +             }
> > +
> > +             kfree(*paramp);
> > +             *paramp = str;
> > +
> > +             return 0;
> > +     }
> >       case MSM_PARAM_SYSPROF:
> >               if (!capable(CAP_SYS_ADMIN))
> >                       return -EPERM;
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index 4ec62b601adc..68f3f8ade76d 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
> >
> >  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> >  {
> > +     struct msm_file_private *ctx = submit->queue->ctx;
> >       struct task_struct *task;
> >
> > +     *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > +     *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > +
> >       task = get_pid_task(submit->pid, PIDTYPE_PID);
> >       if (!task)
> >               return;
> >
> > -     *comm = kstrdup(task->comm, GFP_KERNEL);
> > -     *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> > +     if (!*comm)
> > +             *comm = kstrdup(task->comm, GFP_KERNEL);
>
> What?
>
> If the first allocation failed, then this one is going to fail as well.
> Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
> string?

fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this
isn't intended to deal with OoM, but the case that comm and/or cmdline
is not overridden.

BR,
-R

>
> > +
> > +     if (!*cmd)
> > +             *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
>
> Same.
>
> >
> >       put_task_struct(task);
> >  }
>
> regards,
> dan carpenter
>

Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Dan Carpenter]

On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote:
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > > index 4ec62b601adc..68f3f8ade76d 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
> > >
> > >  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> > >  {
> > > +     struct msm_file_private *ctx = submit->queue->ctx;
> > >       struct task_struct *task;
> > >
> > > +     *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > > +     *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > > +
> > >       task = get_pid_task(submit->pid, PIDTYPE_PID);
> > >       if (!task)
> > >               return;
> > >
> > > -     *comm = kstrdup(task->comm, GFP_KERNEL);
> > > -     *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> > > +     if (!*comm)
> > > +             *comm = kstrdup(task->comm, GFP_KERNEL);
> >
> > What?
> >
> > If the first allocation failed, then this one is going to fail as well.
> > Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
> > string?
> 
> fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this
> isn't intended to deal with OoM, but the case that comm and/or cmdline
> is not overridden.

Ah, I should have thought about that.  Thanks!

regards,
dan carpenter

Re: [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Rob Clark]

On Fri, Mar 18, 2022 at 12:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote:
> > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > > > index 4ec62b601adc..68f3f8ade76d 100644
> > > > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
> > > >
> > > >  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> > > >  {
> > > > +     struct msm_file_private *ctx = submit->queue->ctx;
> > > >       struct task_struct *task;
> > > >
> > > > +     *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > > > +     *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > > > +
> > > >       task = get_pid_task(submit->pid, PIDTYPE_PID);
> > > >       if (!task)
> > > >               return;
> > > >
> > > > -     *comm = kstrdup(task->comm, GFP_KERNEL);
> > > > -     *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> > > > +     if (!*comm)
> > > > +             *comm = kstrdup(task->comm, GFP_KERNEL);
> > >
> > > What?
> > >
> > > If the first allocation failed, then this one is going to fail as well.
> > > Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
> > > string?
> >
> > fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this
> > isn't intended to deal with OoM, but the case that comm and/or cmdline
> > is not overridden.
>
> Ah, I should have thought about that.  Thanks!

np, I realized it was fairly non-obvious so I added a comment in v2 to
hopefully make it more clear

BR,
-R

> regards,
> dan carpenter
>