linux-kernel - Re: [PATCH 1/2] drm/gpuvm: add deferred vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250908141156.3dbdea0b@fedora>
Date: Mon, 8 Sep 2025 14:11:56 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>, "Matthew Brost"
 <matthew.brost@...el.com>, Thomas Hellström
 <thomas.hellstrom@...ux.intel.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Steven Price"
 <steven.price@....com>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 "Liviu Dudau" <liviu.dudau@....com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 1/2] drm/gpuvm: add deferred vm_bo cleanup

On Mon, 08 Sep 2025 13:11:32 +0200
"Danilo Krummrich" <dakr@...nel.org> wrote:

> On Mon Sep 8, 2025 at 12:20 PM CEST, Boris Brezillon wrote:
> > I'm not following. Yes there's going to be a
> > drm_gpuva_unlink_defer_put() that skips the
> >
> >         va->vm_bo = NULL;
> >         drm_gpuvm_bo_put(vm_bo);
> >
> > and adds the gpuva to a list for deferred destruction. But I'm not
> > seeing where the leak is. Once the gpuva has been put in this list,
> > there should be no existing component referring to this object, and it's
> > going to be destroyed or recycled, but not re-used as-is.  
> 
> I'm saying exactly what you say: "has to be a special unlink function" ->
> drm_gpuva_unlink_defer_put(). :)

I don't see how calling drm_gpuva_unlink() instead of
drm_gpuva_unlink_defer_put() would leak the vm_bo though. I mean, it
would certainly be wrong because you'd be calling cleanup methods that
are expected to be called with the resv lock held from the
dma-signalling path, but that's a different issue, no? Anyway, if we're
going to allow gpuva cleanup/destruction deferral, we'll either need to
do that through a different function, or through some specialization of
drm_gpuva_unlink() that does things differently based on the
immediate/non-immediate mode (or some other flag).

> 
> >> Yeah, we really want to avoid that.  
> >
> > I think I agree that we want to avoid it, but I'm not too sure about
> > the solution that involves playing with vm_bo::kref. I'm particularly
> > worried by the fact drivers can iterate the evict/extobj lists
> > directly, and can thus see objects scheduled for destruction. I know
> > there's a gpuvm_bo_is_dead() helper, and drivers should be aware of the
> > risks, but I don't feel comfortable about this.  
> 
> No, drivers can't iterate the evict/extobj lists directly; or at least this is
> not intended by GPUVM's API and if drivers do so, this is considered peeking
> into GPUVM internals, so drivers are on their own anyways.
> 
> Iterators, such as for_each_vm_bo_in_list() are not exposed to drivers.

Okay, that's a good thing. I thought Xe was doing some funky stuff with
the list...

> 
> > And since we've mentioned the possibility of having to support
> > gpuva destruction deferral too, I'm wondering it wouldn't be cleaner
> > to just go for this approach from the start (gpuva owns a ref to a
> > vm_bo, which gets released when the gpuva object is released).