[PATCH] amdgpu/ttm: make sure exported objects are populated

[PATCH] amdgpu/ttm: make sure exported objects are populated

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

While discussing cgroups we noticed a problem where you could export
a BO to a dma-buf without having it ever being backed or accounted for.

This meant in low memory situations or eventually with cgroups, a
lower privledged process might cause the compositor to try and allocate
a lot of memory on it's behalf and this could fail. At least make
sure the exporter has managed to allocate the RAM at least once
before exporting the object.

This only applies currently to TTM_PL_SYSTEM objects, because
GTT objects get populated on first validate, and VRAM doesn't
use TT.

TODO:
other drivers?
split this into two?

Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  5 ++++
 drivers/gpu/drm/ttm/ttm_bo.c                | 28 +++++++++++++++++++++
 include/drm/ttm/ttm_bo.h                    |  1 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index ce27cb5bb05e..d0f578d0ae6c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -343,11 +343,16 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_gem_object *gobj,
 {
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj);
 	struct dma_buf *buf;
+	int ret;
 
 	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) ||
 	    bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
 		return ERR_PTR(-EPERM);
 
+	ret = ttm_bo_setup_export(&bo->tbo);
+	if (ret)
+		return ERR_PTR(ret);
+
 	buf = drm_gem_prime_export(gobj, flags);
 	if (!IS_ERR(buf))
 		buf->ops = &amdgpu_dmabuf_ops;
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 273757974b9f..bf98e28a8bda 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1284,3 +1284,31 @@ int ttm_bo_populate(struct ttm_buffer_object *bo,
 	return 0;
 }
 EXPORT_SYMBOL(ttm_bo_populate);
+
+int ttm_bo_setup_export(struct ttm_buffer_object *bo)
+{
+	struct ttm_operation_ctx ctx = {
+		.interruptible = false,
+		.no_wait_gpu = true,
+		/* We opt to avoid OOM on system pages allocations */
+		.gfp_retry_mayfail = true,
+		.allow_res_evict = false,
+	};
+
+	int ret;
+
+	if (!bo->ttm)
+		return 0;
+
+	if (bo->ttm && ttm_tt_is_populated(bo->ttm))
+		return 0;
+
+	ret = ttm_bo_reserve(bo, false, false, NULL);
+	if (ret != 0)
+		return ret;
+
+	ret = ttm_bo_populate(bo, bo->resource->placement & TTM_PL_FLAG_MEMCG, &ctx);
+	ttm_bo_unreserve(bo);
+	return ret;
+}
+EXPORT_SYMBOL(ttm_bo_setup_export);
diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
index c33b3667ae76..5cdd89da9ef5 100644
--- a/include/drm/ttm/ttm_bo.h
+++ b/include/drm/ttm/ttm_bo.h
@@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo);
 int ttm_bo_populate(struct ttm_buffer_object *bo,
 		    bool memcg_account,
 		    struct ttm_operation_ctx *ctx);
+int ttm_bo_setup_export(struct ttm_buffer_object *bo);
 
 /* Driver LRU walk helpers initially targeted for shrinking. */
 
-- 
2.50.1

Re: [PATCH] amdgpu/ttm: make sure exported objects are populated

[Thomas Hellström]

Hi Dave,

On Mon, 2025-09-01 at 14:56 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> While discussing cgroups we noticed a problem where you could export
> a BO to a dma-buf without having it ever being backed or accounted
> for.
> 
> This meant in low memory situations or eventually with cgroups, a
> lower privledged process might cause the compositor to try and
> allocate
> a lot of memory on it's behalf and this could fail. At least make
> sure the exporter has managed to allocate the RAM at least once
> before exporting the object.

With a shmem analogy, let's say process A creates a shmem object and
doesn't populate it. It's then shared with process B which faults in
the pages or otherwise causes it to be populated. Will this patch
ensure we behave the same WRT memory usage accounting?

Also some concerns below.

