All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Lyude Paul" <lyude@redhat.com>
Cc: <nouveau@lists.freedesktop.org>, "Gary Guo" <gary@garyguo.net>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	<rust-for-linux@vger.kernel.org>,
	"Danilo Krummrich" <dakr@kernel.org>,
	<dri-devel@lists.freedesktop.org>,
	"Matthew Maurer" <mmaurer@google.com>,
	"FUJITA Tomonori" <fujita.tomonori@gmail.com>,
	"Lorenzo Stoakes" <lorenzo.stoakes@oracle.com>,
	<christian.koenig@amd.com>, "Asahi Lina" <lina@asahilina.net>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Boqun Feng" <boqun@kernel.org>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Krishna Ketan Rai" <prafulrai522@gmail.com>,
	<linux-media@vger.kernel.org>,
	"Shankari Anand" <shankari.ak0208@gmail.com>,
	"David Airlie" <airlied@gmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Viresh Kumar" <viresh.kumar@linaro.org>,
	<linaro-mm-sig@lists.linaro.org>,
	"Asahi Lina" <lina+kernel@asahilina.net>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	<kernel@vger.kernel.org>
Subject: Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable
Date: Fri, 24 Apr 2026 20:08:14 +0900	[thread overview]
Message-ID: <DI1C648G6OWI.20O3JB8VS59PB@nvidia.com> (raw)
In-Reply-To: <20260421235346.672794-5-lyude@redhat.com>

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:
<snip>
> @@ -176,6 +195,100 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
>          let _ = unsafe { KBox::from_raw(this) };
>      }
> +
> +    // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
> +    // that it is unmapped on driver unbind.
> +    fn get_sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,

Just noticed that the caller can technically pass a different `dev` from
the one the buffer will be mapped onto, potentially linking the `Devres`
(and thus the device mapping) to a completely unrelated lifetime.

I.e. `dev` is only used to create the `Devres`, while
`drm_gem_shmem_get_pages_sgt_locked` uses the object's device pointer to
create the mapping. We need to add the check ensuring that the two point
to the same device:

    let expected = self.dev().as_ref().as_raw();
    if dev.as_raw() != expected {
        return Err(EINVAL);
    }

Ideally we could do without the `dev` argument altogether, but that's
our only guarantee that the device is bound, so I don't think we can do
without this runtime check.

I think it's also worth mentioning in the documentations of `sg_table`
and `owned_sg_table` which device is expected.

WARNING: multiple messages have this Message-ID (diff)
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Lyude Paul" <lyude@redhat.com>
Cc: nouveau@lists.freedesktop.org, Gary Guo <gary@garyguo.net>,
	Daniel Almeida <daniel.almeida@collabora.com>,
	rust-for-linux@vger.kernel.org,
	Danilo Krummrich <dakr@kernel.org>,
	dri-devel@lists.freedesktop.org,
	Matthew Maurer <mmaurer@google.com>,
	FUJITA Tomonori <fujita.tomonori@gmail.com>,
	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>,
	christian.koenig@amd.com, Asahi Lina <lina@asahilina.net>,
	Miguel Ojeda <ojeda@kernel.org>,
	Andreas Hindborg <a.hindborg@kernel.org>,
	Simona Vetter <simona@ffwll.ch>,
	Alice Ryhl <aliceryhl@google.com>, Boqun Feng <boqun@kernel.org>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Krishna Ketan Rai <prafulrai522@gmail.com>,
	linux-media@vger.kernel.org,
	Shankari Anand <shankari.ak0208@gmail.com>,
	Benno Lossin <lossin@kernel.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	linaro-mm-sig@lists.linaro.org,
	Asahi Lina <lina+kernel@asahilina.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	kernel@vger.kernel.org
Subject: Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable
Date: Fri, 24 Apr 2026 20:08:14 +0900	[thread overview]
Message-ID: <DI1C648G6OWI.20O3JB8VS59PB@nvidia.com> (raw)
In-Reply-To: <20260421235346.672794-5-lyude@redhat.com>

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:
<snip>
> @@ -176,6 +195,100 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
>          let _ = unsafe { KBox::from_raw(this) };
>      }
> +
> +    // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
> +    // that it is unmapped on driver unbind.
> +    fn get_sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,

