From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f73.google.com (mail-wm1-f73.google.com [209.85.128.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CB91639A049 for ; Wed, 29 Apr 2026 08:03:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.73 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777449787; cv=none; b=UL7JaUH6wvMzaiqqqG2vnP1H9sTWYWaSw7nLAMYy0gf6idEE3oXi1EDggkj2b8dLVunCa3lgTdoHrzaWnbjdMseKLuQsaxm71o3iFVvaPhr4BkCHIFHKhaQJzisD9tDL/e66vwklNSPa9kKHRjeb6cZAxJWiqt4TR0RqRRLICPI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777449787; c=relaxed/simple; bh=LN67d/FJzMppHHtbV9T5QweCRhIfpdDesj7GgoJjdpc=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=A1ew3LJKAARGVRmo+iQkqJpkgCgLaZHwDpwlu+L0hFq+exha3j5805YaxirsnNKT55DBDCxkfTH9LOM0MeVgho7kAuNLFGHFfWITDnPkDNRvTQ09Ab963ql+OUEjOWLJHdcuJ92vMNAngyzlDermfpJCNBT4DN8NTH77T/Iknno= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=c2rJ/PW/; arc=none smtp.client-ip=209.85.128.73 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="c2rJ/PW/" Received: by mail-wm1-f73.google.com with SMTP id 5b1f17b1804b1-488c0fcc6deso79177435e9.2 for ; Wed, 29 Apr 2026 01:03:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1777449784; x=1778054584; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=VEG/I8pPPYBATQ/hJAtxhRYJOnBvxSA+K2ZLFC/1AAs=; b=c2rJ/PW/OjYoI8Yir9/ZOjSeAH4c1yyuMQgCIS1+kLoW5UKPMNfqxbOcEv+QSAEBnL 2eQtgAxqIdtyiFJdjAHmmWXQFkLF+DiVGdPHgYahkhQDW8LnvUK0mTeY6IJdQuZjTCcb FxrktJOeka0NC5fDH9V2j+d5mW0LQ2BbMPRRDORMvUHwIHz/zHbT4diwlV3a+hLkKEs9 dPn/HVtFuMVguK2CDitUYoHVYtBYTmXFfiDSXfLfBz4i62LvNyKLYEPwYwZcaTKppzUd BUfCVYzmNT0gjTRY8RiL5SokkWPnaJa7OSkZnzwnqHj/L9EWC/3yvdBoUmR9cSW3JbIY M58w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777449784; x=1778054584; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VEG/I8pPPYBATQ/hJAtxhRYJOnBvxSA+K2ZLFC/1AAs=; b=LkftTyzrYT6tXuPWqAD+84kiO8Av1NW1TzCL3kwgED5z/509bxMXnNAYTXbNfM1CNZ FyFPnVYIoLXVFELV0ejoNydp+dSYvX4NCr3LCuhrllhfEM2jVTfkkeTke4IhK66rk/bJ +miPc0R3rv4xO7+280vKpHDP1PZoW0fdj8X/whw/FFJrBTAR9hAsSj8kqWxCNYdGbt2u bCgr+IYzLUFjuEdyvk7As81T1pSmCCf8m2bU19aa7b3RVHXZ8FIAaq/O1MKuIC9aTEze LG04+5W4T/yf+lm6JoBlWR6SSdEGKO8tvOIOlmi1g7YQBKw9Qxn/nIxo59iPI/hqAyeZ lqPQ== X-Forwarded-Encrypted: i=1; AFNElJ9JktbxIpb2lQiPCElvLaUxV7dxCaVSKpuUzMGQcmSUKgvFIEnKLGA3wXp/Hdd9/uwEIXd9HD5iVjZnTQ8=@vger.kernel.org X-Gm-Message-State: AOJu0YwmnrQrSgdBHLkvb5oafkUSDdqY1euEvNkCRA6UAct1E8sIZMXH Jl1TxD+pdRZIE4C1K1LORz8+0F/fDMsvy5mlyD17AGkMau02FA6xHXWve8tjEqN1JE/uhP/Rhdx HdZqH1mH4WUkd7RChOw== X-Received: from wmdd17.prod.google.com ([2002:a05:600c:a211:b0:488:c696:8bf2]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:600c:1d1a:b0:489:201c:dc46 with SMTP id 5b1f17b1804b1-48a77b04774mr98771505e9.12.1777449784107; Wed, 29 Apr 2026 01:03:04 -0700 (PDT) Date: Wed, 29 Apr 2026 08:03:02 +0000 In-Reply-To: <20260428-fix-drm-1-v1-1-755057178066@nvidia.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260428-fix-drm-1-v1-1-755057178066@nvidia.com> Message-ID: Subject: Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new From: Alice Ryhl To: Eliot Courtney Cc: David Airlie , Simona Vetter , Danilo Krummrich , Miguel Ojeda , Boqun Feng , Gary Guo , "=?utf-8?B?QmrDtnJu?= Roy Baron" , Benno Lossin , Andreas Hindborg , Trevor Gross , Alexandre Courbot , dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" On Tue, Apr 28, 2026 at 09:20:21PM +0900, Eliot Courtney wrote: > If pinned initialization of drm::Device::Data fails, it calls > drm::Device::release via drm_dev_put. This materializes a reference to > &drm::Device, but it's not fully constructed yet, because initializing > `data` failed. It should not be dropped either. Instead, if pinned > initialization fails, make sure drm::Device::release isn't called. > > Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()") > Signed-off-by: Eliot Courtney For the concerns about duplicating vtables, could the temporary vtable be a stack variable? > rust/kernel/drm/device.rs | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs > index adbafe8db54d..78ea0eb12535 100644 > --- a/rust/kernel/drm/device.rs > +++ b/rust/kernel/drm/device.rs > @@ -111,6 +111,13 @@ impl Device { > fops: &Self::GEM_FOPS, > }; > > + // Use a vtable without a `release` callback until `data` is initialized, so init failure > + // can release the DRM device without dropping uninitialized fields. > + const ALLOC_VTABLE: bindings::drm_driver = bindings::drm_driver { > + release: None, > + ..Self::VTABLE > + }; > + > const GEM_FOPS: bindings::file_operations = drm::gem::create_fops(); > > /// Create a new `drm::Device` for a `drm::Driver`. > @@ -120,12 +127,12 @@ pub fn new(dev: &device::Device, data: impl PinInit) -> Result let layout = Kmalloc::aligned_layout(Layout::new::()); > > // SAFETY: > - // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation, > + // - `ALLOC_VTABLE`, as a `const` is pinned to the read-only section of the compilation, > // - `dev` is valid by its type invarants, > let raw_drm: *mut Self = unsafe { > bindings::__drm_dev_alloc( > dev.as_raw(), > - &Self::VTABLE, > + &Self::ALLOC_VTABLE, > layout.size(), > mem::offset_of!(Self, dev), > ) > @@ -133,6 +140,10 @@ pub fn new(dev: &device::Device, data: impl PinInit) -> Result .cast(); > let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?; > > + // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was > + // successful. > + let drm_dev = unsafe { Self::into_drm_device(raw_drm) }; > + > // SAFETY: `raw_drm` is a valid pointer to `Self`. > let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) }; > > @@ -140,15 +151,14 @@ pub fn new(dev: &device::Device, data: impl PinInit) -> Result // - `raw_data` is a valid pointer to uninitialized memory. > // - `raw_data` will not move until it is dropped. > unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| { > - // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was > - // successful. > - let drm_dev = unsafe { Self::into_drm_device(raw_drm) }; > - > // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the > // refcount must be non-zero. > unsafe { bindings::drm_dev_put(drm_dev) }; > })?; > > + // SAFETY: `drm_dev` is still private to this function. > + unsafe { (*drm_dev).driver = &Self::VTABLE }; It would be bad if this ended up being a reference to a local variable. Please use `&const { Self::VTABLE }` so that it doesn't compile if this occurs. Alice