[PATCH for-next] RDMA/ionic: Fix GSI QP ID release

[PATCH for-next] RDMA/ionic: Fix GSI QP ID release

[Håkon Bugge]

QPID allocation differs between ordinary QPs and GSI QPs. If GSI QP
creation fails after allocating its QPID, or is destroyed, we see in
the log (trimmed for brevity):

kernel: ida_free called for id=32 which is not allocated.
kernel: WARNING: CPU: 47 PID: 6673 at lib/idr.c:527 ida_free+0xde/0x140
kernel: CPU: 47 UID: 0 PID: 6673 Comm: modprobe Not tainted 6.12.0+ #18
kernel: RIP: 0010:ida_free+0xde/0x140
kernel: Call Trace:
kernel: <TASK>
kernel: ionic_create_qp+0x4f5/0xbe0 [ionic_rdma]
kernel: create_qp+0x19a/0x290 [ib_core]
kernel: ib_create_qp_kernel+0x3a/0xe0 [ib_core]
kernel: create_mad_qp+0x8e/0xd6 [ib_core]
kernel: ib_mad_port_open+0x268/0x3f2 [ib_core]

GSI QPs reserve allocator ID IB_QPT_GSI directly, whereas ordinary QPs
encode the allocated ID in the QPID. Calling ionic_put_qpid() for a
GSI QP incorrectly applies the ordinary QPID conversion and releases
an unallocated ID.

Release IB_QPT_GSI directly in both the creation error path and the
destruction path.

Fixes: e8521822c733 ("RDMA/ionic: Register device ops for control path")
Cc: <stable@vger.kernel.org>
Reported-by: Keith Lucas <keith.lucas@oracle.com>
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Tested-by: Keith Lucas <keith.lucas@oracle.com>
---
 drivers/infiniband/hw/ionic/ionic_controlpath.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
index a70ef59a8064e..cd623f1de38a3 100644
--- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
+++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
@@ -2450,7 +2450,11 @@ int ionic_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attr,
 err_ahid:
 	kfree(qp->hdr);
 err_ah_alloc:
-	ionic_put_qpid(dev, qp->qpid);
+	if (attr->qp_type == IB_QPT_GSI)
+		ionic_resid_put(&dev->inuse_qpid, IB_QPT_GSI);
+	else
+		ionic_put_qpid(dev, qp->qpid);
+
 	return rc;
 }
 
@@ -2762,7 +2766,10 @@ int ionic_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
 		ionic_put_ahid(dev, qp->ahid);
 		kfree(qp->hdr);
 	}
-	ionic_put_qpid(dev, qp->qpid);
+	if (qp->qpid == IB_QPT_GSI)
+		ionic_resid_put(&dev->inuse_qpid, IB_QPT_GSI);
+	else
+		ionic_put_qpid(dev, qp->qpid);
 
 	return 0;
 }
-- 
2.43.5

Re: [PATCH for-next] RDMA/ionic: Fix GSI QP ID release

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/ionic: QPID 1 collision and wrong IDA free in ionic_destroy_qp()
--

commit edc58ccd1d5e8131fe43b51993df0ab60279d823
Author: Håkon Bugge <haakon.bugge@oracle.com>

RDMA/ionic: Fix GSI QP ID release

This commit changes the error path in ionic_create_qp() and the cleanup path
in ionic_destroy_qp() to release IB_QPT_GSI directly into dev->inuse_qpid
instead of calling ionic_put_qpid() when freeing a GSI QP.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> index 18ad967812f50..47904c5b9ce19 100644
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> @@ -2544,7 +2544,11 @@ int ionic_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attr,
>  err_ahid:
>  	kfree(qp->hdr);
>  err_ah_alloc:
> -	ionic_put_qpid(dev, qp->qpid);
> +	if (attr->qp_type == IB_QPT_GSI)
> +		ionic_resid_put(&dev->inuse_qpid, IB_QPT_GSI);
> +	else
> +		ionic_put_qpid(dev, qp->qpid);
> +
>  	return rc;
>  }
>  
> @@ -2858,7 +2862,10 @@ int ionic_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
>  		ionic_put_ahid(dev, qp->ahid);
>  		kfree(qp->hdr);
>  	}
> -	ionic_put_qpid(dev, qp->qpid);
> +	if (qp->qpid == IB_QPT_GSI)
> +		ionic_resid_put(&dev->inuse_qpid, IB_QPT_GSI);
> +	else
> +		ionic_put_qpid(dev, qp->qpid);
>  
>  	return 0;
>  }

