From patchwork Tue Dec 13 09:38:00 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 17992 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 41800C328D for ; Tue, 13 Dec 2022 09:38:16 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D25D263366; Tue, 13 Dec 2022 10:38:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1670924295; bh=tKJs3qRfRLIfF1oBxpnOMmxW/QJT+8A9hcgV/cpmjmE=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=w0EAkmY4NgsaJZP3LIPOBaO3E9CvbZ0umdV1A5Mt0R4gL5KdOWYpGGLK6arBi4JeW x68ODYnvHTh6yyvRjHjIUlb5GmmRCvA0aEyzp1cWZUC+EzgLbBhaGkMnMOH5mDA7xX oY6gSunN0I5pj7htt2MMsXWG/eCVY1F4BA1Kpw+2kztxC1HH0MhvIR+Kzs/88wqe46 3mmyy1GHP5BrV1dw/6TidM6R5iHHfRjw2tytu060K5qqnvGmygQ3RbUcgtGqJoaU6y Essk1ALxvnPx/7p7ZNL1XME04whipRj0wiBULDq5nFuerv7vEz9Mju6jiRILABNm/G chvEMIUv3+x7A== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A8FB6334D for ; Tue, 13 Dec 2022 10:38:14 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ePdYVyB5"; dkim-atps=neutral Received: from pyrite.tail37cf.ts.net (h175-177-042-159.catv02.itscom.jp [175.177.42.159]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id ED5BF4A7; Tue, 13 Dec 2022 10:38:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1670924293; bh=tKJs3qRfRLIfF1oBxpnOMmxW/QJT+8A9hcgV/cpmjmE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ePdYVyB5TSXMXyLtceeCPj0K5BQiZ55tRAfCDFPmHXKnXMaYdfWQrIo6wg78aCs2B 07JIn4WQvCphiHo1fNiXwtaAhCnVMKo0YUhWNeytH4au3xRnMMkPLj8r1lC8XGUF0t tNrZTSrIJdplpH4uKD15F0P52gbUbwJ6nvnM9hpY= To: libcamera-devel@lists.libcamera.org Date: Tue, 13 Dec 2022 18:38:00 +0900 Message-Id: <20221213093802.704177-2-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221213093802.704177-1-paul.elder@ideasonboard.com> References: <20221213093802.704177-1-paul.elder@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/3] lc-compliance: simple_capture: Fix Request reuse X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Paul Elder via libcamera-devel From: Paul Elder Reply-To: Paul Elder Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" In the balanced simple capture test, in the request completion handler, the Request is marked for reuse *before* checking (based on the queueCount_) if it should actually be queued to the Camera. This causes the last Request to end up being marked for reuse and then *not* get queued to the Camera. The consequence of this is that a buffer is assigned to the Request, so that it must be canceled when the Request is destroyed. Additionally, since the Request was not queued to the Camera, it doesn't get completed at all by the Camera. Thus the buffer is deallocated by SimpleCapture::stop(), and after SimpleCaptureBalanced::capture() exits, the Request is destroyed and tries to cancel the deallocated buffer, causing the segfault. Fix this by reordering marking the Request for reuse so that it happens if and only if the Request wil be queued to the Camera. Fixes: https://bugs.libcamera.org/show_bug.cgi?id=171 Signed-off-by: Paul Elder --- src/apps/lc-compliance/simple_capture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp index cf4d7cf3..4a05b919 100644 --- a/src/apps/lc-compliance/simple_capture.cpp +++ b/src/apps/lc-compliance/simple_capture.cpp @@ -122,6 +122,7 @@ int SimpleCaptureBalanced::queueRequest(Request *request) if (queueCount_ > captureLimit_) return 0; + request->reuse(Request::ReuseBuffers); return camera_->queueRequest(request); } @@ -133,7 +134,6 @@ void SimpleCaptureBalanced::requestComplete(Request *request) return; } - request->reuse(Request::ReuseBuffers); if (queueRequest(request)) loop_->exit(-EINVAL); } From patchwork Tue Dec 13 09:38:01 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 17993 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id A565EC31E9 for ; Tue, 13 Dec 2022 09:38:18 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 68B5163369; Tue, 13 Dec 2022 10:38:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1670924298; bh=7d6tePXZ0Z99ZziKOsYZVhwwWsTn9Jj1eOd5UQlwcNo=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=kqM75YZAd+Mlpb/6agawMRho0Ap3U67eG04DQQgxyyR170Hob20SPaYpb2aHZRJ8t QVTnA9FQo0m34Ojh7zb9pZ9nDGb9Owz6srHcxq6BZqjgYdHB2ORbwzi609cygiJFa1 BvcGmDAzzVM1VvacKHOzPb8XSdvJ+yHT/3ovN9Con2SjEzmLVl8EBOpq7KIb5aPsSm BoDnVM6KJBf7OmZH6FjdFRE2/IAix37MmEE9u7AfXWxeczHIOnQxF9tyL6j2bSCmFI SO97lDFQ07tspr9DLGzTUcjbdm1KyDjmUpklSZssRJEaTfjC4g8jQTTmapa5fpfNGt nm8ECw5iypSww== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 9787563354 for ; Tue, 13 Dec 2022 10:38:15 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="PPV6AGNq"; dkim-atps=neutral Received: from pyrite.tail37cf.ts.net (h175-177-042-159.catv02.itscom.jp [175.177.42.159]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D01C7C5; Tue, 13 Dec 2022 10:38:14 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1670924295; bh=7d6tePXZ0Z99ZziKOsYZVhwwWsTn9Jj1eOd5UQlwcNo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=PPV6AGNql92jv31Eq4tvGMcNs3YVcZUpmLA9McFH73k+wyvuZ4PRSePpPE3dDUO+X NDFj3YXvpMGTbigoaLptr1zVWmqQa0gMLckpN1m2dM2sHYF7COaFztejxtsz7/xqY8 WdymELHoBR7CTZ6mPEKfbxwo0/ojMe6cRBXPn1Ns= To: libcamera-devel@lists.libcamera.org Date: Tue, 13 Dec 2022 18:38:01 +0900 Message-Id: <20221213093802.704177-3-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221213093802.704177-1-paul.elder@ideasonboard.com> References: <20221213093802.704177-1-paul.elder@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 2/3] Documentation: application-developer: Elaborate on request reuse X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Paul Elder via libcamera-devel From: Paul Elder Reply-To: Paul Elder Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" It recently came to our attention that marking a Request for reuse and *not* queueing it causes segfaults at Request deallocation time, if it happens after framebuffer deallocation. Add a note to warn against this in the application-developer documentation. Signed-off-by: Paul Elder --- Documentation/guides/application-developer.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst index 1b2d7727..26d5fc1d 100644 --- a/Documentation/guides/application-developer.rst +++ b/Documentation/guides/application-developer.rst @@ -490,6 +490,10 @@ device: request->reuse(Request::ReuseBuffers); camera->queueRequest(request); +Note that a request should only be marked for re-use if and only if the request +will be re-queued to the camera device. Marking a request for re-use and not +queuing it causes undefined behavior. + Request queueing ---------------- From patchwork Tue Dec 13 09:38:02 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 17994 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 22B4EC328D for ; Tue, 13 Dec 2022 09:38:19 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C8C456336C; Tue, 13 Dec 2022 10:38:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1670924298; bh=AqyWvYINfP/fhCIr12ILm72HuTFOR9Ucc4NP/MUJz1o=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=lbnNQ2t6eVWs8aOkpWndoONpuUJTnKs5+KQzEL32frrJr5L5AjVBidM+JK7+3TVYw VimCIpKVXYHsE8qwQnihqWZl/lDlPZi8Y3dZk70JEc2cpAWU/U5bVQYzO17fwZqHod n2GDuBM7/IbZUeKx+fiU5qAJrJq4zvzQYCt+6l3puuS/3X5vbw1Ozjcp+Sfd8NK3Pi cqdxAl5JcPb/z442nIXnDzaqIpeQF+2xg5ip8LaxXyITAGb/s9Y1aSn5405uHuuj01 B4c5CyI5fjjDXU5o7pJCTyFpZj1i1Z7mVDNavGTVycp8vU+62jmHB+r4RLqmRoXibt 5xmYMAum7iWhA== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 16DB563354 for ; Tue, 13 Dec 2022 10:38:17 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ffuqU4SO"; dkim-atps=neutral Received: from pyrite.tail37cf.ts.net (h175-177-042-159.catv02.itscom.jp [175.177.42.159]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E2EF9AFC; Tue, 13 Dec 2022 10:38:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1670924296; bh=AqyWvYINfP/fhCIr12ILm72HuTFOR9Ucc4NP/MUJz1o=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ffuqU4SOyQ0yq89aN04fwQP4cN+8OGUY+z0blql04pgizPwQYlEJULT093tPcjVMC 5SEP6Ro+RKa4OrEQ4W1+ze2/qT6dOdmb9Is7J4OQ3HFJKIjudL2uXbSZYlKxElyVoQ erVBHKM3xnPMtQXS2GdewGB7WFZKh1M9dZgSAV6g= To: libcamera-devel@lists.libcamera.org Date: Tue, 13 Dec 2022 18:38:02 +0900 Message-Id: <20221213093802.704177-4-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221213093802.704177-1-paul.elder@ideasonboard.com> References: <20221213093802.704177-1-paul.elder@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 3/3] libcamera: camera: Add todo for race condition on queueRequest X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Paul Elder via libcamera-devel From: Paul Elder Reply-To: Paul Elder Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" There is a risk of a racy segfault in Camera::queueRequest, related to marking a Request for reuse without queueing it to the camera. Camera::queueRequest() could race with Camera::stop(), which would trigger a segfault if the buffers are freed before their Requests. As it's not too critical at the moment, add a description of the problem and a todo. Signed-off-by: Paul Elder --- src/libcamera/camera.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2d947a44..6d871895 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1114,6 +1114,21 @@ int Camera::queueRequest(Request *request) { Private *const d = _d(); + /* + * There is a risk of a racy segfault here. If the application marks a + * Request for reuse and queues it, but stop() changes the camera state + * before we reach this point, then we would end up in a situation + * where we have a buffer added to a Request yet not queued to the + * camera. Thus Camera::stop() will not complete the buffer and + * request, and if the buffer is freed before its request is destroyed, + * then it will cause a segfault when the request tries to cancel the + * freed buffer. + * + * The temporary workaround is to force applications to make sure to + * free requests before the buffers. + * + * \todo Fix this race condition. + */ int ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0) return ret;