diff mbox series

IB/hfi1: remove check of list iterator against head past the loop body

Message ID 20220331224501.904039-1-jakobkoschel@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series IB/hfi1: remove check of list iterator against head past the loop body | expand

Commit Message

Jakob Koschel March 31, 2022, 10:45 p.m. UTC 
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].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/infiniband/hw/hfi1/tid_rdma.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)


base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275

Comments

Dennis Dalessandro April 1, 2022, 2:34 p.m. UTC | #1 
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@gmail.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
Jakob Koschel April 1, 2022, 3:16 p.m. UTC | #2 
> On 1. Apr 2022, at 16:34, Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> wrote:
> 
> 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.

Ok, I can see how this is confusing. I'll change the text accordingly.

> 
>> 
>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.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.

This is however not correct. list_for_each_entry() always sets 'group'.
It needs to do so to check the terminating condition of the loop.
It does so even if the list is empty and the loop body is not even executed once.

> 
> 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?

In this case it would be &group->list == &rcd->tid_group_list.list
So the second part of the check is correct whereas the check "if group == true"
is just always true.

Since the intention is to move the scope of the list iterator into the macro
itself I'm removing the accesses to the list iterator after the loop body
('group' in this case).

> 
> 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)....

If I understand you correctly the condition here should be:

	if (!list_empty(&rcd->tid_group_list.list) && !group)

which will only be true if the list actually did at least one iteration but did not
break early.

> 
> 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.

The code behaves exactly as it did before my patch.
Sorry I've missed that comment, I'll update it as well. As explain above, this
comment is a bit misleading since that's not actually what is executing and therefore
I got confused. group would be "at head" if the list being iterated
is empty, since the list iterator macros does not differentiate between
"not running any iteration of the loop since the list is empty" and 
"running to the end of the loop with at least one iteration".

> 
> -Denny

Sorry for the confusing terminology and missing the comment.

Thanks,
Jakob
diff mbox series

Patch

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;
+		}
 	}
 
 	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)
 		goto bail_eagain;
 	group = list_prepare_entry(group, &rcd->tid_group_list.list,
 				   list);