[{"id":18392,"web_url":"https://patchwork.libcamera.org/comment/18392/","msgid":"<20210728021125.GE2167@pyrite.rasen.tech>","date":"2021-07-28T02:11:25","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n> kernels.\n> \n> Handle unexpected buffers by returning a nullptr, and move cache\n> management after the validation of the buffer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nLooks good.\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n> +\t * returns a failure upon queing, being mistakenely 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> -- \n> 2.30.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5127CC322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Jul 2021 02:11:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 913B4687C4;\n\tWed, 28 Jul 2021 04:11:35 +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 AE538687AF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Jul 2021 04:11:33 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 248D2EE;\n\tWed, 28 Jul 2021 04:11:31 +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=\"PPn4i/7Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627438293;\n\tbh=lv/R8uTlChzjL4HrQSGdeSzggrJmJPdWtR6euu1HyNA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PPn4i/7QvJJ7OrpWfQVfugEnAZjxNxaGVvsndye/XNS2t8mN9yYE2L6M1UMSv4mlV\n\t0YzNVH6bGEsp66KDxNdmAf3aoD++SLbNO60NhmC0LxQUl/a5blmDN/QXF35g/oepCt\n\tclwHw+Nn1lVBJOhGgT4sViqtvFjG/ftZ5Cali9r4=","Date":"Wed, 28 Jul 2021 11:11:25 +0900","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20210728021125.GE2167@pyrite.rasen.tech>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18393,"web_url":"https://patchwork.libcamera.org/comment/18393/","msgid":"<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>","date":"2021-07-28T06:24:41","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected 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, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n> kernels.\n> \n> Handle unexpected buffers by returning a nullptr, and move cache\n> management after the validation of the buffer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n> +\t * returns a failure upon queing, being mistakenely kept in the kernel.\n\ns/queing,/queing/\n\nYou've mistakenly spelled mistakenly mistakenely :-)\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\nIt may be worth expanding this a bit to explain about the\nstream-on-at-qbuf-time issue. If I recall correctly, given that the\ndevice fails to stream, the issue only occurs when we call\nVIDIOC_STREAMOFF and all buffers currently in queue (including the one\nerroneously kept in the queue) being signalled as complete in the error\nstate. Is that right ?\n\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\nIs the fix on its way upstream ? If so, this could probably be reworded,\nand mentioning the target kernel version (and even better, the upstream\ncommit) would be nice.\n\n> +\t */\n> +\tif (it == queuedBuffers_.end()) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Dequeued an unexpected buffer:\" << buf.index;\n\nMissing space after ':'.\n\n> +\n\nYou can drop the blank line.\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>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3C880C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Jul 2021 06:24:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AE5E687C4;\n\tWed, 28 Jul 2021 08:24:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FB3D6026F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Jul 2021 08:24:48 +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 DCAF5EE;\n\tWed, 28 Jul 2021 08:24:47 +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=\"hyWKzCpu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627453488;\n\tbh=UvfVp9no4ynjgbgBfb41i4K44FFkr01eiRe7ocak87Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hyWKzCpuHbHCU5IDz8zGpV8vqjxUMgqza5xkvttwID1iNiDKGGT06G6BkoEpe+bqO\n\tfK7mj/cHWLUa9XNDIm1aUjRO3YwdksMMtt8HgZGXhWmp4Ptf8yeGNgx86xmFMB2Ofv\n\t+Usp69v8FSpSi8NbwtzH1PBfRvUbJAoimg77Tmqk=","Date":"Wed, 28 Jul 2021 09:24:41 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18404,"web_url":"https://patchwork.libcamera.org/comment/18404/","msgid":"<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>","date":"2021-07-28T08:51:19","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 28/07/2021 07:24, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n>> kernels.\n>>\n>> Handle unexpected buffers by returning a nullptr, and move cache\n>> management after the validation of the buffer.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n>> +\t * returns a failure upon queing, being mistakenely kept in the kernel.\n> \n> s/queing,/queing/\n> \n> You've mistakenly spelled mistakenly mistakenely :-)\n\nYou're too slow ;-)\n\nHighlighted in my reply to myself already.\n\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> \n> It may be worth expanding this a bit to explain about the\n> stream-on-at-qbuf-time issue. If I recall correctly, given that the\n> device fails to stream, the issue only occurs when we call\n> VIDIOC_STREAMOFF and all buffers currently in queue (including the one\n> erroneously kept in the queue) being signalled as complete in the error\n> state. Is that right ?\n\nI don't think we're calling stream off because we haven't called stream\non (by nature of the fact that it was a failure at stream-on-at-qbuf-time?).\n\nAlthough, perhaps we do as the queueBuffer could lead to an error path\nthat calls cio2_->stop() which does call streamOff regardless, but it's\na fairly moot point by that stage.\n\nThe key issue is that the kernel notifies us of a buffer as completed in\nerror state, that we believed didn't queue - because it failed to\ncomplete in Q_BUF.\n\n\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> Is the fix on its way upstream ? If so, this could probably be reworded,\n> and mentioning the target kernel version (and even better, the upstream\n> commit) would be nice.\n\nI've given Tested-by: tags to Hans on his proof-of-concept patch, I\nhaven't seen any updated patch come through from him though, so I don't\ncurrently know the progress.\n\nIt may need chasing, or taking over if he doesn't have time.\n\n\n>> +\t */\n>> +\tif (it == queuedBuffers_.end()) {\n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Dequeued an unexpected buffer:\" << buf.index;\n> \n> Missing space after ':'.\n\nAlso in my self-review and already fixed locally ;-)\n\n> \n>> +\n> \n> You can drop the blank line.\n\nAck.\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 20D1BC3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Jul 2021 08:51:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7195C687C4;\n\tWed, 28 Jul 2021 10:51:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C966687B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Jul 2021 10:51:22 +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 BCF3F3F;\n\tWed, 28 Jul 2021 10:51:21 +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=\"RN0YzVAo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627462281;\n\tbh=x2sBwZfmZd3pzwqmuiQtzwLKr9qUsZTW3025hqwBqi0=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=RN0YzVAonjBFLDLuXKIprFxLtn3vQt0QEgA00XxdN/N9g+qztZP9AGmQkNhB9h9/T\n\tcutbKFuTMAmwBgR1ZCt7TkkKE+uFlsYaMNWzy8WfI0cOzi1V17I0lm87zYV8tRK4ur\n\tL5Kzn5eixUzmiK5PIVVsZV1UBN/M9B+KQ273jvhY=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>\n\t<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>","Message-ID":"<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>","Date":"Wed, 28 Jul 2021 09:51:19 +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":"<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18406,"web_url":"https://patchwork.libcamera.org/comment/18406/","msgid":"<YQE+1/6xyRokDHZi@pendragon.ideasonboard.com>","date":"2021-07-28T11:26:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 28, 2021 at 09:51:19AM +0100, Kieran Bingham wrote:\n> On 28/07/2021 07:24, Laurent Pinchart wrote:\n> > On Thu, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n> >> kernels.\n> >>\n> >> Handle unexpected buffers by returning a nullptr, and move cache\n> >> management after the validation of the buffer.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n> >> +\t * returns a failure upon queing, being mistakenely kept in the kernel.\n> > \n> > s/queing,/queing/\n> > \n> > You've mistakenly spelled mistakenly mistakenely :-)\n> \n> You're too slow ;-)\n> \n> Highlighted in my reply to myself already.\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> > \n> > It may be worth expanding this a bit to explain about the\n> > stream-on-at-qbuf-time issue. If I recall correctly, given that the\n> > device fails to stream, the issue only occurs when we call\n> > VIDIOC_STREAMOFF and all buffers currently in queue (including the one\n> > erroneously kept in the queue) being signalled as complete in the error\n> > state. Is that right ?\n> \n> I don't think we're calling stream off because we haven't called stream\n> on (by nature of the fact that it was a failure at stream-on-at-qbuf-time?).\n\nStrream-on-at-qbuf means that STREAMON is called and succeeds, but\ndoesn't reach the driver because vb2 blocks the STREAMON until enough\nbuffers are available. When a QBUF provides the needed buffer, vb2 calls\nthe .start_streaming() operation, which starts the device. A failure at\nthat point is reported as a QBUF failure, not a STREAMON failure has\nthat as completed long before. From a userspace point of view, the V4L2\nvideo node is streaming, and thus need to the stopped with STREAMOFF.\n\n> Although, perhaps we do as the queueBuffer could lead to an error path\n> that calls cio2_->stop() which does call streamOff regardless, but it's\n> a fairly moot point by that stage.\n> \n> The key issue is that the kernel notifies us of a buffer as completed in\n> error state, that we believed didn't queue - because it failed to\n> complete in Q_BUF.\n\nrMmy point is that that notification occurs at STREAMOFF time, when all\nqueued buffers are given back to userspace. The device hasn't been\nstarted due to the .start_streaming() failure, so the kernel doesn't\nproduce any buffer ready event until STREAMOFF time.\n\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> > Is the fix on its way upstream ? If so, this could probably be reworded,\n> > and mentioning the target kernel version (and even better, the upstream\n> > commit) would be nice.\n> \n> I've given Tested-by: tags to Hans on his proof-of-concept patch, I\n> haven't seen any updated patch come through from him though, so I don't\n> currently know the progress.\n> \n> It may need chasing, or taking over if he doesn't have time.\n\nLet's start the chase :-)\n\n> >> +\t */\n> >> +\tif (it == queuedBuffers_.end()) {\n> >> +\t\tLOG(V4L2, Error)\n> >> +\t\t\t<< \"Dequeued an unexpected buffer:\" << buf.index;\n> > \n> > Missing space after ':'.\n> \n> Also in my self-review and already fixed locally ;-)\n> \n> >> +\n> > \n> > You can drop the blank line.\n> \n> Ack.\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> >>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A7422C3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Jul 2021 11:26:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D631687BE;\n\tWed, 28 Jul 2021 13:26:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B626D60506\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Jul 2021 13:26:22 +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 3CBDF3F;\n\tWed, 28 Jul 2021 13:26:22 +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=\"W6fwock7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627471582;\n\tbh=cDMaF/sEARweaxDLhVRVnJqKEByYCG6IdxM17kq/rvQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W6fwock7mi09SAy0GApY/aqBH9wt9OOht+6ISjjzFigAf25u1JCx0XkOwb1XiKzP4\n\tiQhrGdTxFZF0b+IFJFWfmtd8CCrVejApnz7b4ewhuT243Tsl5oXcnKnks1LRIUoGaK\n\t2V88KcGZrQ0tLjjChofGX2LloqiGh8efeDvJo4Rg=","Date":"Wed, 28 Jul 2021 14:26:15 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YQE+1/6xyRokDHZi@pendragon.ideasonboard.com>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>\n\t<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>\n\t<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18737,"web_url":"https://patchwork.libcamera.org/comment/18737/","msgid":"<c83ae008-faa2-8561-b52b-029ad20637c3@ideasonboard.com>","date":"2021-08-12T13:09:18","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 28/07/2021 12:26, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Jul 28, 2021 at 09:51:19AM +0100, Kieran Bingham wrote:\n>> On 28/07/2021 07:24, Laurent Pinchart wrote:\n>>> On Thu, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n>>>> kernels.\n>>>>\n>>>> Handle unexpected buffers by returning a nullptr, and move cache\n>>>> management after the validation of the buffer.\n>>>>\n>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n>>>> +\t * returns a failure upon queing, being mistakenely kept in the kernel.\n>>>\n>>> s/queing,/queing/\n>>>\n>>> You've mistakenly spelled mistakenly mistakenely :-)\n>>\n>> You're too slow ;-)\n>>\n>> Highlighted in my reply to myself already.\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>>>\n>>> It may be worth expanding this a bit to explain about the\n>>> stream-on-at-qbuf-time issue. If I recall correctly, given that the\n>>> device fails to stream, the issue only occurs when we call\n>>> VIDIOC_STREAMOFF and all buffers currently in queue (including the one\n>>> erroneously kept in the queue) being signalled as complete in the error\n>>> state. Is that right ?\n>>\n>> I don't think we're calling stream off because we haven't called stream\n>> on (by nature of the fact that it was a failure at stream-on-at-qbuf-time?).\n> \n> Strream-on-at-qbuf means that STREAMON is called and succeeds, but\n> doesn't reach the driver because vb2 blocks the STREAMON until enough\n> buffers are available. When a QBUF provides the needed buffer, vb2 calls\n> the .start_streaming() operation, which starts the device. A failure at\n> that point is reported as a QBUF failure, not a STREAMON failure has\n> that as completed long before. From a userspace point of view, the V4L2\n> video node is streaming, and thus need to the stopped with STREAMOFF.\n> \n>> Although, perhaps we do as the queueBuffer could lead to an error path\n>> that calls cio2_->stop() which does call streamOff regardless, but it's\n>> a fairly moot point by that stage.\n>>\n>> The key issue is that the kernel notifies us of a buffer as completed in\n>> error state, that we believed didn't queue - because it failed to\n>> complete in Q_BUF.\n> \n> rMmy point is that that notification occurs at STREAMOFF time, when all\n> queued buffers are given back to userspace. The device hasn't been\n> started due to the .start_streaming() failure, so the kernel doesn't\n> produce any buffer ready event until STREAMOFF time.\n> \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>>> Is the fix on its way upstream ? If so, this could probably be reworded,\n>>> and mentioning the target kernel version (and even better, the upstream\n>>> commit) would be nice.\n>>\n>> I've given Tested-by: tags to Hans on his proof-of-concept patch, I\n>> haven't seen any updated patch come through from him though, so I don't\n>> currently know the progress.\n>>\n>> It may need chasing, or taking over if he doesn't have time.\n> \n> Let's start the chase :-)\n\n\nThe relevant kernel patch is posted, and headed to the stable trees, so\nit will be picked up by CrOS soon enough.\n\nDo you want to explicitly drop this patch, or are you happy to keep it?\n\nI still think it has some merit, as it's better to report the issue\ncleanly in the event that it happens ... however, its' a rare kernel bug\n(I expect) that is already patched, and thus should become ... rarer ...\n\n--\nKieran\n\n\n> \n>>>> +\t */\n>>>> +\tif (it == queuedBuffers_.end()) {\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Dequeued an unexpected buffer:\" << buf.index;\n>>>\n>>> Missing space after ':'.\n>>\n>> Also in my self-review and already fixed locally ;-)\n>>\n>>>> +\n>>>\n>>> You can drop the blank line.\n>>\n>> Ack.\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 C5954BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 12 Aug 2021 13:09:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 387F460265;\n\tThu, 12 Aug 2021 15:09:24 +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 44ABC60264\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 12 Aug 2021 15:09:22 +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 AE06749A;\n\tThu, 12 Aug 2021 15:09:21 +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=\"p3NnccoN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628773761;\n\tbh=pe6ShZgHi+OOuuhk1iG9TsolB8cFrJlz3SHhzK4FCWM=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=p3NnccoN7aKMZNqwRlhTbMwCb8tzznRkMQF4iHBt5QDneROWS7SXjftZOabNOgHTd\n\tqBqm0gXsUpRiFtiE6Y1ky0k6yyCq38Z+7v1Rr/ZwYzKMZza+GGq8yCtks1aW8A2ktD\n\tgxEU6PgL8SnuY2uFon5WXaDlT1IhXOzXjF5L6ddQ=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>\n\t<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>\n\t<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>\n\t<YQE+1/6xyRokDHZi@pendragon.ideasonboard.com>","Message-ID":"<c83ae008-faa2-8561-b52b-029ad20637c3@ideasonboard.com>","Date":"Thu, 12 Aug 2021 14:09:18 +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":"<YQE+1/6xyRokDHZi@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18739,"web_url":"https://patchwork.libcamera.org/comment/18739/","msgid":"<YRUf8uO5kX/tl+ht@pendragon.ideasonboard.com>","date":"2021-08-12T13:19:46","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Aug 12, 2021 at 02:09:18PM +0100, Kieran Bingham wrote:\n> On 28/07/2021 12:26, Laurent Pinchart wrote:\n> > On Wed, Jul 28, 2021 at 09:51:19AM +0100, Kieran Bingham wrote:\n> >> On 28/07/2021 07:24, Laurent Pinchart wrote:\n> >>> On Thu, Jul 15, 2021 at 03:21:30PM +0100, 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 must protect against unpatched\n> >>>> kernels.\n> >>>>\n> >>>> Handle unexpected buffers by returning a nullptr, and move cache\n> >>>> management after the validation of the buffer.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 3d2d99b46e4e..6c7f9daf24db 100644\n> >>>> --- a/src/libcamera/v4l2_videodevice.cpp\n> >>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >>>> @@ -1519,9 +1519,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 queing a buffer), a vb2 kernel bug can lead to the buffer which\n> >>>> +\t * returns a failure upon queing, being mistakenely kept in the kernel.\n> >>>\n> >>> s/queing,/queing/\n> >>>\n> >>> You've mistakenly spelled mistakenly mistakenely :-)\n> >>\n> >> You're too slow ;-)\n> >>\n> >> Highlighted in my reply to myself already.\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> >>>\n> >>> It may be worth expanding this a bit to explain about the\n> >>> stream-on-at-qbuf-time issue. If I recall correctly, given that the\n> >>> device fails to stream, the issue only occurs when we call\n> >>> VIDIOC_STREAMOFF and all buffers currently in queue (including the one\n> >>> erroneously kept in the queue) being signalled as complete in the error\n> >>> state. Is that right ?\n> >>\n> >> I don't think we're calling stream off because we haven't called stream\n> >> on (by nature of the fact that it was a failure at stream-on-at-qbuf-time?).\n> > \n> > Strream-on-at-qbuf means that STREAMON is called and succeeds, but\n> > doesn't reach the driver because vb2 blocks the STREAMON until enough\n> > buffers are available. When a QBUF provides the needed buffer, vb2 calls\n> > the .start_streaming() operation, which starts the device. A failure at\n> > that point is reported as a QBUF failure, not a STREAMON failure has\n> > that as completed long before. From a userspace point of view, the V4L2\n> > video node is streaming, and thus need to the stopped with STREAMOFF.\n> > \n> >> Although, perhaps we do as the queueBuffer could lead to an error path\n> >> that calls cio2_->stop() which does call streamOff regardless, but it's\n> >> a fairly moot point by that stage.\n> >>\n> >> The key issue is that the kernel notifies us of a buffer as completed in\n> >> error state, that we believed didn't queue - because it failed to\n> >> complete in Q_BUF.\n> > \n> > rMmy point is that that notification occurs at STREAMOFF time, when all\n> > queued buffers are given back to userspace. The device hasn't been\n> > started due to the .start_streaming() failure, so the kernel doesn't\n> > produce any buffer ready event until STREAMOFF time.\n> > \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> >>> Is the fix on its way upstream ? If so, this could probably be reworded,\n> >>> and mentioning the target kernel version (and even better, the upstream\n> >>> commit) would be nice.\n> >>\n> >> I've given Tested-by: tags to Hans on his proof-of-concept patch, I\n> >> haven't seen any updated patch come through from him though, so I don't\n> >> currently know the progress.\n> >>\n> >> It may need chasing, or taking over if he doesn't have time.\n> > \n> > Let's start the chase :-)\n> \n> The relevant kernel patch is posted, and headed to the stable trees, so\n> it will be picked up by CrOS soon enough.\n> \n> Do you want to explicitly drop this patch, or are you happy to keep it?\n> \n> I still think it has some merit, as it's better to report the issue\n> cleanly in the event that it happens ... however, its' a rare kernel bug\n> (I expect) that is already patched, and thus should become ... rarer ...\n\nI'm fine keeping the patch, but I'd like to reword the above comment to\nindicate when the issue has been fixed in the kernel. The corresponding\ncommit ID should be recorded, either in the commit message, or in the\ncomment.\n\n> >>>> +\t */\n> >>>> +\tif (it == queuedBuffers_.end()) {\n> >>>> +\t\tLOG(V4L2, Error)\n> >>>> +\t\t\t<< \"Dequeued an unexpected buffer:\" << buf.index;\n> >>>\n> >>> Missing space after ':'.\n> >>\n> >> Also in my self-review and already fixed locally ;-)\n> >>\n> >>>> +\n> >>>\n> >>> You can drop the blank line.\n> >>\n> >> Ack.\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> >>>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 77885C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 12 Aug 2021 13:19:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE6236884F;\n\tThu, 12 Aug 2021 15:19:52 +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 26E8760264\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 12 Aug 2021 15:19:51 +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 9BDB9EE;\n\tThu, 12 Aug 2021 15:19:50 +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=\"JgJk7XrY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628774390;\n\tbh=xRlgKyGSprAAjeEEc/S7ayZafV3xJvtKOPF/AShnDT4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JgJk7XrYBg4u8CKQxpg4iW4GBnUiZqMTzO/YCx4JkNjwrbuJp2II0BzLQxjW1NiDQ\n\togepPD7AIPWbqM23eOseYMC6AJH3vOgzv5rR01dqvRpUrd+Y6cBXRE/rnV3NUljfIO\n\tJJVwbwdSx2/6+AQoJADcTS63DZsx6pfhw+SvLaIo=","Date":"Thu, 12 Aug 2021 16:19:46 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YRUf8uO5kX/tl+ht@pendragon.ideasonboard.com>","References":"<20210715142130.139675-1-kieran.bingham@ideasonboard.com>\n\t<YQD4Kd6qWjUtV2x2@pendragon.ideasonboard.com>\n\t<299af811-0807-6144-b663-ebe05a3bec90@ideasonboard.com>\n\t<YQE+1/6xyRokDHZi@pendragon.ideasonboard.com>\n\t<c83ae008-faa2-8561-b52b-029ad20637c3@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<c83ae008-faa2-8561-b52b-029ad20637c3@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Handle\n\tunexpected buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]