From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=82.65.109.163 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793367104; b=AyT83XDb4Axq6K6JWF8bLkyZmbdPigithlv546hBxaJjSrNZrPny4unCUaO6paMSEEZ6 WtxEXQUoJdGLuhIeNDoUrWw88Ge4SqrDCtIGMtKL2S8uWMpxXUgDBaZb+g0JwBs5OPBlI 4zsde0dxKsXRTkKgpJqSWJ8E0WU/wp02Ldz497YQMySYtq08bRdYI9SBf/QYxKbGQg4iu IuODsjcxmKLcS39uNl+WDJojb4JsV3x7hI8w8mAZrNK+zuVVifnqeZzWUo671CLvTQ2Es N5B8Ub7d6Q5UL+O7DIicxoae5aw9B2cfWiqcmysKXxLc7abjEv6YACQcYsbbmtdr/5w== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793367104; h=DMARC-Filter:Received:Date:From:To:Cc:Subject:Message-ID:References: MIME-Version:Content-Type:Content-Disposition: Content-Transfer-Encoding:In-Reply-To:Jabber-ID; bh=tlEiADi4+8g0idHc+u87QI/42EGWWf/uchgiFaBfvoE=; b=QQw/4plT/GntTUhDujWpG8LS0tb1fOTTi13zZB0djGlsfYowFiJL0b/Qsnt+EEIiaENl kztIlhMWTv6ar0hYwGCLTA02m/1kvIUHitea22+GSQCyzthG9l4EdE2TQejJo/VzhhUho og0sPN2iqvhCBfHKpf1TePgoL8YzkkDfGtHVIVo3yynXwphdwfJ9iwgp89htdQ52RyLWk nsSIk3lcy92/ZXT6JdOF7ANpdUCKjBWFmIerBUAbfbgZCASyiWiZhFJOkK0Hf1grkKO2I PvseZk6fr5oGN3jfjnQTQxnEwGbjXLMLxwApAhn4/iXqHzlG6jzVu7X4L3L1ClBvOpw== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none header.from=linkmauve.fr; spf=pass smtp.mailfrom=linkmauve.fr; arc=none smtp.remote-ip=82.65.109.163 Received: from linkmauve.fr (linkmauve.fr [82.65.109.163]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CBCC2606A32E; Fri, 30 Oct 2026 13:31:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org CBCC2606A32E Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=linkmauve.fr Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linkmauve.fr Received: by linkmauve.fr (Postfix, from userid 1000) id 91F417229E1B; Fri, 30 Oct 2026 14:31:40 +0100 (CET) Date: Fri, 30 Oct 2026 14:31:40 +0100 From: Link Mauve To: Konrad Dybcio Cc: Rob Clark , Dmitry Baryshkov , Abhinav Kumar , Jessica Zhang , Sean Paul , Marijn Suijten , David Airlie , Simona Vetter , Miguel Ojeda , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , Danilo Krummrich , Daniel Almeida , Tamir Duberstein , Alexandre Courbot , Onur =?iso-8859-1?Q?=D6zkan?= , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, rust-for-linux@vger.kernel.org, Konrad Dybcio Subject: Re: [PATCH RFC] drm/msm: move VM_BIND op validation to Rust Message-ID: References: <20261026-topic-vm_bind_rs-v1-1-6581465f2428@oss.qualcomm.com> 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-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20261026-topic-vm_bind_rs-v1-1-6581465f2428@oss.qualcomm.com> Jabber-ID: linkmauve@linkmauve.fr Hi, On Mon, Oct 26, 2026 at 12:40:02PM +0100, Konrad Dybcio wrote: > From: Konrad Dybcio > > Much of the VM handling code is a headache to read and maintain. > There's a soup of mixed-signedness/size types which need to be compared > and added and getting that wrong in C is very easy. > > The "(range + obj_offset) > obj->size" check could wrap for a large > obj_offset, which let an out-of-range map be accepted and then mapped > as nothing. Oxidize it and use checked_add() to fix that. > > This commit is a "get the foot in the door" type change to make sure > that folks building kernels for Qualcomm SoCs have their Rust setup > ready. It may/should be extended to the wider mm code once the > appropriate abstractions are finalized. > > Worth noting, the C UERR() macro expands to dev_dbg() and the Rust > implementation for that currently doesn't support dynamic_debug. > > The DRM_BRIDGE dependency is dropped because including RUST exposed > some spaghetti dependencies ending at DEBUGFS and it's enabled anyway. > > Signed-off-by: Konrad Dybcio > --- > (if this is rejected we may just patch the C code). > > Notably, this will regress some ancient configurations: > -- arch/arm/Kconfig > select HAVE_RUST if CPU_LITTLE_ENDIAN && CPU_32v7 && !KASAN > > Smoke-tested on Glymur > --- > drivers/gpu/drm/msm/Kconfig | 2 +- > drivers/gpu/drm/msm/Makefile | 1 + > drivers/gpu/drm/msm/msm_gem_vma.c | 66 +++------------ > drivers/gpu/drm/msm/msm_vm_bind.h | 16 ++++ > drivers/gpu/drm/msm/msm_vm_bind.rs | 160 +++++++++++++++++++++++++++++++++++++ > rust/uapi/uapi_helper.h | 1 + > 6 files changed, 188 insertions(+), 58 deletions(-) > […] > diff --git a/drivers/gpu/drm/msm/msm_vm_bind.rs b/drivers/gpu/drm/msm/msm_vm_bind.rs > new file mode 100644 > index 000000000000..164208e0a1b4 > --- /dev/null > +++ b/drivers/gpu/drm/msm/msm_vm_bind.rs > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +//! VM_BIND operation validation for the MSM driver. > + > +use kernel::{ > + bindings, > + device::Device, > + error::code::EINVAL, > + prelude::*, > + uapi, // > +}; > + > +/// Validates a single VM_BIND operation. > +/// > +/// Returns 0 if the operation is valid. Otherwise logs the reason and returns > +/// `-EINVAL`. > +/// > +/// `page_size` is the granularity that `iova`, `obj_offset` and `range` must be > +/// aligned to. It is the GPU's alignment rather than the CPU's, but no known qcom > +/// SMMU lacks support for the CPU's smallest page size. > +/// `range_valid` is the result of `drm_gpuvm_range_valid()` for this operation. > +/// `prr_supported` is only consulted for `MSM_VM_BIND_OP_MAP_NULL`. > +/// > +/// # Safety > +/// > +/// `dev` must point to a live `struct device`. > +#[no_mangle] > +pub unsafe extern "C" fn msm_vm_bind_check_op( > + dev: *mut bindings::device, > + op: u32, > + handle: u32, > + obj_offset: u64, > + iova: u64, > + range: u64, > + flags: u32, > + page_size: u64, > + range_valid: bool, > + prr_supported: bool, > +) -> c_int { > + // SAFETY: The caller guarantees that `dev` is a live `struct device`. > + let dev: &Device = unsafe { Device::from_raw(dev) }; > + > + match check_op( > + dev, > + op, > + handle, > + obj_offset, > + iova, > + range, > + flags, > + page_size, > + range_valid, > + prr_supported, > + ) { > + Ok(()) => 0, > + Err(e) => e.to_errno(), > + } I think we usually use kernel::error::from_result() instead of a match block in this situation. > +} > + > +fn check_op( > + dev: &Device, > + op: u32, > + handle: u32, > + obj_offset: u64, > + iova: u64, > + range: u64, > + flags: u32, > + page_size: u64, > + range_valid: bool, > + prr_supported: bool, > +) -> Result { > + let aligned = |addr: u64| page_size.is_power_of_two() && (addr & (page_size - 1)) == 0; > + > + if flags & !uapi::MSM_VM_BIND_OP_FLAGS != 0 { > + dev_dbg!(dev, "invalid flags: {:x}\n", flags); > + return Err(EINVAL); > + } > + > + if !aligned(iova) { > + dev_dbg!(dev, "invalid address: {:016x}\n", iova); > + return Err(EINVAL); > + } > + > + if !aligned(obj_offset) { > + dev_dbg!(dev, "invalid bo_offset: {:016x}\n", obj_offset); > + return Err(EINVAL); > + } > + > + if !aligned(range) { > + dev_dbg!(dev, "invalid range: {:016x}\n", range); > + return Err(EINVAL); > + } > + > + if !range_valid { > + dev_dbg!(dev, "invalid range: {:016x}, {:016x}\n", iova, range); > + return Err(EINVAL); > + } > + > + // MAP must name a handle; UNMAP and MAP_NULL must not. > + match op { > + uapi::MSM_VM_BIND_OP_MAP => { > + if handle == 0 { > + dev_dbg!(dev, "invalid handle\n"); > + return Err(EINVAL); > + } > + } > + uapi::MSM_VM_BIND_OP_UNMAP | uapi::MSM_VM_BIND_OP_MAP_NULL => { > + if handle != 0 { > + dev_dbg!(dev, "handle must be zero\n"); > + return Err(EINVAL); > + } > + } > + _ => { > + dev_dbg!(dev, "invalid op: {}\n", op); > + return Err(EINVAL); > + } > + } > + > + if op == uapi::MSM_VM_BIND_OP_MAP_NULL && !prr_supported { > + dev_dbg!(dev, "PRR not supported\n"); > + return Err(EINVAL); > + } > + > + Ok(()) > +} > + > +/// Checks that `[obj_offset, obj_offset + range)` lies within an object of > +/// `obj_size` bytes. > +/// > +/// Returns 0 if it does. Otherwise logs the reason and returns `-EINVAL`. > +/// > +/// # Safety > +/// > +/// `dev` must point to a live `struct device`. > +#[no_mangle] > +pub unsafe extern "C" fn msm_vm_bind_check_bo_range( > + dev: *mut bindings::device, > + obj_offset: u64, > + range: u64, > + obj_size: u64, > +) -> c_int { > + // SAFETY: The caller guarantees that `dev` is a live `struct device`. > + let dev: &Device = unsafe { Device::from_raw(dev) }; > + > + let fits = obj_offset > + .checked_add(range) > + .is_some_and(|end| end <= obj_size); > + > + if fits { > + return 0; > + } > + > + dev_dbg!( > + dev, > + "invalid range: {:016x} + {:016x} > {:016x}\n", > + range, > + obj_offset, > + obj_size > + ); > + EINVAL.to_errno() Same here, the whole content of this function could be wrapped in a from_result(). > +} […] Thanks, -- Link Mauve