> -----Original Message-----
> From: Auld, Matthew <matthew.auld@xxxxxxxxx>
> Sent: Tuesday, September 10, 2024 6:42 PM
> To: intel-xe@xxxxxxxxxxxxxxxxxxxxx
> Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray@xxxxxxxxx>; Upadhyay,
> Tejas <tejas.upadhyay@xxxxxxxxx>; Thomas Hellström
> <thomas.hellstrom@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [PATCH 2/4] drm/xe/client: add missing bo locking in
> show_meminfo()
>
> bo_meminfo() wants to inspect bo state like tt and the ttm resource, however
> this state can change at any point leading to stuff like NPD and UAF, if the bo
> lock is not held. Grab the bo lock when calling bo_meminfo(), ensuring we
> drop any spinlocks first. In the case of object_idr we now also need to hold a
> ref.
>
> Fixes: 0845233388f8 ("drm/xe: Implement fdinfo memory stats printing")
> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@xxxxxxxxx>
> Cc: Tejas Upadhyay <tejas.upadhyay@xxxxxxxxx>
> Cc: "Thomas Hellström" <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.8+
> ---
> drivers/gpu/drm/xe/xe_drm_client.c | 37 +++++++++++++++++++++++++++---
> 1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
> b/drivers/gpu/drm/xe/xe_drm_client.c
> index badfa045ead8..3cca741c500c 100644
> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> @@ -10,6 +10,7 @@
> #include <linux/slab.h>
> #include <linux/types.h>
>
> +#include "xe_assert.h"
> #include "xe_bo.h"
> #include "xe_bo_types.h"
> #include "xe_device_types.h"
> @@ -151,10 +152,13 @@ void xe_drm_client_add_bo(struct xe_drm_client
> *client,
> */
> void xe_drm_client_remove_bo(struct xe_bo *bo) {
> + struct xe_device *xe = ttm_to_xe_device(bo->ttm.bdev);
> struct xe_drm_client *client = bo->client;
>
> + xe_assert(xe, !kref_read(&bo->ttm.base.refcount));
> +
> spin_lock(&client->bos_lock);
> - list_del(&bo->client_link);
> + list_del_init(&bo->client_link);
> spin_unlock(&client->bos_lock);
>
> xe_drm_client_put(client);
> @@ -207,7 +211,20 @@ static void show_meminfo(struct drm_printer *p,
> struct drm_file *file)
> idr_for_each_entry(&file->object_idr, obj, id) {
> struct xe_bo *bo = gem_to_xe_bo(obj);
>
> - bo_meminfo(bo, stats);
> + if (dma_resv_trylock(bo->ttm.base.resv)) {
> + bo_meminfo(bo, stats);
> + xe_bo_unlock(bo);
> + } else {
> + xe_bo_get(bo);
> + spin_unlock(&file->table_lock);
> +
> + xe_bo_lock(bo, false);
> + bo_meminfo(bo, stats);
> + xe_bo_unlock(bo);
> +
> + xe_bo_put(bo);
> + spin_lock(&file->table_lock);
> + }
> }
> spin_unlock(&file->table_lock);
>
> @@ -217,7 +234,21 @@ static void show_meminfo(struct drm_printer *p,
> struct drm_file *file)
> if (!kref_get_unless_zero(&bo->ttm.base.refcount))
> continue;
>
While we have ref to BO, why would it need lock here, can you please explain if I am missing something. I though BO cant be deleted till will hold ref?
Thanks,
Tejas
> - bo_meminfo(bo, stats);
> + if (dma_resv_trylock(bo->ttm.base.resv)) {
> + bo_meminfo(bo, stats);
> + xe_bo_unlock(bo);
> + } else {
> + spin_unlock(&client->bos_lock);
> +
> + xe_bo_lock(bo, false);
> + bo_meminfo(bo, stats);
> + xe_bo_unlock(bo);
> +
> + spin_lock(&client->bos_lock);
> + /* The bo ref will prevent this bo from being removed
> from the list */
> + xe_assert(xef->xe, !list_empty(&bo->client_link));
> + }
> +
> xe_bo_put_deferred(bo, &deferred);
> }
> spin_unlock(&client->bos_lock);
> --
> 2.46.0
