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

From: Kristian Høgsberg
Date: Fri Oct 23 2020 - 05:09:01 EST

Next message: Hector Yuan: "Re: [PATCH v1] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver"
Previous message: syzbot: "possible deadlock in send_sigurg (2)"
In reply to: Rob Clark: "[PATCH v3 07/23] drm/msm/submit: Move copy_from_user ahead of locking bos"
Next in thread: Rob Clark: "[PATCH v3 09/23] drm/msm/gem: Switch over to obj->resv for locking"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 19, 2020 at 10:45 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Next message: Hector Yuan: "Re: [PATCH v1] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver"
Previous message: syzbot: "possible deadlock in send_sigurg (2)"
In reply to: Rob Clark: "[PATCH v3 07/23] drm/msm/submit: Move copy_from_user ahead of locking bos"
Next in thread: Rob Clark: "[PATCH v3 09/23] drm/msm/gem: Switch over to obj->resv for locking"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]