[PATCH] ttm: remove check of list iterator against head outside the loop

[PATCH] ttm: remove check of list iterator against head outside the loop

[Xiaomeng Tong]

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.

While it is safe to use the pointer to determine if it was computed
based on the head element with &pos->member == head, using the iterator
variable after the loop should be avoided.

In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].

Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/

Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index db3dc7ef5382..413b5bbf2414 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -673,36 +673,36 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
 			struct ww_acquire_ctx *ticket)
 {
 	struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
+	struct ttm_buffer_object *iter;
 	bool locked = false;
 	unsigned i;
 	int ret;
 
 	spin_lock(&bdev->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
-		list_for_each_entry(bo, &man->lru[i], lru) {
+		list_for_each_entry(iter, &man->lru[i], lru) {
 			bool busy;
 
-			if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
+			if (!ttm_bo_evict_swapout_allowable(iter, ctx, place,
 							    &locked, &busy)) {
 				if (busy && !busy_bo && ticket !=
-				    dma_resv_locking_ctx(bo->base.resv))
-					busy_bo = bo;
+				    dma_resv_locking_ctx(iter->base.resv))
+					busy_bo = iter;
 				continue;
 			}
 
-			if (!ttm_bo_get_unless_zero(bo)) {
+			if (!ttm_bo_get_unless_zero(iter)) {
 				if (locked)
-					dma_resv_unlock(bo->base.resv);
+					dma_resv_unlock(iter->base.resv);
 				continue;
 			}
+
+			bo = iter;
 			break;
 		}
 
-		/* If the inner loop terminated early, we have our candidate */
-		if (&bo->lru != &man->lru[i])
+		if (bo)
 			break;
-
-		bo = NULL;
 	}
 
 	if (!bo) {
-- 
2.17.1

Re: [PATCH] ttm: remove check of list iterator against head outside the loop

[Christian König]

Am 19.03.22 um 08:31 schrieb Xiaomeng Tong:
> When list_for_each_entry() completes the iteration over the whole list
> without breaking the loop, the iterator value will be a bogus pointer
> computed based on the head element.
>
> While it is safe to use the pointer to determine if it was computed
> based on the head element with &pos->member == head, using the iterator
> variable after the loop should be avoided.
>
> In preparation to limiting the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element [1].
>
> Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/

Well exactly that's why I'm pushing back to those changes.

We have tons of cases like this and I certainly won't accept patches to 
make the code more complex than necessary. Especially not adding extra 
local variables.

Regards,
Christian.

>
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index db3dc7ef5382..413b5bbf2414 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -673,36 +673,36 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>   			struct ww_acquire_ctx *ticket)
>   {
>   	struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
> +	struct ttm_buffer_object *iter;
>   	bool locked = false;
>   	unsigned i;
>   	int ret;
>   
>   	spin_lock(&bdev->lru_lock);
>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> -		list_for_each_entry(bo, &man->lru[i], lru) {
> +		list_for_each_entry(iter, &man->lru[i], lru) {
>   			bool busy;
>   
> -			if (!ttm_bo_evict_swapout_allowable(bo, ctx, place,
> +			if (!ttm_bo_evict_swapout_allowable(iter, ctx, place,
>   							    &locked, &busy)) {
>   				if (busy && !busy_bo && ticket !=
> -				    dma_resv_locking_ctx(bo->base.resv))
> -					busy_bo = bo;
> +				    dma_resv_locking_ctx(iter->base.resv))
> +					busy_bo = iter;
>   				continue;
>   			}
>   
> -			if (!ttm_bo_get_unless_zero(bo)) {
> +			if (!ttm_bo_get_unless_zero(iter)) {
>   				if (locked)
> -					dma_resv_unlock(bo->base.resv);
> +					dma_resv_unlock(iter->base.resv);
>   				continue;
>   			}
> +
> +			bo = iter;
>   			break;
>   		}
>   
> -		/* If the inner loop terminated early, we have our candidate */
> -		if (&bo->lru != &man->lru[i])
> +		if (bo)
>   			break;
> -
> -		bo = NULL;
>   	}
>   
>   	if (!bo) {