Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

[Alexandre Courbot <acourbot@chromium.org>]

On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>  /*
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:         videobuf2 queue
> @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>         if (vb->in_fence) {
>                 ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
>                                              vb2_qbuf_fence_cb);
> -               if (ret == -EINVAL) {
> +               /* is the fence signaled? */
> +               if (ret == -ENOENT) {
> +                       dma_fence_put(vb->in_fence);
> +                       vb->in_fence = NULL;
> +               } else if (ret) {
>                         spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
>                         goto err;
> -               } else if (!ret) {
> -                       goto fill;
>                 }
> -
> -               dma_fence_put(vb->in_fence);
> -               vb->in_fence = NULL;

This chunk seems to deal with input fences, shouldn't it be part of
the previous patch instead of this one?

>
> -       if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> -           !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +       if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
>                 dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
>                 return -EINVAL;
>         }
>
> +       if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +               dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> +               return -EINVAL;
> +       }
> +

Same here?

>         return __verify_planes_array(q->bufs[b->index], b);
>  }
>
> @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>         b->sequence = vbuf->sequence;
>         b->reserved = 0;
>
> -       b->fence_fd = 0;
> +       if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +               b->fence_fd = vb->out_fence_fd;
> +       } else {
> +               b->fence_fd = 0;
> +       }

Sorry if this has already been discussed, but I don't remember the
outcome if it has.

I wonder if doing this here could not make out_fence_fd leak in
situations where we don't need/want it to. Let's take for instance a
multi-process user program. One process queues a buffer with an
OUT_FENCE and gets a valid fd in fence_fd upon return. Then the other
process performs a QUERYBUF and gets the same fence_fd - which is
invalid in its context. Would it not be preferable fill the out fence
information only when queuing buffers, since it is the only time where
we are guaranteed it will be usable by the caller?

Similarly, when a buffer is processed and user-space performs a DQBUF,
the V4L2_BUF_FLAG_OUT_FENCE will be set but fence_fd will be 0. Again,
limiting the return of out fence information to QBUF would prevent
this.

If we go that route, out_fence_fd could maybe become a local variable
of vb2_qbuf() instead of being a member of vb2_buffer, and would be
returned by vb2_setup_out_fence(). This would guarantee it does not
leak anywhere else.