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@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);
Add a xe_bo_assert_held to bo_meminfo.
With that exrta assert:
Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> + 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
>
