[bug report] IB/hfi1: TID RDMA RcvArray programming and TID allocation

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

Hello Kaike Wan,

The patch 838b6fd2d9ca: "IB/hfi1: TID RDMA RcvArray programming and
TID allocation" from Jan 23, 2019, leads to the following Smatch
static checker warning:

	drivers/infiniband/hw/hfi1/tid_rdma.c:1280 kern_alloc_tids()
	warn: iterator used outside loop: 'group'

drivers/infiniband/hw/hfi1/tid_rdma.c
    1237 static int kern_alloc_tids(struct tid_rdma_flow *flow)
    1238 {
    1239         struct hfi1_ctxtdata *rcd = flow->req->rcd;
    1240         struct hfi1_devdata *dd = rcd->dd;
    1241         u32 ngroups, pageidx = 0;
    1242         struct tid_group *group = NULL, *used;
                                   ^^^^^^^^^^^^
"group" is NULL here.

    1243         u8 use;
    1244 
    1245         flow->tnode_cnt = 0;
    1246         ngroups = flow->npagesets / dd->rcv_entries.group_size;
    1247         if (!ngroups)
    1248                 goto used_list;

"group" is still NULL on this error path.

    1249 
    1250         /* First look at complete groups */
    1251         list_for_each_entry(group,  &rcd->tid_group_list.list, list) {
    1252                 kern_add_tid_node(flow, rcd, "complete groups", group,
    1253                                   group->size);
    1254 
    1255                 pageidx += group->size;
    1256                 if (!--ngroups)
    1257                         break;
    1258         }

If we do not hit the break statement then "group" points to invalid
memory.

    1259 
    1260         if (pageidx >= flow->npagesets)
    1261                 goto ok;
    1262 
    1263 used_list:
    1264         /* Now look at partially used groups */
    1265         list_for_each_entry(used, &rcd->tid_used_list.list, list) {
    1266                 use = min_t(u32, flow->npagesets - pageidx,
    1267                             used->size - used->used);
    1268                 kern_add_tid_node(flow, rcd, "used groups", used, use);
    1269 
    1270                 pageidx += use;
    1271                 if (pageidx >= flow->npagesets)
    1272                         goto ok;
    1273         }
    1274 
    1275         /*
    1276          * Look again at a complete group, continuing from where we left.
    1277          * However, if we are at the head, we have reached the end of the
    1278          * complete groups list from the first loop above
    1279          */
--> 1280         if (group && &group->list == &rcd->tid_group_list.list)

Okay.  We could silence this warning and clean up the code by writing

	if (list_entry_is_head(group, &rcd->tid_group_list.list, list))
		goto bail_eagain;

But what about if "group" is NULL?  Perhaps this should be:

	if (!group || list_entry_is_head(group, &rcd->tid_group_list.list, list))
		goto bail_eagain;

Because otherwise the code will crash.  See below.

    1281                 goto bail_eagain;
    1282         group = list_prepare_entry(group, &rcd->tid_group_list.list,
    1283                                    list);

Then group would still be NULL here.

    1284         if (list_is_last(&group->list, &rcd->tid_group_list.list))
    1285                 goto bail_eagain;
    1286         group = list_next_entry(group, list);

Now group points to invalid memory

    1287         use = min_t(u32, flow->npagesets - pageidx, group->size);
                                                             ^^^^^^^^^^^
And this dereference will crash

    1288         kern_add_tid_node(flow, rcd, "complete continue", group, use);
    1289         pageidx += use;
    1290         if (pageidx >= flow->npagesets)
    1291                 goto ok;
    1292 bail_eagain:
    1293         trace_hfi1_msg_alloc_tids(flow->req->qp, " insufficient tids: needed ",
    1294                                   (u64)flow->npagesets);
    1295         return -EAGAIN;
    1296 ok:
    1297         return 0;
    1298 }

regards,
dan carpenter