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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0d3e61ea-b3b8-620a-f418-0c91b381b67d@cornelisnetworks.com>
Date:   Fri, 1 Apr 2022 10:34:26 -0400
From:   Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>
To:     Jakob Koschel <jakobkoschel@...il.com>,
        Mike Marciniszyn <mike.marciniszyn@...nelisnetworks.com>
Cc:     Jason Gunthorpe <jgg@...pe.ca>, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org, Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH] IB/hfi1: remove check of list iterator against head past
 the loop body

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@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
> ---
>  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