[{"id":19585,"web_url":"https://patchwork.libcamera.org/comment/19585/","msgid":"<743efd34-950b-fb62-a1f5-e170df6bee64@ideasonboard.com>","date":"2021-09-09T15:45:53","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2_videodevice:\n\tHandle unexpected buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Sorry - this patch was already posted, so this is in fact v2.\n\nOn 09/09/2021 16:08, Kieran Bingham wrote:\n> A kernel bug can lead to unexpected buffers being dequeued where we\n> haven't entered the buffer in our queuedBuffers_ list.\n> \n> This causes invalid accesses if not handled correctly within libcamera,\n> and while it is a kernel issue, we can protect against unpatched\n> kernels to provide a more suitable error message.\n> \n> This is fixed in the kernel by c592b46907ad (\"media: videobuf2-core:\n> dequeue if start_streaming fails\") [0]\n> \n> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c592b46907ad\n> \n> Handle unexpected buffers by returning a nullptr, and move cache\n> management after the validation of the buffer.\n> \n\nPaul had already provided this:\n\nReviewed-by: Paul Elder \n\n> Signed-off-by: Kieran Bingham \n> ---\n\nv2:\n - Update commit message to reference the now upstream kernel fix.\n - Fix spelling errors\n\n\n> src/libcamera/v4l2_videodevice.cpp | 21 ++++++++++++++++++++-\n> 1 file changed, 20 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 837a59d9bae2..7bb28aea357a 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1654,9 +1654,28 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \n> \tLOG(V4L2, Debug) << \"Dequeuing buffer \" << buf.index;\n> \n> +\tauto it = queuedBuffers_.find(buf.index);\n> +\t/*\n> +\t * If the video node fails to stream-on successfully (which can occur\n> +\t * when queuing a buffer), a vb2 kernel bug can lead to the buffer which\n> +\t * returns a failure upon queuing, being mistakenly kept in the kernel.\n> +\t * This leads to the kernel notifying us that a buffer is available to\n> +\t * dequeue, which we have no awareness of being queued, and thus we will\n> +\t * not find it in the queuedBuffers_ list.\n> +\t *\n> +\t * Whilst this is a kernel bug and should be fixed there, ensure that we\n> +\t * safely ignore buffers which are unexpected to prevent crashes on\n> +\t * unpatched kernels.\n> +\t */\n> +\tif (it == queuedBuffers_.end()) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Dequeued an unexpected buffer: \" << buf.index;\n> +\n> +\t\treturn nullptr;\n> +\t}\n> +\n> \tcache_->put(buf.index);\n> \n> -\tauto it = queuedBuffers_.find(buf.index);\n> \tFrameBuffer *buffer = it->second;\n> \tqueuedBuffers_.erase(it);\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05108BDC71\n\tfor ;\n\tThu, 9 Sep 2021 15:45:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F3896916E;\n\tThu, 9 Sep 2021 17:45:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53A936916B\n\tfor ;\n\tThu, 9 Sep 2021 17:45:57 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12FE8883;\n\tThu, 9 Sep 2021 17:45:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Y1I0Y+kS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631202357;\n\tbh=VJ6i7Q/bF+FbKmqCVj2kdUN7Zy6TBD5p5B35pYLKQdw=;\n\th=From:Subject:To:References:Date:In-Reply-To:From;\n\tb=Y1I0Y+kSKNUEi3Hyw1TfZzfAcFPhm+FnhZYth2iihUo5rOP/NZblZ2LS+2kKhxbqm\n\tjVUmVAs2jwSQU43/bwyWqyPf2oMSxoWUnq/ad3KFYq03lAswo4Y2LH4QAMph6ODNX0\n\tKQ2MkqVbHr9vVQLOlWGoUin+smBTenmty+nMGjrs=","From":"Kieran Bingham ","To":"Kieran Bingham ,\n\tLibCamera Devel ","References":"<20210909150803.4014957-1-kieran.bingham@ideasonboard.com>","Message-ID":"<743efd34-950b-fb62-a1f5-e170df6bee64@ideasonboard.com>","Date":"Thu, 9 Sep 2021 16:45:53 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210909150803.4014957-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2_videodevice:\n\tHandle unexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19586,"web_url":"https://patchwork.libcamera.org/comment/19586/","msgid":"","date":"2021-09-09T16:08:44","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2_videodevice:\n\tHandle unexpected buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Sep 09, 2021 at 04:45:53PM +0100, Kieran Bingham wrote:\n> Sorry - this patch was already posted, so this is in fact v2.\n> \n> On 09/09/2021 16:08, Kieran Bingham wrote:\n> > A kernel bug can lead to unexpected buffers being dequeued where we\n> > haven't entered the buffer in our queuedBuffers_ list.\n> > \n> > This causes invalid accesses if not handled correctly within libcamera,\n> > and while it is a kernel issue, we can protect against unpatched\n> > kernels to provide a more suitable error message.\n> > \n> > This is fixed in the kernel by c592b46907ad (\"media: videobuf2-core:\n> > dequeue if start_streaming fails\") [0]\n\nThis is fixed in the kernel by commit c592b46907ad (\"media:\nvideobuf2-core: dequeue if start_streaming fails\") in the mainline\nkernel [0]\n\n> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c592b46907ad\n> > \n> > Handle unexpected buffers by returning a nullptr, and move cache\n> > management after the validation of the buffer.\n> \n> Paul had already provided this:\n> \n> Reviewed-by: Paul Elder \n> \n> > Signed-off-by: Kieran Bingham \n> > ---\n> \n> v2:\n> - Update commit message to reference the now upstream kernel fix.\n> - Fix spelling errors\n> \n> > src/libcamera/v4l2_videodevice.cpp | 21 ++++++++++++++++++++-\n> > 1 file changed, 20 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 837a59d9bae2..7bb28aea357a 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1654,9 +1654,28 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > \n> > \tLOG(V4L2, Debug) << \"Dequeuing buffer \" << buf.index;\n> > \n> > +\tauto it = queuedBuffers_.find(buf.index);\n\nI'd move this after the comment.\n\n> > +\t/*\n> > +\t * If the video node fails to stream-on successfully (which can occur\n> > +\t * when queuing a buffer), a vb2 kernel bug can lead to the buffer which\n> > +\t * returns a failure upon queuing, being mistakenly kept in the kernel.\n\ns/queuing,/queuing/\n\n> > +\t * This leads to the kernel notifying us that a buffer is available to\n> > +\t * dequeue, which we have no awareness of being queued, and thus we will\n> > +\t * not find it in the queuedBuffers_ list.\n> > +\t *\n> > +\t * Whilst this is a kernel bug and should be fixed there, ensure that we\n> > +\t * safely ignore buffers which are unexpected to prevent crashes on\n> > +\t * unpatched kernels.\n\n * Whilst this kernel bug has been fixed in mainline, ensure that we safely\n * ignore buffers which are unexpected to prevent crashes on older kernels.\n\n> > +\t */\n> > +\tif (it == queuedBuffers_.end()) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Dequeued an unexpected buffer: \" << buf.index;\n\nI'd write\n\n\t\t\t<< \"Dequeued unexpected buffer \" << buf.index;\n\nor\n\n\t\t\t<< \"Dequeued unexpected buffer index \" << buf.index;\n\nOtherwise we'll print \"Dequeued an unexpected buffer: 3\" and it won't be\nclear if 3 identifies a buffer or is an error code.\n\nReviewed-by: Laurent Pinchart \n\n> > +\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > \tcache_->put(buf.index);\n> > \n> > -\tauto it = queuedBuffers_.find(buf.index);\n> > \tFrameBuffer *buffer = it->second;\n> > \tqueuedBuffers_.erase(it);\n> > \n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B25CBDC71\n\tfor ;\n\tThu, 9 Sep 2021 16:09:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C720369170;\n\tThu, 9 Sep 2021 18:09:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1614C6916B\n\tfor ;\n\tThu, 9 Sep 2021 18:09:05 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C284883;\n\tThu, 9 Sep 2021 18:09:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Cs8qhxJ0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631203744;\n\tbh=5r/YlWNX3goVXMEDYsv6JNu+iknHdQxZiyzh5P6ndRo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Cs8qhxJ0vlqV9KhdWEI9Q7cSTY3qZc3UOHRON24PDaW2HqdbT6pfDdcJzF75GRBPC\n\tEg5/QvKPhSaEw4yVRZiKiZQ/WjeLI18bd/l9ItwIT6VOhNxxIsYFgnVQfN0sG85oCh\n\t+asPM7BiqFY/8XPSYl7PoCBbkNuam8Z54+ydVmV8=","Date":"Thu, 9 Sep 2021 19:08:44 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210909150803.4014957-1-kieran.bingham@ideasonboard.com>\n\t<743efd34-950b-fb62-a1f5-e170df6bee64@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<743efd34-950b-fb62-a1f5-e170df6bee64@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2_videodevice:\n\tHandle unexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"LibCamera Devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]