| Message ID | 20201019204636.139997-8-robdclark@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm: de-struct_mutex-ification | expand |
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 --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; }