From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=100.103.45.18 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793974186; b=Htmyf/gXsqCRVaPgyxqpwDbkvumfNYhACp43Ih2tjBMDgN0CcCjVX23beNhTAMGk4R2X V1eoozV4975fSt9NYvJYFYLHuK9TRDHmBZxtxuUTezXVtCTRDsPf0iYCMO3q6QreFynbJ lD52EIv1CA7x+HmImbZkneWgws0w9XLLC5GwceMdd60pPW9AewC0kaDnDStufMjorYtKx B5EJWFj3JNEXd2u/qSvy75gJGgrjdbEBI/TTxIneegdGNiKgx6C+zqiYPO424IsLSiX0C E8jWu/D0te6j7wVu9n+px4KnzvSV7PQWr42+DG0N43NBNKznJ0+Oo2HkGFl46o6fkWg== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793974186; h=Received:DKIM-Signature:From:To:Cc:Subject:Date:Message-ID:X-Mailer: In-Reply-To:References:MIME-Version:Content-Transfer-Encoding; bh=IH/TJ53LzMS9+Dp833uc+s0hgSoGSvDJkuv+KDBZzzs=; b=C+XTCY12w9goKC24xJH+RPwZ0GeXPrGvPhL9ThrMcZhy1w3HWypjH7Gsu10tmbFyA73Q sFXxiruqFJPwKdc9V0KvMDb1bOyEZ0YHV9Xwn4xMA5yr9V13bADf2s2XwehfbEwQ0ccgV KniEjfnCw0fce0pEp1LTlg8Vz1vxV2RFmMHhfkL5fr1pVmTgfiMYn9eg+DLFs1TFctTuo IJtv2XNjrbv3x580up3IlpOAUzkL58SnleceA1T7FOajb+bvAQYa/ssojjGIaCsgvra1a 1l04cM/guG5rXMYCM4s+LXbiktybVNcp3b2YUN8KZtoL7ni+Vctuz9TgHdINjkCFLHw== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=Y37gP8Z7; arc=none smtp.remote-ip=100.103.45.18 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7BF19606A332; Fri, 6 Nov 2026 14:09:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=Y37gP8Z7 Received: by smtp.kernel.org (Postfix) with ESMTPSA id D68321F008A6; Fri, 6 Nov 2026 14:09:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1793974186; bh=IH/TJ53LzMS9+Dp833uc+s0hgSoGSvDJkuv+KDBZzzs=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=Y37gP8Z75TisB/IDzD4Bu4eiETiIs+6NzjcM8Hx9wKtIxGIlsDC2thHVJAZZDJKK0 UcGyktwawD9GdCu9hWAw9g9+OtrVIu+XzmUVbdC81IqWPsRoUB1WLybK/kW9JqPVbh +ylUCMCxbLbnIzfhD+ks5CWPk6iLT89sw9kukFOD539FsrLInMY62Z679pCQtM+/5j 7A1bSGGl744DPWlnBF9GAlp4DaolwEcirJJWwDeco3j4ysvSuxqnIUEN4V99iplXE0 rmbWqgyUFuZHRRxF7I3xFK40Uj4TOHqEct/fGb4sW3wgj9dfE45gizlo7mrSCur/3Y D/zQL8+BBODdg== From: Philipp Stanner To: Sumit Semwal , =?UTF-8?q?Christian=20K=C3=B6nig?= , Philipp Stanner , Danilo Krummrich , Alice Ryhl , Miguel Ojeda , Boqun Feng , Gary Guo , =?UTF-8?q?Bj=C3=B6rn=20Roy=20Baron?= , Benno Lossin , Andreas Hindborg , Trevor Gross , Daniel Almeida , Tamir Duberstein , Alexandre Courbot , =?UTF-8?q?Onur=20=C3=96zkan?= , Matthew Brost , David Airlie , Simona Vetter , Tvrtko Ursulin Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org Subject: [PATCH 4/8] rust: DmaFence: Replace call_rcu() with synchronize_rcu() Date: Fri, 6 Nov 2026 15:07:49 +0100 Message-ID: <20261106140752.520386-6-phasta@kernel.org> X-Mailer: git-send-email 2.56.0 In-Reply-To: <20261106140752.520386-2-phasta@kernel.org> References: <20261106140752.520386-2-phasta@kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Transfer-Encoding: 8bit The C dma_fence contract demands that a fence's data disappears no earlier than 1 RCU grace period after the DriverFence was signaled. So far, we achieved this with dropping the DriverFence's data in a deferred manner with a manual, raw binding call to call_rcu(). However, this call_rcu() does not solve one problem: It is conceivable that some driver data must not drop in atomic context; but call_rcu()'s payload *can* be executed in atomic context. Moreover, the current developments in drm::JobQueue, where a DriverFence is always coupled 1:1 with a Job, allows for dropping job and fence through a work item. The undesirable blocking behavior of synchronize_rcu() would, thus, not be annoying any thread anymore. The direct users of DmaFence could achieve the same by dropping their DriverFences through work items. The only conceivable other solution, queue_rcu_work(), would, for many users, schedule a work_item from another work_item, which seems undesirable. Additionally, using the existing synchronize_rcu() abstraction has the advantage of us getting rid of a raw bindings call. Replace DriverFence::drop()'s call_rcu() with synchronize_rcu(). Signed-off-by: Philipp Stanner --- rust/kernel/dma_buf/dma_fence.rs | 105 +++++++++---------------------- 1 file changed, 28 insertions(+), 77 deletions(-) diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs index c04b68fe67fe..f74119b4d7f8 100644 --- a/rust/kernel/dma_buf/dma_fence.rs +++ b/rust/kernel/dma_buf/dma_fence.rs @@ -42,7 +42,7 @@ Atomic, Relaxed, // }, - rcu::rcu_barrier, // + rcu::synchronize_rcu, // }, // }; @@ -58,6 +58,8 @@ pub trait FenceContextOps { /// A dma-fence context. A fence context takes care of associating related fences /// with each other, providing each with raising sequence numbers and a common /// identifier. +/// +/// A FenceContext must drop in non-atomic context. #[pin_data(PinnedDrop)] pub struct FenceContext { /// The fence context number. @@ -65,7 +67,7 @@ pub struct FenceContext { /// The sequence number for the next fence created. seqno: Atomic, // The name parameters can be accessed by the dma_fence backend_ops. UAF - // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`. + // errors are prevented by `synchronize_rcu()`. /// The name of the driver this FenceContext's fences belong to. driver_name: CString, /// The name of the timeline this FenceContext's fences belong to. @@ -149,15 +151,11 @@ fn next_seqno(&self) -> u64 { /// /// This is needed because many times, creation of a [`DriverFence`] must not /// fail, and allocating might deadlock in some situations. - /// - /// The `data` you pass here must not perform any operations that are illegal - /// in atomic context in its [`Drop`] implementation. pub fn new_fence_allocation( &self, data: T::FenceDataType, ) -> Result> { let fence_data = DriverFenceData { - rcu_head: Default::default(), // `inner` remains uninitialized until a `DriverFence` takes over. inner: Fence { inner: Opaque::uninit(), @@ -243,15 +241,10 @@ fn drop(self: Pin<&mut Self>) { panic!("Forgotten fences in FenceContext."); } - // Ensure that the driver cannot unload while there are still dma_fence - // callbacks running. At the same time, the RCU barrier addresses the - // problem inherited by the C backend, in which backend ops callbacks - // might be accessing the fence while it is being signaled (or shortly - // after). This could cause UAF access on the fence context's - // `fctx.driver_name` and `fctx.timeline_name`. - // - // Wait for the RCU callbacks in `DriverFence::drop`. - rcu_barrier(); + // If a DriverFence signals without doing a synchronize_rcu(), and a + // fctx drops immediately afterwards, then backend_ops still accessing + // the fctx might run into UAF if we don't wait for them here. + synchronize_rcu(); } } @@ -615,12 +608,9 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> { // necessary so that the C backend can free the allocation (coming from our // Rust code) with kfree_rcu(). inner: Fence, - /// Callback head for dropping this in a deferred manner through RCU. - rcu_head: bindings::callback_head, /// Reference to access the FenceContext. fctx: &'a FenceContext, - /// The API user's data. It is essential that the data only performs - /// operations legal in atomic context in its [`Drop`] implementation. + /// The API user's data. #[pin] data: T::FenceDataType, } @@ -659,6 +649,8 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> { /// /// In the unlikely case of such violations, a panic is thrown. /// +/// A DriverFence must drop in non-atomic context. +/// /// # Examples /// /// ``` @@ -955,15 +947,15 @@ fn drop(&mut self) { // dma_fence_is_signaled_locked() because the C backend wants to get rid // of the latter. - // SAFETY: `guard` is valid until the `call_rcu()` below. + // SAFETY: `guard` is valid. let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) }; if !signaled { pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n"); - // SAFETY: `guard` is valid until the `call_rcu()` below. The fence - // must not have been signaled yet, which we check directly above. + // SAFETY: `guard` is valid. The fence must not have been signaled + // yet, which we check directly above. unsafe { bindings::dma_fence_set_error(guard.as_raw(), ECANCELED.to_errno()) }; - // SAFETY: `guard` is valid until the `call_rcu()` below. The lock - // must be held, which we acquired above. + // SAFETY: `guard` is valid. The lock must be held, which we + // acquired above. unsafe { bindings::dma_fence_signal_locked(guard.as_raw()) }; // SAFETY: `self.data` is valid because `self` is valid. @@ -988,59 +980,18 @@ fn drop(&mut self) { return; } - // SAFETY: Valid because `self` is valid. - let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head }; + // Make sure none of the fence backend_ops can access fence-data anymore. + // + // TODO: + // This would not be necessary if the C dma_fence backend were using a + // spinlock to properly synchronize its signaled state. Fix it in C and + // then remove synchronize_rcu(). + synchronize_rcu(); - // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was - // created validly above. The module must perform a `synchronize_rcu()` - // or `rcu_barrier()` call to guard against module unload. - unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::)) }; + // SAFETY: Valid because `self` is valid. + unsafe { drop_in_place(&raw mut self.data) }; + + // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone. + unsafe { bindings::dma_fence_put(self.as_raw()) }; } } - -// TODO: -// The entire call_rcu() mechanism in the drop above and the code below would be -// unnecessary if C's dma_fence_signal() could be reworked in a way that after it -// ran, the caller knows that no fence_ops callbacks can be running anymore. -// In other words, if the dma_fence backend would use its spinlock for full -// synchronization. -// -// Then we could move the drop_in_place() and dma_fence_put() upwards into the -// drop() implementation and call it a day. - -/// Finally really drop this `DriverFence` -/// -/// # Safety -/// -/// `head` references the `rcu_head` field of an `DriverFenceData`. All -/// accessors to that `DriverFenceData` must be gone by now. This must be -/// ensured by signalling the associated `DriverFence` and then waiting -/// for a grace period until calling this function here. -unsafe extern "C" fn drop_driver_fence_data( - head: *mut bindings::callback_head, -) { - // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData`. - let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) }; - - // SAFETY: `fence_data` was created validly above. All the fence's data will - // only drop below, but the raw pointer to the raw C `dma_fence` remains - // valid because the reference count is only decremented at the end of the - // function. - let fence = unsafe { (*fence_data).inner.inner.get() }; - - // SAFETY: `fence_data` was created validly above. The user has already - // dropped the only conventional accessor to the user data, the `DriverFence`, - // one grace period ago. All accessors are gone now. - unsafe { drop_in_place(&raw mut (*fence_data).data) }; - - // The inner `Fence` explicitly does not get dropped because there may be - // many more users / consumers, each holding their own reference. - - // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is valid - // and initialized. It is valid until the refcount drops to 0, which can - // earliest happen once we drop the `DriverFence`'s reference here. - unsafe { bindings::dma_fence_put(fence) }; - - // The actual memory the data associated with a `DriverFence` lives in - // gets freed by the C dma_fence backend once the fence's refcount reaches 0. -} -- 2.56.0