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
