linux-kernel - Re: [RFC PATCH] media: docs-rst: Document m2m stateless video decoder interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3cd8e2f0-d9c1-6907-d16b-d3c6699c9ef8@xs4all.nl>
Date:   Mon, 8 Oct 2018 12:07:07 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Alexandre Courbot <acourbot@...omium.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Pawel Osciak <posciak@...omium.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] media: docs-rst: Document m2m stateless video decoder
 interface

On 10/03/2018 12:10 PM, Tomasz Figa wrote:
> On Tue, Sep 11, 2018 at 5:48 PM Hans Verkuil <hverkuil@...all.nl> wrote:
>>
>> On 09/11/18 10:40, Alexandre Courbot wrote:
>>>> I am not sure whether this should be documented, but there are some additional
>>>> restrictions w.r.t. reference frames:
>>>>
>>>> Since decoders need access to the decoded reference frames there are some corner
>>>> cases that need to be checked:
>>>>
>>>> 1) V4L2_MEMORY_USERPTR cannot be used for the capture queue: the driver does not
>>>>    know when a malloced but dequeued buffer is freed, so the reference frame
>>>>    could suddenly be gone.
>>>
>>> It it is confirmed that we cannot use USERPTR buffers as CAPTURE then
>>> this probably needs to be documented. I wonder if there isn't a way to
>>> avoid this by having vb2 keep a reference to the pages in such a way
>>> that they would not be recycled after after userspace calls free() on
>>> the buffer. Is that possible with user-allocated memory?
>>
>> vb2 keeps a reference while the buffer is queued, but that reference is
>> released once the buffer is dequeued. Correctly, IMHO. If you provide
>> USERPTR, than userspace is responsible for the memory. Changing this
>> would require changing the API, since USERPTR has always worked like
>> this.
> 
> That would be a userspace bug wouldn't it? The next try to get user
> pages would fail in that case and we could just fail such decode
> request, couldn't we?
> 
> (Personally I'm not a big fan of USERPTR, though.)

Yes, just don't support USERPTR for drivers like this. USERPTR just
doesn't make sense.

> 
>>
>>>
>>> Not that I think that forbidding USERPTR buffers in this scenario
>>> would be a big problem.
>>
>> I think it is perfectly OK to forbid this. Ideally I would like to have
>> some test in v4l2-compliance or (even better) v4l2-mem2mem.c for this,
>> but it is actually not that easy to identify drivers like this.
>>
>> Suggestions for this on a postcard...
>>
>>>
>>>>
>>>> 2) V4L2_MEMORY_DMABUF can be used, but drivers should check that the dma buffer is
>>>>    still available AND increase the dmabuf refcount while it is used by the HW.
>>>
>>> Yeah, with DMABUF the above problem can easily be avoided at least.
>>>
>>>>
>>>> 3) What to do if userspace has requeued a buffer containing a reference frame,
>>>>    and you want to decode a B/P-frame that refers to that buffer? We need to
>>>>    check against that: I think that when you queue a capture buffer whose index
>>>>    is used in a pending request as a reference frame, than that should fail with
>>>>    an error.

Perhaps an error is overkill, but I think a warning should be issued.

 And trying to queue a request referring to a buffer that has been
 requeued should also fail.

I don't think this is right, this is likely a valid case.

Regards,

	Hans

>>>>
>>>> We might need to add some support for this in v4l2-mem2mem.c or vb2.
>>>
>>> Sounds good, and we should document this as well.
>>>
>>
>> Right. And test it!
> 
> I'm not convinced that we should be enforcing this. Moreover,
> requeuing a buffer containing a reference frame for a pending request
> is not bound to be an error. It might be a legit case when the same
> entry in the reference list is replaced with a different key frame
> decoded into the same buffer as the reference list entry pointed until
> now.
> 
> Best regards,
> Tomasz
>