All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Timur Tabi" <ttabi@nvidia.com>
Cc: "gary@garyguo.net" <gary@garyguo.net>,
	"mmaurer@google.com" <mmaurer@google.com>,
	"rust-for-linux@vger.kernel.org" <rust-for-linux@vger.kernel.org>,
	"nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
	Joel Fernandes <joelagnelf@nvidia.com>,
	"aliceryhl@google.com" <aliceryhl@google.com>,
	Alexandre Courbot <acourbot@nvidia.com>
Subject: Re: [PATCH v7 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace
Date: Tue, 10 Mar 2026 20:56:53 +0100	[thread overview]
Message-ID: <DGZD8D623QRZ.BXYEDETBVH84@kernel.org> (raw)
In-Reply-To: <4452ba874cae2d6dfa64b9625e4e5f2ab7f34559.camel@nvidia.com>

On Tue Mar 10, 2026 at 8:52 PM CET, Timur Tabi wrote:
> On Mon, 2026-03-09 at 20:59 +0100, Danilo Krummrich wrote:
>
>> > +    /// Low-level write from a raw pointer. Caller must ensure ptr is valid for `len` bytes.
>> > +    fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
>> 
>> The method has to be unsafe as the caller has to promise that ptr is indeed a
>> slice with len elements.
>
> Ok.
>
>> Another option would be to pass a fat pointer, i.e. *const [u8]. write_dma()
>> would then need to use ptr::slice_from_raw_parts() and the safety requirement of
>> this function becomes that ptr simply has to be valid.
>
> So I tried this approach, but the end result was that write_raw() and write_slice() were practically
> identical.

That sounds wrong, the only thing that write_slice() needs to do is to forward
to write_raw(), that's a one-liner and not different to what you have in your
patch currently?

> At this point, why bother with write_raw() -- just have write_dma() call write_slice():
>
>         let src_ptr = unsafe { alloc.start_ptr().add(offset) };
>         let slice = unsafe { core::slice::from_raw_parts(src_ptr, count) };
>         self.write_slice(slice)
>
> I think this is better, but I wanted to get your opinion before I posted a v8 with this change.

The problem is that creating a slice of a DMA buffer, that is used by a device
concurrently, is undefined behavior. Hence the indirection through write_raw().

WARNING: multiple messages have this Message-ID (diff)
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Timur Tabi" <ttabi@nvidia.com>
Cc: "John Hubbard" <jhubbard@nvidia.com>,
	"gary@garyguo.net" <gary@garyguo.net>,
	"mmaurer@google.com" <mmaurer@google.com>,
	"rust-for-linux@vger.kernel.org" <rust-for-linux@vger.kernel.org>,
	"nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
	"Joel Fernandes" <joelagnelf@nvidia.com>,
	"aliceryhl@google.com" <aliceryhl@google.com>,
	"Alexandre Courbot" <acourbot@nvidia.com>
Subject: Re: [PATCH v7 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace
Date: Tue, 10 Mar 2026 20:56:53 +0100	[thread overview]
Message-ID: <DGZD8D623QRZ.BXYEDETBVH84@kernel.org> (raw)
In-Reply-To: <4452ba874cae2d6dfa64b9625e4e5f2ab7f34559.camel@nvidia.com>

On Tue Mar 10, 2026 at 8:52 PM CET, Timur Tabi wrote:
> On Mon, 2026-03-09 at 20:59 +0100, Danilo Krummrich wrote:
>
>> > +    /// Low-level write from a raw pointer. Caller must ensure ptr is valid for `len` bytes.
>> > +    fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
>> 
>> The method has to be unsafe as the caller has to promise that ptr is indeed a
>> slice with len elements.
>
> Ok.
>
>> Another option would be to pass a fat pointer, i.e. *const [u8]. write_dma()
>> would then need to use ptr::slice_from_raw_parts() and the safety requirement of
>> this function becomes that ptr simply has to be valid.
>
> So I tried this approach, but the end result was that write_raw() and write_slice() were practically
> identical.

That sounds wrong, the only thing that write_slice() needs to do is to forward
to write_raw(), that's a one-liner and not different to what you have in your
patch currently?

> At this point, why bother with write_raw() -- just have write_dma() call write_slice():
>
>         let src_ptr = unsafe { alloc.start_ptr().add(offset) };
>         let slice = unsafe { core::slice::from_raw_parts(src_ptr, count) };
>         self.write_slice(slice)
>
> I think this is better, but I wanted to get your opinion before I posted a v8 with this change.

The problem is that creating a slice of a DMA buffer, that is used by a device
concurrently, is undefined behavior. Hence the indirection through write_raw().

  reply	other threads:[~2026-03-10 19:57 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-03 22:47 [PATCH v7 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
2026-02-03 22:47 ` [PATCH v7 1/7] rust: device: add device name method Timur Tabi
2026-02-03 22:47 ` [PATCH v7 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace Timur Tabi
2026-02-04  2:06   ` kernel test robot
2026-02-04 20:01     ` Timur Tabi
2026-03-09 19:59   ` Danilo Krummrich
2026-03-09 19:59     ` Danilo Krummrich
2026-03-10 19:52     ` Timur Tabi
2026-03-10 19:52       ` Timur Tabi
2026-03-10 19:56       ` Danilo Krummrich [this message]
2026-03-10 19:56         ` Danilo Krummrich
2026-03-10 20:11         ` Timur Tabi
2026-03-10 20:11           ` Timur Tabi
2026-03-10 20:01       ` Alice Ryhl
2026-02-03 22:47 ` [PATCH v7 3/7] rust: dma: implement BinaryWriter for CoherentAllocation<u8> Timur Tabi
2026-02-03 22:47 ` [PATCH v7 4/7] gpu: nova-core: Replace module_pci_driver! with explicit module init Timur Tabi
2026-02-03 22:47 ` [PATCH v7 5/7] gpu: nova-core: use pin projection in method boot() Timur Tabi
2026-02-03 22:47 ` [PATCH v7 6/7] gpu: nova-core: create debugfs root in module init Timur Tabi
2026-02-03 22:47 ` [PATCH v7 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries Timur Tabi
2026-02-10  3:28 ` [PATCH v7 0/7] gpu: nova-core: expose the logging buffers via debugfs John Hubbard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DGZD8D623QRZ.BXYEDETBVH84@kernel.org \
    --to=dakr@kernel.org \
    --cc=acourbot@nvidia.com \
    --cc=aliceryhl@google.com \
    --cc=gary@garyguo.net \
    --cc=joelagnelf@nvidia.com \
    --cc=mmaurer@google.com \
    --cc=nouveau@lists.freedesktop.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=ttabi@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.