diff mbox series

[v3,07/23] drm/msm/submit: Move copy_from_user ahead of locking bos

Message ID 20201019204636.139997-8-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/msm: de-struct_mutex-ification | expand

Commit Message

Rob Clark Oct. 19, 2020, 8:46 p.m. UTC 
From: Rob Clark <robdclark@chromium.org>

We cannot switch to using obj->resv for locking without first moving all
the copy_from_user() ahead of submit_lock_objects().  Otherwise in the
mm fault path we aquire mm->mmap_sem before obj lock, but in the submit
path the order is reversed.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_gem.h        |   3 +
 drivers/gpu/drm/msm/msm_gem_submit.c | 121 ++++++++++++++++-----------
 2 files changed, 76 insertions(+), 48 deletions(-)

Comments

Kristian Høgsberg Oct. 23, 2020, 9:08 a.m. UTC | #1 
On Mon, Oct 19, 2020 at 10:45 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> We cannot switch to using obj->resv for locking without first moving all
> the copy_from_user() ahead of submit_lock_objects().  Otherwise in the
> mm fault path we aquire mm->mmap_sem before obj lock, but in the submit
> path the order is reversed.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_gem.h        |   3 +
>  drivers/gpu/drm/msm/msm_gem_submit.c | 121 ++++++++++++++++-----------
>  2 files changed, 76 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index c5232b8da794..0b7dda312992 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -240,7 +240,10 @@ struct msm_gem_submit {
>                 uint32_t type;
>                 uint32_t size;  /* in dwords */
>                 uint64_t iova;
> +               uint32_t offset;/* in dwords */
>                 uint32_t idx;   /* cmdstream buffer idx in bos[] */
> +               uint32_t nr_relocs;
> +               struct drm_msm_gem_submit_reloc *relocs;
>         } *cmd;  /* array of size nr_cmds */
>         struct {
>                 uint32_t flags;
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index aa5c60a7132d..002130d826aa 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -62,11 +62,16 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>
>  void msm_gem_submit_free(struct msm_gem_submit *submit)
>  {
> +       unsigned i;
> +
>         dma_fence_put(submit->fence);
>         list_del(&submit->node);
>         put_pid(submit->pid);
>         msm_submitqueue_put(submit->queue);
>
> +       for (i = 0; i < submit->nr_cmds; i++)
> +               kfree(submit->cmd[i].relocs);
> +
>         kfree(submit);
>  }
>
> @@ -150,6 +155,60 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
>         return ret;
>  }
>
> +static int submit_lookup_cmds(struct msm_gem_submit *submit,
> +               struct drm_msm_gem_submit *args, struct drm_file *file)
> +{
> +       unsigned i, sz;
> +       int ret = 0;
> +
> +       for (i = 0; i < args->nr_cmds; i++) {
> +               struct drm_msm_gem_submit_cmd submit_cmd;
> +               void __user *userptr =
> +                       u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
> +
> +               ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd));
> +               if (ret) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +
> +               /* validate input from userspace: */
> +               switch (submit_cmd.type) {
> +               case MSM_SUBMIT_CMD_BUF:
> +               case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> +               case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> +                       break;
> +               default:
> +                       DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
> +                       return -EINVAL;
> +               }
> +
> +               if (submit_cmd.size % 4) {
> +                       DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
> +                                       submit_cmd.size);
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               submit->cmd[i].type = submit_cmd.type;
> +               submit->cmd[i].size = submit_cmd.size / 4;
> +               submit->cmd[i].offset = submit_cmd.submit_offset / 4;
> +               submit->cmd[i].idx  = submit_cmd.submit_idx;
> +               submit->cmd[i].nr_relocs = submit_cmd.nr_relocs;
> +
> +               sz = sizeof(struct drm_msm_gem_submit_reloc) * submit_cmd.nr_relocs;
> +               submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL);

kmalloc_array() or check_mul_overflow() here for the integer overflow check.