> 
> This only applies currently to TTM_PL_SYSTEM objects, because
> GTT objects get populated on first validate, and VRAM doesn't
> use TT.
> 
> TODO:
> other drivers?
> split this into two?
> 
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  5 ++++
>  drivers/gpu/drm/ttm/ttm_bo.c                | 28
> +++++++++++++++++++++
>  include/drm/ttm/ttm_bo.h                    |  1 +
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index ce27cb5bb05e..d0f578d0ae6c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -343,11 +343,16 @@ struct dma_buf *amdgpu_gem_prime_export(struct
> drm_gem_object *gobj,
>  {
>  	struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj);
>  	struct dma_buf *buf;
> +	int ret;
>  
>  	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) ||
>  	    bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
>  		return ERR_PTR(-EPERM);
>  
> +	ret = ttm_bo_setup_export(&bo->tbo);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	buf = drm_gem_prime_export(gobj, flags);
>  	if (!IS_ERR(buf))
>  		buf->ops = &amdgpu_dmabuf_ops;
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> b/drivers/gpu/drm/ttm/ttm_bo.c
> index 273757974b9f..bf98e28a8bda 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1284,3 +1284,31 @@ int ttm_bo_populate(struct ttm_buffer_object
> *bo,
>  	return 0;
>  }
>  EXPORT_SYMBOL(ttm_bo_populate);
> +
> +int ttm_bo_setup_export(struct ttm_buffer_object *bo)
> +{
> +	struct ttm_operation_ctx ctx = {
> +		.interruptible = false,
> +		.no_wait_gpu = true,
> +		/* We opt to avoid OOM on system pages allocations
> */
> +		.gfp_retry_mayfail = true,
> +		.allow_res_evict = false,
> +	};

I think we'd want to have the caller (driver) provide the
ttm_operation_ctx. The driver may want to subclass or call
interruptible.


> +
> +	int ret;
> +
> +	if (!bo->ttm)
> +		return 0;
> +
> +	if (bo->ttm && ttm_tt_is_populated(bo->ttm))
> +		return 0;
> +

bo->ttm is not safe to reference without the bo lock. Nor is the
ttm_tt_is_populated stable without the bo lock since you may race with
a swapout / shrink.

Thanks,
Thomas


> +	ret = ttm_bo_reserve(bo, false, false, NULL);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = ttm_bo_populate(bo, bo->resource->placement &
> TTM_PL_FLAG_MEMCG, &ctx);
> +	ttm_bo_unreserve(bo);
> +	return ret;
> +}
> +EXPORT_SYMBOL(ttm_bo_setup_export);
> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> index c33b3667ae76..5cdd89da9ef5 100644
> --- a/include/drm/ttm/ttm_bo.h
> +++ b/include/drm/ttm/ttm_bo.h
> @@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object
> *bo);
>  int ttm_bo_populate(struct ttm_buffer_object *bo,
>  		    bool memcg_account,
>  		    struct ttm_operation_ctx *ctx);
> +int ttm_bo_setup_export(struct ttm_buffer_object *bo);
>  
>  /* Driver LRU walk helpers initially targeted for shrinking. */
>  

Re: [PATCH] amdgpu/ttm: make sure exported objects are populated

[David Airlie]

On Mon, Sep 1, 2025 at 6:02 PM Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> Hi Dave,
>
> On Mon, 2025-09-01 at 14:56 +1000, Dave Airlie wrote:
> > From: Dave Airlie <airlied@redhat.com>
> >
> > While discussing cgroups we noticed a problem where you could export
> > a BO to a dma-buf without having it ever being backed or accounted
> > for.
> >
> > This meant in low memory situations or eventually with cgroups, a
> > lower privledged process might cause the compositor to try and
> > allocate
> > a lot of memory on it's behalf and this could fail. At least make
> > sure the exporter has managed to allocate the RAM at least once
> > before exporting the object.
>
> With a shmem analogy, let's say process A creates a shmem object and
> doesn't populate it. It's then shared with process B which faults in
> the pages or otherwise causes it to be populated. Will this patch
> ensure we behave the same WRT memory usage accounting?
>

I wandered down a few holes, but with cgroup v2, owner pays seems to
be how it works.

They use the inode which records the cgroup owner who created it.

cgroups v1 did it the other way, but I think we should ignore that.


> Also some concerns below.
>
> I think we'd want to have the caller (driver) provide the
> ttm_operation_ctx. The driver may want to subclass or call
> interruptible.

