Hello Rob Clark,
Commit 68dc6c2d5eec ("drm/msm: Fix submit error-path leaks") from May
9, 2023 (linux-next), leads to the following Smatch static checker
warning:
drivers/gpu/drm/msm/msm_gem_submit.c:816 msm_ioctl_gem_submit() warn: fd used
after fd_install() 'out_fence_fd'
drivers/gpu/drm/msm/msm_gem_submit.c:818 msm_ioctl_gem_submit() warn: fd used
after fd_install() 'sync_file->file'
drivers/gpu/drm/msm/msm_gem_submit.c
751 WARN_ON(ret);
752 } else {
753 /*
754 * Allocate an id which can be used by WAIT_FENCE ioctl
to map
755 * back to the underlying fence.
756 */
757 submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
758 submit->user_fence,
1,
759 INT_MAX,
GFP_NOWAIT);
760 }
761
762 spin_unlock(&queue->idr_lock);
763 idr_preload_end();
764
765 if (submit->fence_id < 0) {
766 ret = submit->fence_id;
767 submit->fence_id = 0;
768 }
769
770 if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
771 sync_file = sync_file_create(submit->user_fence);
772 if (!sync_file) {
773 ret = -ENOMEM;
774 } else {
775 fd_install(out_fence_fd, sync_file->file);
^^^^^^^^^^^^
Once we call fd_install() the file is exposed to userspace and they can make
the fd point to a different file.
776 args->fence_fd = out_fence_fd;
777 }
778 }
779
780 if (ret)
781 goto out;
782
783 submit_attach_object_fences(submit);
784
785 if (msm_context_is_vmbind(ctx)) {
786 /*
787 * If we are not using VM_BIND, submit_pin_vmas() will
validate
788 * just the BOs attached to the submit. In that case
we don't
789 * need to validate the _entire_ vm, because userspace
tracked
790 * what BOs are associated with the submit.
791 */
792 ret = drm_gpuvm_validate(submit->vm, &submit->exec);
793 if (ret)
794 goto out;
795 }
796
797 /* The scheduler owns a ref now: */
798 msm_gem_submit_get(submit);
799
800 msm_rd_dump_submit(priv->rd, submit, NULL);
801
802 drm_sched_entity_push_job(&submit->base);
803
804 args->fence = submit->fence_id;
805 queue->last_fence = submit->fence_id;
806
807 msm_syncobj_reset(syncobjs_to_reset, args->nr_in_syncobjs);
808 msm_syncobj_process_post_deps(post_deps, args->nr_out_syncobjs,
submit->user_fence);
809
810 out:
811 submit_cleanup(submit, !!ret);
812 out_unlock:
813 mutex_unlock(&queue->lock);
814 out_post_unlock:
815 if (ret && (out_fence_fd >= 0)) {
--> 816 put_unused_fd(out_fence_fd);
^^^^^^^^^^^^^^^^^^^^^^^^^^^
So this put_unused_fd() could potentially do something to the wrong file.
Traditionally, we either do the fd_install() last or we just leak until the
process dies and all the files are released.
(Hand wavey because I'm not sure how all this works exactly)
817 if (sync_file)
818 fput(sync_file->file);
819 }
820
821 if (!IS_ERR_OR_NULL(submit)) {
822 msm_gem_submit_put(submit);
823 } else {
824 /*
825 * If the submit hasn't yet taken ownership of the queue
826 * then we need to drop the reference ourself:
827 */
828 msm_submitqueue_put(queue);
829 }
830 if (!IS_ERR_OR_NULL(post_deps)) {
831 for (i = 0; i < args->nr_out_syncobjs; ++i) {
832 kfree(post_deps[i].chain);
833 drm_syncobj_put(post_deps[i].syncobj);
834 }
835 kfree(post_deps);
836 }
837
838 if (!IS_ERR_OR_NULL(syncobjs_to_reset)) {
839 for (i = 0; i < args->nr_in_syncobjs; ++i) {
840 if (syncobjs_to_reset[i])
841 drm_syncobj_put(syncobjs_to_reset[i]);
842 }
843 kfree(syncobjs_to_reset);
844 }
845
846 return ret;
847 }
regards,
dan carpenter