> +               ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
> +               if (ret) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +       }
> +
> +out:
> +       return ret;
> +}
> +
>  static void submit_unlock_unpin_bo(struct msm_gem_submit *submit,
>                 int i, bool backoff)
>  {
> @@ -301,7 +360,7 @@ static int submit_bo(struct msm_gem_submit *submit, uint32_t idx,
>
>  /* process the reloc's and patch up the cmdstream as needed: */
>  static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *obj,
> -               uint32_t offset, uint32_t nr_relocs, uint64_t relocs)
> +               uint32_t offset, uint32_t nr_relocs, struct drm_msm_gem_submit_reloc *relocs)
>  {
>         uint32_t i, last_offset = 0;
>         uint32_t *ptr;
> @@ -327,18 +386,11 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
>         }
>
>         for (i = 0; i < nr_relocs; i++) {
> -               struct drm_msm_gem_submit_reloc submit_reloc;
> -               void __user *userptr =
> -                       u64_to_user_ptr(relocs + (i * sizeof(submit_reloc)));
> +               struct drm_msm_gem_submit_reloc submit_reloc = relocs[i];
>                 uint32_t off;
>                 uint64_t iova;
>                 bool valid;
>
> -               if (copy_from_user(&submit_reloc, userptr, sizeof(submit_reloc))) {
> -                       ret = -EFAULT;
> -                       goto out;
> -               }
> -
>                 if (submit_reloc.submit_offset % 4) {
>                         DRM_ERROR("non-aligned reloc offset: %u\n",
>                                         submit_reloc.submit_offset);
> @@ -694,6 +746,10 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>         if (ret)
>                 goto out;
>
> +       ret = submit_lookup_cmds(submit, args, file);
> +       if (ret)
> +               goto out;
> +
>         /* copy_*_user while holding a ww ticket upsets lockdep */
>         ww_acquire_init(&submit->ticket, &reservation_ww_class);
>         has_ww_ticket = true;
> @@ -710,60 +766,29 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>                 goto out;
>
>         for (i = 0; i < args->nr_cmds; i++) {
> -               struct drm_msm_gem_submit_cmd submit_cmd;
> -               void __user *userptr =
> -                       u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
>                 struct msm_gem_object *msm_obj;
>                 uint64_t iova;
>
> -               ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd));
> -               if (ret) {
> -                       ret = -EFAULT;
> -                       goto out;
> -               }
> -
> -               /* validate input from userspace: */
> -               switch (submit_cmd.type) {
> -               case MSM_SUBMIT_CMD_BUF:
> -               case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> -               case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> -                       break;
> -               default:
> -                       DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
> -                       ret = -EINVAL;
> -                       goto out;
> -               }
> -
> -               ret = submit_bo(submit, submit_cmd.submit_idx,
> +               ret = submit_bo(submit, submit->cmd[i].idx,
>                                 &msm_obj, &iova, NULL);
>                 if (ret)
>                         goto out;
>
> -               if (submit_cmd.size % 4) {
> -                       DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
> -                                       submit_cmd.size);
> +               if (!submit->cmd[i].size ||
> +                       ((submit->cmd[i].size + submit->cmd[i].offset) >
> +                               msm_obj->base.size / 4)) {
> +                       DRM_ERROR("invalid cmdstream size: %u\n", submit->cmd[i].size * 4);
>                         ret = -EINVAL;
>                         goto out;
>                 }
>
> -               if (!submit_cmd.size ||
> -                       ((submit_cmd.size + submit_cmd.submit_offset) >
> -                               msm_obj->base.size)) {
> -                       DRM_ERROR("invalid cmdstream size: %u\n", submit_cmd.size);
> -                       ret = -EINVAL;
> -                       goto out;
> -               }
> -
> -               submit->cmd[i].type = submit_cmd.type;
> -               submit->cmd[i].size = submit_cmd.size / 4;
> -               submit->cmd[i].iova = iova + submit_cmd.submit_offset;
> -               submit->cmd[i].idx  = submit_cmd.submit_idx;
> +               submit->cmd[i].iova = iova + (submit->cmd[i].offset * 4);
>
>                 if (submit->valid)
>                         continue;
>
> -               ret = submit_reloc(submit, msm_obj, submit_cmd.submit_offset,
> -                               submit_cmd.nr_relocs, submit_cmd.relocs);
> +               ret = submit_reloc(submit, msm_obj, submit->cmd[i].offset * 4,
> +                               submit->cmd[i].nr_relocs, submit->cmd[i].relocs);
>                 if (ret)
>                         goto out;
>         }
> --
> 2.26.2
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index c5232b8da794..0b7dda312992 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -240,7 +240,10 @@  struct msm_gem_submit {
 		uint32_t type;
 		uint32_t size;  /* in dwords */
 		uint64_t iova;
+		uint32_t offset;/* in dwords */
 		uint32_t idx;   /* cmdstream buffer idx in bos[] */
+		uint32_t nr_relocs;
+		struct drm_msm_gem_submit_reloc *relocs;
 	} *cmd;  /* array of size nr_cmds */
 	struct {
 		uint32_t flags;
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index aa5c60a7132d..002130d826aa 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -62,11 +62,16 @@  static struct msm_gem_submit *submit_create(struct drm_device *dev,
 
 void msm_gem_submit_free(struct msm_gem_submit *submit)
 {
+	unsigned i;
+
 	dma_fence_put(submit->fence);
 	list_del(&submit->node);
 	put_pid(submit->pid);
 	msm_submitqueue_put(submit->queue);
 
+	for (i = 0; i < submit->nr_cmds; i++)
+		kfree(submit->cmd[i].relocs);
+
 	kfree(submit);
 }
 
@@ -150,6 +155,60 @@  static int submit_lookup_objects(struct msm_gem_submit *submit,
 	return ret;
 }
 
+static int submit_lookup_cmds(struct msm_gem_submit *submit,
+		struct drm_msm_gem_submit *args, struct drm_file *file)
+{
+	unsigned i, sz;
+	int ret = 0;
+
+	for (i = 0; i < args->nr_cmds; i++) {
+		struct drm_msm_gem_submit_cmd submit_cmd;
+		void __user *userptr =
+			u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
+
+		ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		/* validate input from userspace: */
+		switch (submit_cmd.type) {
+		case MSM_SUBMIT_CMD_BUF:
+		case MSM_SUBMIT_CMD_IB_TARGET_BUF:
+		case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
+			break;
+		default:
+			DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
+			return -EINVAL;
+		}
+
+		if (submit_cmd.size % 4) {
+			DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
+					submit_cmd.size);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		submit->cmd[i].type = submit_cmd.type;
+		submit->cmd[i].size = submit_cmd.size / 4;
+		submit->cmd[i].offset = submit_cmd.submit_offset / 4;
+		submit->cmd[i].idx  = submit_cmd.submit_idx;
+		submit->cmd[i].nr_relocs = submit_cmd.nr_relocs;
+
+		sz = sizeof(struct drm_msm_gem_submit_reloc) * submit_cmd.nr_relocs;
+		submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL);
+		ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+	}
+
+out:
+	return ret;
+}
+
 static void submit_unlock_unpin_bo(struct msm_gem_submit *submit,
 		int i, bool backoff)
 {
@@ -301,7 +360,7 @@  static int submit_bo(struct msm_gem_submit *submit, uint32_t idx,
 
 /* process the reloc's and patch up the cmdstream as needed: */
 static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *obj,
-		uint32_t offset, uint32_t nr_relocs, uint64_t relocs)
+		uint32_t offset, uint32_t nr_relocs, struct drm_msm_gem_submit_reloc *relocs)
 {
 	uint32_t i, last_offset = 0;
 	uint32_t *ptr;
@@ -327,18 +386,11 @@  static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
 	}
 
 	for (i = 0; i < nr_relocs; i++) {
-		struct drm_msm_gem_submit_reloc submit_reloc;
-		void __user *userptr =
-			u64_to_user_ptr(relocs + (i * sizeof(submit_reloc)));
+		struct drm_msm_gem_submit_reloc submit_reloc = relocs[i];
 		uint32_t off;
 		uint64_t iova;
 		bool valid;
 
-		if (copy_from_user(&submit_reloc, userptr, sizeof(submit_reloc))) {
-			ret = -EFAULT;
-			goto out;
-		}
-
 		if (submit_reloc.submit_offset % 4) {
 			DRM_ERROR("non-aligned reloc offset: %u\n",
 					submit_reloc.submit_offset);
@@ -694,6 +746,10 @@  int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto out;
 
+	ret = submit_lookup_cmds(submit, args, file);
+	if (ret)
+		goto out;
+
 	/* copy_*_user while holding a ww ticket upsets lockdep */
 	ww_acquire_init(&submit->ticket, &reservation_ww_class);
 	has_ww_ticket = true;
@@ -710,60 +766,29 @@  int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto out;
 
 	for (i = 0; i < args->nr_cmds; i++) {
-		struct drm_msm_gem_submit_cmd submit_cmd;
-		void __user *userptr =
-			u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
 		struct msm_gem_object *msm_obj;
 		uint64_t iova;
 
-		ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd));
-		if (ret) {
-			ret = -EFAULT;
-			goto out;
-		}
-
-		/* validate input from userspace: */
-		switch (submit_cmd.type) {
-		case MSM_SUBMIT_CMD_BUF:
-		case MSM_SUBMIT_CMD_IB_TARGET_BUF:
-		case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
-			break;
-		default:
-			DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
-			ret = -EINVAL;
-			goto out;
-		}
-
-		ret = submit_bo(submit, submit_cmd.submit_idx,
+		ret = submit_bo(submit, submit->cmd[i].idx,
 				&msm_obj, &iova, NULL);
 		if (ret)
 			goto out;
 
-		if (submit_cmd.size % 4) {
-			DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
-					submit_cmd.size);
+		if (!submit->cmd[i].size ||
+			((submit->cmd[i].size + submit->cmd[i].offset) >
+				msm_obj->base.size / 4)) {
+			DRM_ERROR("invalid cmdstream size: %u\n", submit->cmd[i].size * 4);
 			ret = -EINVAL;
 			goto out;
 		}
 
-		if (!submit_cmd.size ||
-			((submit_cmd.size + submit_cmd.submit_offset) >
-				msm_obj->base.size)) {
-			DRM_ERROR("invalid cmdstream size: %u\n", submit_cmd.size);
-			ret = -EINVAL;
-			goto out;
-		}
-
-		submit->cmd[i].type = submit_cmd.type;
-		submit->cmd[i].size = submit_cmd.size / 4;
-		submit->cmd[i].iova = iova + submit_cmd.submit_offset;
-		submit->cmd[i].idx  = submit_cmd.submit_idx;
+		submit->cmd[i].iova = iova + (submit->cmd[i].offset * 4);
 
 		if (submit->valid)
 			continue;
 
-		ret = submit_reloc(submit, msm_obj, submit_cmd.submit_offset,
-				submit_cmd.nr_relocs, submit_cmd.relocs);
+		ret = submit_reloc(submit, msm_obj, submit->cmd[i].offset * 4,
+				submit->cmd[i].nr_relocs, submit->cmd[i].relocs);
 		if (ret)
 			goto out;
 	}