| Message ID | 20210909150803.4014957-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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] > > [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@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@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); > + /* > + * 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. > + * 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. > + */ > + if (it == queuedBuffers_.end()) { > + LOG(V4L2, Error) > + << "Dequeued an unexpected buffer: " << buf.index; > + > + return nullptr; > + } > + > cache_->put(buf.index); > > - auto it = queuedBuffers_.find(buf.index); > FrameBuffer *buffer = it->second; > queuedBuffers_.erase(it); > >
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@ideasonboard.com> > > > Signed-off-by: Kieran Bingham <kieran.bingham@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@ideasonboard.com> > > + > > + return nullptr; > > + } > > + > > cache_->put(buf.index); > > > > - auto it = queuedBuffers_.find(buf.index); > > FrameBuffer *buffer = it->second; > > queuedBuffers_.erase(it); > > > >
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); + /* + * 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. + * 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. + */ + if (it == queuedBuffers_.end()) { + LOG(V4L2, Error) + << "Dequeued an unexpected buffer: " << buf.index; + + return nullptr; + } + cache_->put(buf.index); - auto it = queuedBuffers_.find(buf.index); FrameBuffer *buffer = it->second; queuedBuffers_.erase(it);
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] [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. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)