From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CFF163093C6 for ; Sat, 30 May 2026 15:06:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780153585; cv=none; b=ndDsoAeHezxb3RnSOL78CYCONL57OwbwcM+L1shiWtxVl6YMWIPewdQPk9r3dYpscEADNjqNG0LpNayrOH/NnE4/w2jGkleVhpHE49G/Yr3okmHTZUF1MwuBGTj4oVcuBHezUhzdzlaxKW5Nwc/OvVGjmX489xoha66MHiRNJMA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780153585; c=relaxed/simple; bh=HIyWHxYLXZNg/c6l1z/0l9ommiZTcuYmF4H/53c+uHk=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=PBkEWkEZrcm/n/EyItbfg2r9WIiABh8HOYHG4dGVM6o1tMUPYUQ4eCjWHpilepEK7g4ZGhTljdOMSBE9ZYLFL1ShecGNY8FFIq6jj6jOIWPgJVzbPemO9BUTPaTT2/CM4fBQCFQsJk2kT/zV707ZykRt7CZYz20eVEdGvaV4+64= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=PKIGo7Ad; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="PKIGo7Ad" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 393FC1F00893; Sat, 30 May 2026 15:06:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780153582; bh=VAqIyrmvuzHHk9D3eLJaA8Psc/ewn49nLxW/zDyacQc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=PKIGo7AdAUvY7wUHTwS1TGm8w6AhA4GxnavDdA2sKSSrrnpiGFPjtJ82tsnEE9QxP /iq5SAbSUStnG34XtK+FmJpUtAqZpyWXomrHQ2TpjLHNftuELWymhRsr08/8R41wkc Xq9iwziVFyAzyboddVcmo7/tQJUpI5UyHczHm+KnNmvbwK0kWzcu9cZk0K2u0pfHxB AQxxHqL9qBpirlujkUWLt9Z+fIdeblHQLS3P4LgAKePz9S/85NwBGz7PLU1jgmq/c+ /fKwS4Yq0r8M7hwwlTOC4dXOxUs0lmykcnDcKl3ikMmslA++zwqqpxJFX63Fn0l6ac ha2F4aWzDLQOg== From: sashiko-bot@kernel.org Subject: Re: [PATCH 3/4] rust: Add dma_fence abstractions Reply-To: sashiko-reviews@lists.linux.dev To: "Philipp Stanner" Cc: linux-media@vger.kernel.org, ojeda@kernel.org In-Reply-To: <20260530143541.229628-5-phasta@kernel.org> References: <20260530143541.229628-5-phasta@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sat, 30 May 2026 15:06:21 +0000 Message-Id: <20260530150622.393FC1F00893@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 4 potential issue(= s) to consider: - [Critical] DriverFence::drop eagerly drops FenceCtx and user data before = the C dma_fence refcount reaches 0, leading to a Use-After-Free. - [High] FenceCtx::drop unconditionally calls rcu_barrier(), causing a 'sch= eduling while atomic' panic when a DriverFence is dropped from an interrupt= handler or under a spinlock. - [High] DriverFence::drop() passes a positive error code (ECANCELED) to dm= a_fence_set_error(), triggering a WARN_ON and misconfiguring the error stat= e. - [High] DriverFence::as_raw and DriverFenceBorrow::as_raw create exclusive= mutable references (&mut) to actively shared data, causing Rust aliasing U= ndefined Behavior. -- commit bf1d1a15337629cfe74eaeb7d34f6878de552428 Author: Philipp Stanner Subject: rust: Add dma_fence abstractions This commit introduces Rust abstractions for the dma_fence framework. It provides DriverFence, Fence, and FenceCtx types to decouple fence issuers from consumers and safely manage synchronization primitives required by GPU drivers. > diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_f= ence.rs > new file mode 100644 > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > +#[pinned_drop] > +impl PinnedDrop for FenceCtx { > + fn drop(self: Pin<&mut Self>) { > + // SAFETY: `rcu_barrier()` is always safe to be called. > + unsafe { bindings::rcu_barrier() }; [Severity: High] Does calling rcu_barrier() here risk a scheduling-while-atomic panic? Since DriverFence::signal() consumes self, the DriverFence will be dropped at the end of the method. If this happens in an atomic context (such as an interrupt handler or under a spinlock), and it holds the last reference to the FenceCtx, dropping it triggers FenceCtx::drop(). Because rcu_barrier() blocks and sleeps until all pending RCU callbacks complete, this could attempt to sleep in atomic context. > + } > +} [ ... ] > +impl DriverFence { > + fn as_raw(&self) -> *mut bindings::dma_fence { > + // SAFETY: Valid because `self` is valid. > + let fence_data =3D unsafe { &mut *self.data.as_ptr() }; [Severity: High] Does this create an exclusive mutable reference to actively shared memory? DriverFenceData can be accessed concurrently by other threads holding Fence references (for instance, when checking if the fence is signaled). Creating a mutable reference (&mut) in Rust asserts exclusive access and violates aliasing rules, which allows the compiler to make invalid optimization assumptions. Could this use an immutable reference &*self.data.as_ptr() instead? > + > + fence_data.inner.inner.get() > + } [ ... ] > +impl DriverFenceBorrow { > + fn as_raw(&self) -> *mut bindings::dma_fence { > + // SAFETY: Valid because `self` is valid. > + let fence_data =3D unsafe { &mut *self.data.as_ptr() }; [Severity: High] Similar to DriverFence::as_raw(), does this also incorrectly create a mutable reference to shared data? > + > + fence_data.inner.inner.get() > + } [ ... ] > +impl Drop for DriverFence { > + fn drop(&mut self) { > + let fence =3D self.as_raw(); > + let mut fence_flags: usize =3D 0; > + let flag_ptr =3D &raw mut fence_flags; > + > + // 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 the `DriverFence` has be= en dropped. > + unsafe { > + bindings::dma_fence_lock_irqsave(fence, flag_ptr); > + #[allow(unused_unsafe)] > + if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) { > + bindings::dma_fence_set_error(fence, ECANCELED as i32); [Severity: High] Is it intentional to pass a positive error code here? bindings::ECANCELED evaluates to a positive integer, but dma_fence_set_error() expects a negative error code (and triggers a WARN_ON for positive values). A positive value might also be interpreted as success by callers checking for values < 0. Should this be - (bindings::ECANCELED as i32)? > + bindings::dma_fence_signal_locked(fence); > + } > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr); > + } > + > + // SAFETY: `self.data` is owned by the DriverFence, but could be= accessed > + // through some dma_fence callbacks right now. Access is being r= evoked > + // above by signalling the fence. The DriverFenceAllowedData tra= it > + // ensures that the data either does not need drop, or if it doe= s it > + // lives in a RcuBox which will delay dropping by one grace peri= od, hence > + // ensuring that all readers have disappeared. > + unsafe { drop_in_place(self.data.as_ptr()) }; [Severity: Critical] Could this eagerly drop FenceCtx and user data before the C dma_fence refcount reaches 0? The C dma_fence object can remain alive as long as consumers (like sync_file or the GPU scheduler) hold references to it. If the backend later invokes callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would attempt to read fctx.driver_name from the already-dropped FenceCtx: FenceCtx::get_driver_name() { ... fctx.driver_name.as_char_ptr() } Can this result in a use-after-free? > + > + // 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 the `DriverFence` has be= en dropped. > + unsafe { > + bindings::dma_fence_put(fence); > + } --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260530143541.2296= 28-2-phasta@kernel.org?part=3D3