[Severity: High]
Does releasing raw IB_QPT_GSI (bitid 1) in dev->inuse_qpid and checking
qp->qpid == IB_QPT_GSI in ionic_destroy_qp() leave the GSI bitid unreserved
and cause ordinary QPs on udma 1 to collide with QPID 1 when
dev->lif_cfg.udma_qgrp_shift == 0?

The IDA allocator dev->inuse_qpid tracks bit indices (bitid), which
ionic_get_qpid() and ionic_put_qpid() convert to and from queue IDs (qpid)
using ionic_bitid_to_qid() and ionic_qid_to_bitid().

In the configuration from the commit message where ionic_put_qpid(dev, 1)
passes id=32 to ida_free() (dev->lif_cfg.udma_qgrp_shift == 0 and
dev->half_qpid_udma_shift == 5), ionic_qid_to_bitid(IB_QPT_GSI, 0, 5) is 32,
and its inverse ionic_bitid_to_qid(32, 0, 5) is 1 (IB_QPT_GSI). Meanwhile,
bitid 1 maps to qpid 2 on udma 0.

In drivers/infiniband/hw/ionic/ionic_controlpath.c, ionic_get_gsi_qpid()
reserves raw bitid 1 instead of the bitid corresponding to IB_QPT_GSI:

	rc = ionic_resid_get_shared(&dev->inuse_qpid, IB_QPT_GSI, IB_QPT_GSI + 1);
	if (rc < 0)
		return rc;

	*qpid = IB_QPT_GSI;

And in ionic_get_qpid(), the reserved QPID check only skips bitids 0 and 1
when base == 0 (udma_ix == 0):

		size = dev->lif_cfg.qp_count / dev->lif_cfg.udma_count;
		base = size * udma_ix;
		bound = base + size;

		/* skip reserved SMI and GSI qpids in group zero */
		if (!base)
			base = 2;

		rc = ionic_resid_get_shared(&dev->inuse_qpid, base, bound);
		if (rc >= 0) {
			*qpid = ionic_bitid_to_qid(rc,
						   dev->lif_cfg.udma_qgrp_shift,
						   dev->half_qpid_udma_shift);

When dev->lif_cfg.udma_qgrp_shift == 0 and an ordinary QP is allocated on
udma_ix == 1 (base == 32), bitid 32 is not reserved in dev->inuse_qpid, so
ionic_get_qpid() allocates bitid 32 and sets *qpid = 1 (IB_QPT_GSI).

If GSI QP creation failed earlier so no GSI QP exists, that ordinary QP is
created with qp->qpid == 1 backed by bitid 32. When that ordinary QP is later
destroyed, wouldn't ionic_destroy_qp() see qp->qpid == IB_QPT_GSI and call
ionic_resid_put(&dev->inuse_qpid, IB_QPT_GSI), freeing unallocated bitid 1
(triggering the ida_free() warning for id=1) and leaking bitid 32?

Also, if the GSI QP is active at QPID 1 when an ordinary QP on udma_ix == 1
is assigned qp->qpid == 1 in ionic_create_qp():

	entry = xa_store_irq(&dev->qp_tbl, qp->qpid, qp, GFP_KERNEL);
	if (entry) {
		if (!xa_is_err(entry))
			rc = -EINVAL;
		else
			rc = xa_err(entry);

		goto err_qp_cmd;
	}

can xa_store_irq() overwrite the active GSI QP in dev->qp_tbl[1] with the new
qp and jump to err_qp_cmd, destroying QPID 1 in firmware via
ionic_destroy_qp_cmd(dev, 1) and leaving a dangling pointer to the freed
ordinary qp in dev->qp_tbl[1]?

Would it make more sense for ionic_get_gsi_qpid() to reserve
ionic_qid_to_bitid(IB_QPT_GSI, dev->lif_cfg.udma_qgrp_shift,
dev->half_qpid_udma_shift) in dev->inuse_qpid (and for ionic_get_qpid() to
only skip bitid 0 on udma 0 when udma_qgrp_shift == 0), so that
ionic_put_qpid(dev, qp->qpid) works uniformly for both GSI and ordinary QPs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021161651.4020222-1-haakon.bugge@oracle.com?part=1