Indeed, might make more sense, though I think we should only be
calling this after creating an object in most cases,
>
>
> > +
> > +     int ret;
> > +
> > +     if (!bo->ttm)
> > +             return 0;
> > +
> > +     if (bo->ttm && ttm_tt_is_populated(bo->ttm))
> > +             return 0;
> > +
>
> bo->ttm is not safe to reference without the bo lock. Nor is the
> ttm_tt_is_populated stable without the bo lock since you may race with
> a swapout / shrink.

Indeed, I was probably being a bit hacky here, I'll always reserve first.

Dave.
>
> Thanks,
> Thomas
>
>
> > +     ret = ttm_bo_reserve(bo, false, false, NULL);
> > +     if (ret != 0)
> > +             return ret;
> > +
> > +     ret = ttm_bo_populate(bo, bo->resource->placement &
> > TTM_PL_FLAG_MEMCG, &ctx);
> > +     ttm_bo_unreserve(bo);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_setup_export);
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index c33b3667ae76..5cdd89da9ef5 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object
> > *bo);
> >  int ttm_bo_populate(struct ttm_buffer_object *bo,
> >                   bool memcg_account,
> >                   struct ttm_operation_ctx *ctx);
> > +int ttm_bo_setup_export(struct ttm_buffer_object *bo);
> >
> >  /* Driver LRU walk helpers initially targeted for shrinking. */
> >
>

Re: [PATCH] amdgpu/ttm: make sure exported objects are populated

[David Airlie]

On Tue, Sep 2, 2025 at 11:22 AM David Airlie <airlied@redhat.com> wrote:
>
> On Mon, Sep 1, 2025 at 6:02 PM Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
> >
> > Hi Dave,
> >
> > On Mon, 2025-09-01 at 14:56 +1000, Dave Airlie wrote:
> > > From: Dave Airlie <airlied@redhat.com>
> > >
> > > While discussing cgroups we noticed a problem where you could export
> > > a BO to a dma-buf without having it ever being backed or accounted
> > > for.
> > >
> > > This meant in low memory situations or eventually with cgroups, a
> > > lower privledged process might cause the compositor to try and
> > > allocate
> > > a lot of memory on it's behalf and this could fail. At least make
> > > sure the exporter has managed to allocate the RAM at least once
> > > before exporting the object.
> >
> > With a shmem analogy, let's say process A creates a shmem object and
> > doesn't populate it. It's then shared with process B which faults in
> > the pages or otherwise causes it to be populated. Will this patch
> > ensure we behave the same WRT memory usage accounting?
> >
>
> I wandered down a few holes, but with cgroup v2, owner pays seems to
> be how it works.
>
> They use the inode which records the cgroup owner who created it.
>
> cgroups v1 did it the other way, but I think we should ignore that.
>

Also it does the allocations at fault time, however since we share our
buffers with hw and higher privilege apps like compositors, allowing
one process to ENOMEM those seems like a bad plan.

This doesn't fully stop it, but at least makes sure the originating
process can allocate the backing store.

Dave.

>
> > Also some concerns below.
> >
> > I think we'd want to have the caller (driver) provide the
> > ttm_operation_ctx. The driver may want to subclass or call
> > interruptible.
>
> Indeed, might make more sense, though I think we should only be
> calling this after creating an object in most cases,
> >
> >
> > > +
> > > +     int ret;
> > > +
> > > +     if (!bo->ttm)
> > > +             return 0;
> > > +
> > > +     if (bo->ttm && ttm_tt_is_populated(bo->ttm))
> > > +             return 0;
> > > +
> >
> > bo->ttm is not safe to reference without the bo lock. Nor is the
> > ttm_tt_is_populated stable without the bo lock since you may race with
> > a swapout / shrink.
>
> Indeed, I was probably being a bit hacky here, I'll always reserve first.
>
> Dave.
> >
> > Thanks,
> > Thomas
> >
> >
> > > +     ret = ttm_bo_reserve(bo, false, false, NULL);
> > > +     if (ret != 0)
> > > +             return ret;
> > > +
> > > +     ret = ttm_bo_populate(bo, bo->resource->placement &
> > > TTM_PL_FLAG_MEMCG, &ctx);
> > > +     ttm_bo_unreserve(bo);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL(ttm_bo_setup_export);
> > > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > > index c33b3667ae76..5cdd89da9ef5 100644
> > > --- a/include/drm/ttm/ttm_bo.h
> > > +++ b/include/drm/ttm/ttm_bo.h
> > > @@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object
> > > *bo);
> > >  int ttm_bo_populate(struct ttm_buffer_object *bo,
> > >                   bool memcg_account,
> > >                   struct ttm_operation_ctx *ctx);
> > > +int ttm_bo_setup_export(struct ttm_buffer_object *bo);
> > >
> > >  /* Driver LRU walk helpers initially targeted for shrinking. */
> > >
> >

