[libcamera-devel] [PATCH v2] libcamera: v4l2_videodevice: Handle unexpected buffers

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Sep 09, 2021 at 04:45:53PM +0100, Kieran Bingham wrote:
> Sorry - this patch was already posted, so this is in fact v2.
> 
> On 09/09/2021 16:08, Kieran Bingham wrote:
> > A kernel bug can lead to unexpected buffers being dequeued where we
> > haven't entered the buffer in our queuedBuffers_ list.
> > 
> > This causes invalid accesses if not handled correctly within libcamera,
> > and while it is a kernel issue, we can protect against unpatched
> > kernels to provide a more suitable error message.
> > 
> > This is fixed in the kernel by c592b46907ad ("media: videobuf2-core:
> > dequeue if start_streaming fails") [0]

This is fixed in the kernel by commit c592b46907ad ("media:
videobuf2-core: dequeue if start_streaming fails") in the mainline
kernel [0]

> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c592b46907ad
> > 
> > Handle unexpected buffers by returning a nullptr, and move cache
> > management after the validation of the buffer.
> 
> Paul had already provided this:
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> 
> v2:
>  - Update commit message to reference the now upstream kernel fix.
>  - Fix spelling errors
> 
> >  src/libcamera/v4l2_videodevice.cpp | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 837a59d9bae2..7bb28aea357a 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1654,9 +1654,28 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >  
> >  	LOG(V4L2, Debug) << "Dequeuing buffer " << buf.index;
> >  
> > +	auto it = queuedBuffers_.find(buf.index);

I'd move this after the comment.

> > +	/*
> > +	 * If the video node fails to stream-on successfully (which can occur
> > +	 * when queuing a buffer), a vb2 kernel bug can lead to the buffer which
> > +	 * returns a failure upon queuing, being mistakenly kept in the kernel.

s/queuing,/queuing/

> > +	 * This leads to the kernel notifying us that a buffer is available to
> > +	 * dequeue, which we have no awareness of being queued, and thus we will
> > +	 * not find it in the queuedBuffers_ list.
> > +	 *
> > +	 * Whilst this is a kernel bug and should be fixed there, ensure that we
> > +	 * safely ignore buffers which are unexpected to prevent crashes on
> > +	 * unpatched kernels.

 * Whilst this kernel bug has been fixed in mainline, ensure that we safely
 * ignore buffers which are unexpected to prevent crashes on older kernels.

> > +	 */
> > +	if (it == queuedBuffers_.end()) {
> > +		LOG(V4L2, Error)
> > +			<< "Dequeued an unexpected buffer: " << buf.index;

I'd write

			<< "Dequeued unexpected buffer " << buf.index;

or

			<< "Dequeued unexpected buffer index " << buf.index;

Otherwise we'll print "Dequeued an unexpected buffer: 3" and it won't be
clear if 3 identifies a buffer or is an error code.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +
> > +		return nullptr;
> > +	}
> > +
> >  	cache_->put(buf.index);
> >  
> > -	auto it = queuedBuffers_.find(buf.index);
> >  	FrameBuffer *buffer = it->second;
> >  	queuedBuffers_.erase(it);
> >  
> > 

-- 
Regards,

Laurent Pinchart