From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) (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 07C74214800 for ; Mon, 3 Mar 2025 15:45:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741016708; cv=none; b=JvZR9ZstRgRHFnNOUPKwBL1ho8/dDiN5zsKsTUzwz5INQaW8hUYUDtPDMBJSVEBed7+wpjv3K3VDgAoE1XsDAQz+dnX6oBPioJVc+CgO++q7g1bjHGqniWUo5Nz9Vt8/juHfkGMcfa9BUUJJIuYiDccgrIMI2JoYW5H88HV63lM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741016708; c=relaxed/simple; bh=TvEt7HL2f2+ZiPYpZgA9Wv//CyB2sGr83sClfxce/Js=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=ciJO+sPUU0vpN8Zd52Ph4peptJcriERWxWEKwB5JYX8aV4uA4vvC0ILQPbhCh7xHUb8nRCc7Rqc8VDYQAvyGSnE7WQbbHYaYOcRr9rfBjqC3dTMwK7eYjA2R7BDvz64QNTyxAtlzdry4r3YZTaMaIQnFRPc8M+oy8uPfO/aZhvw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=s1zlH7O0; arc=none smtp.client-ip=209.85.128.48 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=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="s1zlH7O0" Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-4394345e4d5so31853875e9.0 for ; Mon, 03 Mar 2025 07:45:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1741016704; x=1741621504; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=3kow4xuk+1mNeQ4oexSjSmUNlEIdhe4RIwb0wGDmCC8=; b=s1zlH7O0h915hMWBG4aPJ0ZiB9mvsECjZ+YtIAz5FaT00pxSVz9vLtf4tnOISz7YmK qP2sFtt76VC2xdOkapqpeGY3RDX9ABhuvnD+K8sSpypE6cDyexjy9yajNNgmAfXJ5i2c CvpDFeRW999FXUN6ebuHHw+hA/IIlKZrOvDqN5h2CLee1ZoX+kcA/QjldpIx1Ev2owjy dOrfSrtC/9m2lZ6uN1cBDH8tpTm06ymFFmozowsX/yw7VVHmYQ06CvHGFhxFEQU6KaGD NM8QCoKGeKNzM5Z64x+LzlZdUpaC9avyb3/FOXCtohL/fdisgyv+9rMQ0V/jxKkF1y5v 4lBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741016704; x=1741621504; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3kow4xuk+1mNeQ4oexSjSmUNlEIdhe4RIwb0wGDmCC8=; b=Ys/juTtLw1rcd/wFjvvLuxZ1jsPUhc7U9EK4L43eTFdiNE3YoDmaOSnMeb+v1YBGo+ V4hUasFwzAHPREjYc7oYFpM8tMzgS+OymYHX8GOWCC7RH/BwkNXXfdy1NtBT+DQ2/Umr y2ra6K19TK0Z0IwcYeQFgnuV+ZQ/uQDHq4gQkzLzSHRaIhRlwvQ69c3Yw2rl2v2qbtWu eWjk3vQV920wvoRUjoDs8JTFLsV6c/bz7DZQzATqmqcMq8ZW+0I0/4CbTwelv/rAxd9n Ovwk59VMwq+Je3pzCD07ihUxtdRPAAe0XOqJTwPX4hevAnEhpfMFQchwXj1KgIrkNmrm IJYA== X-Forwarded-Encrypted: i=1; AJvYcCWus+89G3EyPWiXSRFUcHbGPH8x6vCT0P0uNjeUtMrdga3WSoVaSBpx6F3q/VvH61hdgxIi6gQLdX9C59FcpQ==@vger.kernel.org X-Gm-Message-State: AOJu0YwFF7Q6S5+D+m4FqI9XvaXfaG1j3vSz7XiX5HBNe1P3zHI8RLK2 /BghJFk6o/XgZSCigee5Vrmw/HB0u3/xJix1g0ovPUA7j0Hor5cbQNAwp75iSIvdiXvz44SJrVM EveuGffEM6/qhPxY4VXSiIzWGg0chG9MHZJay X-Gm-Gg: ASbGncswGuyOOJyf9WsYKqv/XTHVCl2uHNjSLjVpnJrpMarP/zqePthuUKiU0G9M59M H/8rd+5DFHeaz3r33jvw5HTYlJ7Tkn6jpzJPxdMykJpC+kQc90JzTCP8XAVRqPx1sOk93EAuDX9 ewtOfK+NS9sg0FPtQNY+ujxQpDo23wgUesUn35A41YnJsUkeFyQ40an4UQ X-Google-Smtp-Source: AGHT+IHXXQ7buQBEorS4efQ1j2iNuWmeS1bZKP0/m94hhxiT8OFf6H7yU2EGk9/SwAGsuWxPOOcRN2PU93WYRcIT0a8= X-Received: by 2002:a05:6000:270f:b0:390:dfe0:1320 with SMTP id ffacd0b85a97d-390ec9c1bddmr8951802f8f.33.1741016704234; Mon, 03 Mar 2025 07:45:04 -0800 (PST) Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20250224115007.2072043-1-abdiel.janulgue@gmail.com> <20250224115007.2072043-3-abdiel.janulgue@gmail.com> <6dea7b6a-1534-47e7-94d2-d67417c3d4c1@proton.me> <3202F69F-397E-4BC4-8DD8-E2D4B0AB056F@collabora.com> <87bjuil15w.fsf@kernel.org> <87ikoqjg1n.fsf@kernel.org> In-Reply-To: <87ikoqjg1n.fsf@kernel.org> From: Alice Ryhl Date: Mon, 3 Mar 2025 16:44:51 +0100 X-Gm-Features: AQ5f1JpQwVPMNAtyLQEi5zozvB7zbblDDYl0-W2nDZDSA_G1jc6Lh4mO8DNqPys Message-ID: Subject: Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction. To: Andreas Hindborg Cc: Daniel Almeida , Benno Lossin , Abdiel Janulgue , dakr@kernel.org, robin.murphy@arm.com, rust-for-linux@vger.kernel.org, Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Trevor Gross , Valentin Obst , linux-kernel@vger.kernel.org, Christoph Hellwig , Marek Szyprowski , airlied@redhat.com, iommu@lists.linux.dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Mar 3, 2025 at 4:21=E2=80=AFPM Andreas Hindborg wrote: > > "Alice Ryhl" writes: > > > On Mon, Mar 3, 2025 at 2:00=E2=80=AFPM Andreas Hindborg wrote: > >> > >> "Daniel Almeida" writes: > >> > >> > Hi Benno, > >> > > >> > >> [...] > >> > >> >>> + /// Writes data to the region starting from `offset`. `offset= ` is in units of `T`, not the > >> >>> + /// number of bytes. > >> >>> + /// > >> >>> + /// # Examples > >> >>> + /// > >> >>> + /// ``` > >> >>> + /// # fn test(alloc: &mut kernel::dma::CoherentAllocation= ) -> Result { > >> >>> + /// let somedata: [u8; 4] =3D [0xf; 4]; > >> >>> + /// let buf: &[u8] =3D &somedata; > >> >>> + /// alloc.write(buf, 0)?; > >> >>> + /// # Ok::<(), Error>(()) } > >> >>> + /// ``` > >> >>> + pub fn write(&self, src: &[T], offset: usize) -> Result { > >> >>> + let end =3D offset.checked_add(src.len()).ok_or(EOVERFLOW= )?; > >> >>> + if end >=3D self.count { > >> >>> + return Err(EINVAL); > >> >>> + } > >> >>> + // SAFETY: > >> >>> + // - The pointer is valid due to type invariant on `Coher= entAllocation` > >> >>> + // and we've just checked that the range and index is wit= hin bounds. > >> >>> + // - `offset` can't overflow since it is smaller than `se= lfcount` and we've checked > >> >>> + // that `self.count` won't overflow early in the construc= tor. > >> >>> + unsafe { > >> >>> + core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu= _addr.add(offset), src.len()) > >> >> > >> >> Why are there no concurrent write or read operations on `cpu_addr`? > >> > > >> > Sorry, can you rephrase this question? > >> > >> This write is suffering the same complications as discussed here [1]. > >> There are multiple issues with this implementation. > >> > >> 1) `write` takes a shared reference and thus may be called concurrentl= y. > >> There is no synchronization, so `copy_nonoverlapping` could be called > >> concurrently on the same address. The safety requirements for > >> `copy_nonoverlapping` state that the destination must be valid for > >> write. Alice claims in [1] that any memory area that experience data > >> races are not valid for writes. So the safety requirement of > >> `copy_nonoverlapping` is violated and this call is potential UB. > >> > >> 2) The destination of this write is DMA memory. It could be concurrent= ly > >> modified by hardware, leading to the same issues as 1). Thus the > >> function cannot be safe if we cannot guarantee hardware will not write > >> to the region while this function is executing. > >> > >> Now, I don't think that these _should_ be issues, but according to our > >> Rust language experts they _are_. > >> > >> I really think that copying data through a raw pointer to or from a > >> place that experiences data races, should _not_ be UB if the data is n= ot > >> interpreted in any way, other than moving it. > >> > >> > >> Best regards, > >> Andreas Hindborg > > > > We need to make progress on this series, and it's starting to get late > > in the cycle. I suggest we: > > There is always another cycle. > > > > > 1. Delete as_slice, as_slice_mut, write, and skip_drop. > > 2. Change field_read/field_write to use a volatile read/write. > > Volatile reads/writes that race are OK? I will not give a blanket yes to that. If you read their docs, you will find that they claim to not allow it. But they are the correct choice for DMA memory, and there's no way in practice to get miscompilations on memory locations that are only accessed with volatile operations, and never have references to them created. In general, this will fall into the exception that we've been given from the Rust people. In cases such as this where the Rust language does not give us the operation we want, do it like you do in C. Since Rust uses LLVM which does not miscompile the C part of the kernel, it should not miscompile the Rust part either. > > This will let us make progress now and sidestep this discussion. The > > deleted methods can happen in a follow-up. > > `item_from_index`, the `dma_read` and `dma_write` macros as well, I would= think? Those are necessary to use field_read/field_write, so I think it's fine to keep those. Alice