Re: [Intel-gfx] [PATCH] drm/ttm: Fix ttm_bo_move_memcpy() for subclassed struct ttm_resource

[Ben Skeggs <skeggsb@gmail.com>]

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
>