* Re: [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource
2021-08-30 7:48 [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource Thomas Hellström
@ 2021-08-30 8:06 ` Christian König
2021-08-30 8:39 ` Ben Skeggs
2021-08-30 9:27 ` Daniel Vetter
2 siblings, 0 replies; 4+ messages in thread
From: Christian König @ 2021-08-30 8:06 UTC (permalink / raw)
To: Thomas Hellström, intel-gfx, dri-devel; +Cc: Ben Skeggs, Dave Airlie
Am 30.08.21 um 09:48 schrieb Thomas Hellström:
> The code was making a copy of a struct ttm_resource. However,
> recently the struct ttm_resources were allowed to be subclassed and
> also were allowed to be malloced, hence the driver could end up assuming
> the copy we handed it was subclassed and worse, the original could have
> been freed at this point.
>
> Fix this by using the original struct ttm_resource before it is
> potentially freed in ttm_bo_move_sync_cleanup()
>
> Reported-by: Ben Skeggs <skeggsb@gmail.com>
> Reported-by: Dave Airlie <airlied@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem")
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/ttm/ttm_bo_util.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 5c20d0541cc3..c893c3db2623 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -139,7 +139,6 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> struct ttm_resource *src_mem = bo->resource;
> struct ttm_resource_manager *src_man =
> ttm_manager_type(bdev, src_mem->mem_type);
> - struct ttm_resource src_copy = *src_mem;
> union {
> struct ttm_kmap_iter_tt tt;
> struct ttm_kmap_iter_linear_io io;
> @@ -173,11 +172,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)))
> ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter);
>
> - src_copy = *src_mem;
> + if (!src_iter->ops->maps_tt)
> + ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, src_mem);
> ttm_bo_move_sync_cleanup(bo, dst_mem);
>
> - if (!src_iter->ops->maps_tt)
> - ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, &src_copy);
> out_src_iter:
> if (!dst_iter->ops->maps_tt)
> ttm_kmap_iter_linear_io_fini(&_dst_iter.io, bdev, dst_mem);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource
2021-08-30 7:48 [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource Thomas Hellström
2021-08-30 8:06 ` Christian König
@ 2021-08-30 8:39 ` Ben Skeggs
2021-08-30 9:27 ` Daniel Vetter
2 siblings, 0 replies; 4+ messages in thread
From: Ben Skeggs @ 2021-08-30 8:39 UTC (permalink / raw)
To: Thomas Hellström
Cc: intel-gfx, ML dri-devel, Dave Airlie, Christian König
On Mon, 30 Aug 2021 at 17:48, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> The code was making a copy of a struct ttm_resource. However,
> recently the struct ttm_resources were allowed to be subclassed and
> also were allowed to be malloced, hence the driver could end up assuming
> the copy we handed it was subclassed and worse, the original could have
> been freed at this point.
>
> Fix this by using the original struct ttm_resource before it is
> potentially freed in ttm_bo_move_sync_cleanup()
>
> Reported-by: Ben Skeggs <skeggsb@gmail.com>
> Reported-by: Dave Airlie <airlied@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem")
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
That's basically identical to what I came up with locally, so:
Reviewed-by: Ben Skeggs <bskeggs@redhat.com>
> ---
> drivers/gpu/drm/ttm/ttm_bo_util.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 5c20d0541cc3..c893c3db2623 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -139,7 +139,6 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> struct ttm_resource *src_mem = bo->resource;
> struct ttm_resource_manager *src_man =
> ttm_manager_type(bdev, src_mem->mem_type);
> - struct ttm_resource src_copy = *src_mem;
> union {
> struct ttm_kmap_iter_tt tt;
> struct ttm_kmap_iter_linear_io io;
> @@ -173,11 +172,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)))
> ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter);
>
> - src_copy = *src_mem;
> + if (!src_iter->ops->maps_tt)
> + ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, src_mem);
> ttm_bo_move_sync_cleanup(bo, dst_mem);
>
> - if (!src_iter->ops->maps_tt)
> - ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, &src_copy);
> out_src_iter:
> if (!dst_iter->ops->maps_tt)
> ttm_kmap_iter_linear_io_fini(&_dst_iter.io, bdev, dst_mem);
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource
2021-08-30 7:48 [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource Thomas Hellström
2021-08-30 8:06 ` Christian König
2021-08-30 8:39 ` Ben Skeggs
@ 2021-08-30 9:27 ` Daniel Vetter
2 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2021-08-30 9:27 UTC (permalink / raw)
To: Thomas Hellström, Maarten Lankhorst
Cc: intel-gfx, dri-devel, Ben Skeggs, Dave Airlie, Christian König
On Mon, Aug 30, 2021 at 09:48:35AM +0200, Thomas Hellström wrote:
> The code was making a copy of a struct ttm_resource. However,
> recently the struct ttm_resources were allowed to be subclassed and
> also were allowed to be malloced, hence the driver could end up assuming
> the copy we handed it was subclassed and worse, the original could have
> been freed at this point.
>
> Fix this by using the original struct ttm_resource before it is
> potentially freed in ttm_bo_move_sync_cleanup()
>
> Reported-by: Ben Skeggs <skeggsb@gmail.com>
> Reported-by: Dave Airlie <airlied@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem")
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
This doesn't apply cleanly to drm-misc-next-fixes, do we need a backmerge
or something? Can you pls coordinate with Maarten and then also poke for a
pull request so this isn't stuck?
Also since 5.14 is released this needs cc: stable.
-Daniel
> ---
> drivers/gpu/drm/ttm/ttm_bo_util.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 5c20d0541cc3..c893c3db2623 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -139,7 +139,6 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> struct ttm_resource *src_mem = bo->resource;
> struct ttm_resource_manager *src_man =
> ttm_manager_type(bdev, src_mem->mem_type);
> - struct ttm_resource src_copy = *src_mem;
> union {
> struct ttm_kmap_iter_tt tt;
> struct ttm_kmap_iter_linear_io io;
> @@ -173,11 +172,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
> if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)))
> ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter);
>
> - src_copy = *src_mem;
> + if (!src_iter->ops->maps_tt)
> + ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, src_mem);
> ttm_bo_move_sync_cleanup(bo, dst_mem);
>
> - if (!src_iter->ops->maps_tt)
> - ttm_kmap_iter_linear_io_fini(&_src_iter.io, bdev, &src_copy);
> out_src_iter:
> if (!dst_iter->ops->maps_tt)
> ttm_kmap_iter_linear_io_fini(&_dst_iter.io, bdev, dst_mem);
> --
> 2.31.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread