Re: [for-next v1 5/5] RDMA/ionic: implement SRQ event handling support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Software SRQ tracking state is irreversibly destroyed before verifying that the fallible hardware SRQ destroy command succeeds, leading to refcount underflow and deadlock on retry.

Pre-existing issues:
- [High] Incorrect flag passed to xa_init_flags() causes a sleep-in-atomic bug when inserting items into the XArray.
--

commit 6b80795e1502f8e968be68240853d9d74c4b43a3
Author: Amit Kumar <a.k@amd.com>

RDMA/ionic: implement SRQ event handling support
    
This commit introduces SRQ event types and associated events for the ionic
driver. It updates the event processing loop to report these events through
the SRQ event handler.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> index 6979e0c5a25c2..de1b8aa1815cc 100644
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
[ ... ]
> @@ -3056,6 +3056,13 @@ int ionic_create_srq(struct ib_srq *ibsrq, struct ib_srq_init_attr *attr,
>  			goto err_resp;
>  	}
>  
> +	kref_init(&srq->kref);
> +	init_completion(&srq->rel_comp);
> +
> +	rc = xa_insert_irq(&dev->srq_tbl, srq->rq.qid, srq, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but won't this xa_insert_irq() call trigger a
sleep-in-atomic bug if the XArray needs to allocate memory?

The xa_insert_irq() function disables interrupts. If a node allocation is
required, xas_nomem() is called which attempts to drop the lock and
re-enable interrupts. However, because dev->srq_tbl is initialized using
GFP_ATOMIC instead of XA_FLAGS_LOCK_IRQ, xas_unlock_type() drops the lock
using spin_unlock() instead of spin_unlock_irq(). This leaves hardware
interrupts disabled while potentially sleeping for a GFP_KERNEL allocation.

> +	if (rc)
> +		goto err_resp;
> +
>  	ionic_pgtbl_unbuf(dev, &buf);
>  	attr->attr.max_wr = srq->rq.q.mask;
>  
[ ... ]
> @@ -3080,6 +3087,11 @@ int ionic_destroy_srq(struct ib_srq *ibsrq, struct ib_udata *udata)
>  	struct ionic_srq *srq = to_ionic_srq(ibsrq);
>  	int rc;
>  
> +	xa_erase_irq(&dev->srq_tbl, srq->rq.qid);
> +
> +	kref_put(&srq->kref, ionic_srq_complete);
> +	wait_for_completion(&srq->rel_comp);
> +

[Severity: High]
Is it safe to irreversibly destroy the software SRQ tracking state before
verifying that the hardware command succeeds?

In ionic_destroy_srq(), the driver erases the SRQ from its tracking table,
drops the kref, and consumes the completion before issuing the hardware
destroy command via ionic_destroy_srq_cmd().

If the hardware command fails (for example, due to a firmware timeout), it
returns an error back to the RDMA core, which leaves the SRQ object intact.
If a user subsequently retries destroying this SRQ, the driver will execute
kref_put() on an already-zero refcount, and call wait_for_completion() on
a completion that has already been consumed and will never be signaled again.
Will this lead to a refcount underflow and a permanent hang?

>  	rc = ionic_destroy_srq_cmd(dev, srq->rq.qid);
>  	if (rc)
>  		return rc;
> diff --git a/drivers/infiniband/hw/ionic/ionic_ibdev.c b/drivers/infiniband/hw/ionic/ionic_ibdev.c
> index 3187f6297946f..b0206d34b6dcf 100644
> --- a/drivers/infiniband/hw/ionic/ionic_ibdev.c
> +++ b/drivers/infiniband/hw/ionic/ionic_ibdev.c
[ ... ]
> @@ -336,6 +338,7 @@ static struct ionic_ibdev *ionic_create_ibdev(struct ionic_aux_dev *ionic_adev)
>  
>  	xa_init_flags(&dev->qp_tbl, GFP_ATOMIC);
>  	xa_init_flags(&dev->cq_tbl, GFP_ATOMIC);
> +	xa_init_flags(&dev->srq_tbl, GFP_ATOMIC);

[Severity: High]
This is a pre-existing issue, but does GFP_ATOMIC result in the correct
XArray lock type? xa_init_flags() expects XArray flags (like
XA_FLAGS_LOCK_IRQ), not gfp_t memory allocation flags.

When GFP_ATOMIC (which doesn't set the XA_FLAGS_LOCK_IRQ bit) is passed,
the XArray lock type defaults to XA_LOCK_NORMAL. If an IRQ-safe operation
like xa_insert_irq() later requires a memory allocation, xas_nomem() will
drop the lock using spin_unlock() rather than spin_unlock_irq(). This
leaves hardware interrupts disabled while xas_nomem() potentially sleeps for
a GFP_KERNEL allocation, resulting in a sleep-in-atomic bug.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090802.152142-1-abhijit.gangurde@amd.com?part=5