Re: [PATCH 2/4] drm/xe/client: add missing bo locking in show_meminfo()

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Sep 10, 2024 at 02:11:47PM +0100, Matthew Auld wrote:
> 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.
> 

Path LGTM, one suggestion.

> Fixes: 0845233388f8 ("drm/xe: Implement fdinfo memory stats printing")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: <stable@vger.kernel.org> # 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);

Add a xe_bo_assert_held to bo_meminfo.

With that exrta assert:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +			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;
>  
> -		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
>