Just noticed that the caller can technically pass a different `dev` from
the one the buffer will be mapped onto, potentially linking the `Devres`
(and thus the device mapping) to a completely unrelated lifetime.

I.e. `dev` is only used to create the `Devres`, while
`drm_gem_shmem_get_pages_sgt_locked` uses the object's device pointer to
create the mapping. We need to add the check ensuring that the two point
to the same device:

    let expected = self.dev().as_ref().as_raw();
    if dev.as_raw() != expected {
        return Err(EINVAL);
    }

Ideally we could do without the `dev` argument altogether, but that's
our only guarantee that the device is bound, so I don't think we can do
without this runtime check.

I think it's also worth mentioning in the documentations of `sg_table`
and `owned_sg_table` which device is expected.

  parent reply	other threads:[~2026-04-24 11:08 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-21 23:52 [PATCH v12 0/5] Rust bindings for gem shmem Lyude Paul
2026-04-21 23:52 ` Lyude Paul
2026-04-21 23:52 ` [PATCH v12 1/5] rust: drm: gem: s/device::Device/Device/ for shmem.rs Lyude Paul
2026-04-21 23:52   ` Lyude Paul
2026-04-21 23:52 ` [PATCH v12 2/5] drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked() Lyude Paul
2026-04-21 23:52   ` Lyude Paul
2026-04-22 22:52   ` lyude
2026-04-22 22:52     ` lyude
2026-04-21 23:52 ` [PATCH v12 3/5] drm/gem/shmem: Export drm_gem_shmem_get_pages_sgt_locked() Lyude Paul
2026-04-21 23:52   ` Lyude Paul
2026-04-21 23:52 ` [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable Lyude Paul
2026-04-21 23:52   ` Lyude Paul
2026-04-23 15:01   ` Alexandre Courbot
2026-04-23 15:01     ` Alexandre Courbot
2026-04-23 15:09     ` Gary Guo
2026-04-23 15:09       ` Gary Guo
2026-04-23 15:27       ` Alexandre Courbot
2026-04-23 15:27         ` Alexandre Courbot
2026-04-23 16:13         ` Gary Guo
2026-04-23 16:13           ` Gary Guo
2026-04-24  2:10           ` Alexandre Courbot
2026-04-24  2:10             ` Alexandre Courbot
2026-04-23 15:28     ` Alexandre Courbot
2026-04-23 15:28       ` Alexandre Courbot
2026-04-24 18:13     ` lyude
2026-04-24 18:13       ` lyude
2026-04-24 23:10     ` lyude
2026-04-24 23:10       ` lyude
2026-04-25  2:38       ` Alexandre Courbot
2026-04-25  2:38         ` Alexandre Courbot
2026-04-24 11:08   ` Alexandre Courbot [this message]
2026-04-24 11:08     ` Alexandre Courbot
2026-04-21 23:52 ` [PATCH v12 5/5] rust: drm: gem: Add vmap functions to shmem bindings Lyude Paul
2026-04-21 23:52   ` Lyude Paul
2026-04-23 15:01   ` Alexandre Courbot
2026-04-23 15:01     ` Alexandre Courbot
2026-04-27 17:29     ` lyude
2026-04-27 17:29       ` lyude

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=DI1C648G6OWI.20O3JB8VS59PB@nvidia.com \
    --to=acourbot@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=airlied@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=boqun@kernel.org \
    --cc=christian.koenig@amd.com \
    --cc=dakr@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=fujita.tomonori@gmail.com \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel@vger.kernel.org \
    --cc=lina+kernel@asahilina.net \
    --cc=lina@asahilina.net \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-media@vger.kernel.org \
    --cc=lorenzo.stoakes@oracle.com \
    --cc=lossin@kernel.org \
    --cc=lyude@redhat.com \
    --cc=mmaurer@google.com \
    --cc=nouveau@lists.freedesktop.org \
    --cc=ojeda@kernel.org \
    --cc=prafulrai522@gmail.com \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=shankari.ak0208@gmail.com \
    --cc=simona@ffwll.ch \
    --cc=sumit.semwal@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /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.