Re: [PATCH] amdgpu/ttm: make sure exported objects are populated

[Christian König]

On 02.09.25 03:22, David Airlie wrote:
> On Mon, Sep 1, 2025 at 6:02 PM Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>>
>> Hi Dave,
>>
>> On Mon, 2025-09-01 at 14:56 +1000, Dave Airlie wrote:
>>> From: Dave Airlie <airlied@redhat.com>
>>>
>>> While discussing cgroups we noticed a problem where you could export
>>> a BO to a dma-buf without having it ever being backed or accounted
>>> for.
>>>
>>> This meant in low memory situations or eventually with cgroups, a
>>> lower privledged process might cause the compositor to try and
>>> allocate
>>> a lot of memory on it's behalf and this could fail. At least make
>>> sure the exporter has managed to allocate the RAM at least once
>>> before exporting the object.
>>
>> With a shmem analogy, let's say process A creates a shmem object and
>> doesn't populate it. It's then shared with process B which faults in
>> the pages or otherwise causes it to be populated. Will this patch
>> ensure we behave the same WRT memory usage accounting?
>>
> 
> I wandered down a few holes, but with cgroup v2, owner pays seems to
> be how it works.

Which is pretty much the correct approach as far as I can see. The only problem is that the ENOMEM is delivered to the wrong process.

In other words the one using it for the first time gets the ENOMEM and not the owner.

This is usually solved by making the allocation or at least the reservation of resources happen when they transit a domain boundary, but Linux cgroups doesn't have any of those concepts.

> They use the inode which records the cgroup owner who created it.
> 
> cgroups v1 did it the other way, but I think we should ignore that.

Yeah agree, cgroups v1 is not something we need to take into account.

Apart from that with the technical problems pointed out by Thomas fixed the general approach looks sane to me.

Regards,
Christian.

> 
> 
>> Also some concerns below.
>>
>> I think we'd want to have the caller (driver) provide the
>> ttm_operation_ctx. The driver may want to subclass or call
>> interruptible.
> 
> Indeed, might make more sense, though I think we should only be
> calling this after creating an object in most cases,
>>
>>
>>> +
>>> +     int ret;
>>> +
>>> +     if (!bo->ttm)
>>> +             return 0;
>>> +
>>> +     if (bo->ttm && ttm_tt_is_populated(bo->ttm))
>>> +             return 0;
>>> +
>>
>> bo->ttm is not safe to reference without the bo lock. Nor is the
>> ttm_tt_is_populated stable without the bo lock since you may race with
>> a swapout / shrink.
> 
> Indeed, I was probably being a bit hacky here, I'll always reserve first.
> 
> Dave.
>>
>> Thanks,
>> Thomas
>>
>>
>>> +     ret = ttm_bo_reserve(bo, false, false, NULL);
>>> +     if (ret != 0)
>>> +             return ret;
>>> +
>>> +     ret = ttm_bo_populate(bo, bo->resource->placement &
>>> TTM_PL_FLAG_MEMCG, &ctx);
>>> +     ttm_bo_unreserve(bo);
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL(ttm_bo_setup_export);
>>> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
>>> index c33b3667ae76..5cdd89da9ef5 100644
>>> --- a/include/drm/ttm/ttm_bo.h
>>> +++ b/include/drm/ttm/ttm_bo.h
>>> @@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object
>>> *bo);
>>>  int ttm_bo_populate(struct ttm_buffer_object *bo,
>>>                   bool memcg_account,
>>>                   struct ttm_operation_ctx *ctx);
>>> +int ttm_bo_setup_export(struct ttm_buffer_object *bo);
>>>
>>>  /* Driver LRU walk helpers initially targeted for shrinking. */
>>>
>>
>