Re: [PATCH] IB/hfi1: remove check of list iterator against head past the loop body

[Dennis Dalessandro]

On 3/31/22 6:45 PM, Jakob Koschel wrote:
> 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, either with list_entry_is_head() or
> &pos->member == head, using the iterator variable after the loop should
> be avoided.
> 
> In preparation to limit the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element [1].

The code isn't searching the list. So there is no "found" element. It is walking
a list of things and using each one it comes across.

> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@xxxxxxxxxxxxxx/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@xxxxxxxxx>
> ---
>  drivers/infiniband/hw/hfi1/tid_rdma.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c b/drivers/infiniband/hw/hfi1/tid_rdma.c
> index 2a7abf7a1f7f..b12abf83a91c 100644
> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>  	struct hfi1_ctxtdata *rcd = flow->req->rcd;
>  	struct hfi1_devdata *dd = rcd->dd;
>  	u32 ngroups, pageidx = 0;
> -	struct tid_group *group = NULL, *used;
> +	struct tid_group *group = NULL, *used, *iter;
>  	u8 use;
>  
>  	flow->tnode_cnt = 0;
> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>  		goto used_list;
>  
>  	/* First look at complete groups */
> -	list_for_each_entry(group,  &rcd->tid_group_list.list, list) {
> -		kern_add_tid_node(flow, rcd, "complete groups", group,
> -				  group->size);
> +	list_for_each_entry(iter,  &rcd->tid_group_list.list, list) {
> +		kern_add_tid_node(flow, rcd, "complete groups", iter,
> +				  iter->size);
>  
> -		pageidx += group->size;
> -		if (!--ngroups)
> +		pageidx += iter->size;
> +		if (!--ngroups) {
> +			group = iter;
>  			break;
> +		}
>  	}

The original code's intention was that if group is NULL we never iterated the
list. If group != NULL we either iterated the entire list and ran out or we had
enough and hit the break.

Under the proposed code, group is NULL if we never iterated the loop. It will
also be NULL if we reach the end of the list. So the only time group != NULL is
when we iterated the list and found all the groups we needed.

>  	if (pageidx >= flow->npagesets)
> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>  	 * However, if we are at the head, we have reached the end of the
>  	 * complete groups list from the first loop above
>  	 */
> -	if (group && &group->list == &rcd->tid_group_list.list)
> +	if (!group)

So the problem here is group->list may point to gibberish if we iterated the
entire loop?

Perhaps instead of group, add a bool, call it "need_more" or something. Set it
to True at init time. Then when/if we hit the break set it to False. Means we
found enough groups. Then down here we check if (need_more)....

At the very least if you want to keep the code as it is, fix up the comments to
make sense and explain the situation.

-Denny