Re: [PATCH v1 06/15] net/xsc: add the CQ and QP resource pools

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net/xsc: premature DESTROY_CQ and skipped refcount wait in CQ teardown
- [Medium] net/xsc: missing abort on invalid radix tree delete in xsc_core_destroy_cq()
- [Medium] net/xsc: inverted CQ/QP destroy ordering and inaccurate commit description
- [Low] net/xsc: duplicated CQ teardown in xsc_core_destroy_cq() and changelog mismatch
--

commit bc5206144b3c6edd9114d2a7f0311c3a018b5c6a
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add the CQ and QP resource pools

This commit adds the device-wide radix-tree registries and lifecycle helpers
for completion queues and queue pairs, and wires completion and asynchronous
events from EQs into their respective callbacks.

>     net/xsc: add the CQ and QP resource pools
>     
>     Add the device-wide registries for completion queues and queue pairs,
>     and wire completion events from the EQs into them.
>     
>     CQs and QPs live in radix trees keyed by CQN and QPN and are looked
>     up from the EQ dispatch under RCU with a reference count taken on
>     hit; destroy removes the entry, synchronizes RCU and waits for
>     in-flight users before issuing the firmware destroy command, so a
>     completion arriving concurrently with teardown cannot touch freed
>     memory.  QP destroy issues two RST transitions first to drain
>     pending work.
>     
>     Provide two CQ creation paths: a legacy one used by the ethernet
>     function, and an extended one used by RDMA that falls back to the
>     legacy command when firmware rejects it and splits large contexts
>     into a context command followed by chunked buffer-address commands
>     when the context exceeds the maximum command payload.  New CQs are
>     armed after creation.
>     
>     Add the QP creation, destruction and state transition helpers,
>     including raw-TSO queues and multi-QP RSS receive queues needed by
>     the ethernet function, and route asynchronous QP events to the
>     event callback stored in the core QP.
>     
>     Signed-off-by: Xin Tian <tianx@yunsilicon.com>

[Severity: Medium]
Does the commit message match what the patch actually implements in a few
places?

First, the changelog states that both CQs and QPs are looked up under RCU
and synchronized with RCU on destroy, whereas xsc_qp_event() and
xsc_eq_cq_event() perform radix_tree_lookup() under spin_lock(&table->lock),
and xsc_core_destroy_resource_common() does not call synchronize_rcu().

Second, the changelog states that destroy waits for in-flight users before
issuing the firmware destroy command, but both xsc_core_eth_destroy_cq() and
xsc_core_destroy_cq() issue XSC_CMD_OP_DESTROY_CQ before decrementing
refcount and calling wait_for_completion().

Third, the changelog states that QP destroy issues two RST transitions
first, which appears to misread the opcode name XSC_CMD_OP_2RST_QP
(transition to RST): xsc_core_eth_destroy_qp() issues a single
XSC_CMD_OP_2RST_QP command, and xsc_core_destroy_qp() does not issue a RST
transition at all.

[ ... ]

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/cq.c b/drivers/net/ethernet/yunsilicon/xsc/pci/cq.c
> new file mode 100644
> index 0000000000000..e3e13ef531988
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/cq.c
> @@ -0,0 +1,319 @@

[ ... ]

> +static int xsc_destroy_cq(struct xsc_core_device *xdev, u32 cqn)
> +{
> +	struct xsc_destroy_cq_mbox_out out;
> +	struct xsc_destroy_cq_mbox_in in;
> +
> +	memset(&in, 0, sizeof(in));
> +	memset(&out, 0, sizeof(out));
> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_DESTROY_CQ);
> +	in.cqn = cpu_to_be32(cqn);
> +
> +	return xsc_cmd_exec(xdev, &in, sizeof(in), &out, sizeof(out));
> +}

[ ... ]

> +int xsc_core_eth_destroy_cq(struct xsc_core_device *xdev,
> +			    struct xsc_core_cq *xcq)
> +{
> +	struct xsc_cq_table *table = &xdev->dev_res->cq_table;
> +	struct xsc_core_cq *tmp;
> +	int err;
> +
> +	spin_lock_irq(&table->lock);
> +	tmp = radix_tree_delete(&table->tree, xcq->cqn);
> +	spin_unlock_irq(&table->lock);
> +	if (!tmp) {
> +		err = -ENOENT;
> +		goto err_delete_cq;
> +	}
> +
> +	if (tmp != xcq) {
> +		err = -EINVAL;
> +		goto err_delete_cq;
> +	}
> +
> +	/* the completion reader looks the cq up under rcu_read_lock and
> +	 * takes its reference afterwards; wait for it before dropping
> +	 * the tree reference so it never touches a freed cq
> +	 */
> +	synchronize_rcu();
> +
> +	err = xsc_destroy_cq(xdev, xcq->cqn);
> +	if (err)
> +		goto err_destroy_cq;
> +
> +	if (atomic_dec_and_test(&xcq->refcount))
> +		complete(&xcq->free);
> +	wait_for_completion(&xcq->free);
> +	return 0;

[Severity: High]
Should atomic_dec_and_test(&xcq->refcount) and
wait_for_completion(&xcq->free) happen before xsc_destroy_cq() here (and
likewise before xsc_cmd_exec() in xsc_core_destroy_cq())?

In xsc_cq_completion(), rcu_read_lock() only covers radix_tree_lookup() and
atomic_inc(&cq->refcount):

drivers/net/ethernet/yunsilicon/xsc/pci/eq.c:xsc_cq_completion() {
	rcu_read_lock();
	cq = radix_tree_lookup(&table->tree, cqn);
	if (likely(cq))
		atomic_inc(&cq->refcount);
	rcu_read_unlock();
	...
	if (!cq->comp)
		pci_err(xdev->pdev, "cq->comp is NULL\n");
	else
		cq->comp(cq);

	if (atomic_dec_and_test(&cq->refcount))
		complete(&cq->free);
}

Similarly, xsc_eq_cq_event() drops table->lock before calling cq->event().

Because cq->comp() and cq->event() run outside rcu_read_lock() and
table->lock, synchronize_rcu() only guarantees that no new reader can take a
reference; it does not wait for an in-flight callback that already took a
reference to finish. Issuing XSC_CMD_OP_DESTROY_CQ before waiting on
xcq->free destroys the hardware CQ (allowing the firmware to reassign the
CQN) while cq->comp() or cq->event() is still executing on another CPU.

Furthermore, if xsc_destroy_cq() fails and jumps to err_destroy_cq, xcq has
already been deleted from table->tree, but the function returns without
waiting for in-flight callbacks on xcq->free. If the caller frees xcq after
destroy returns, an in-flight xsc_cq_completion() or xsc_eq_cq_event() will
access freed memory; if the caller retries xsc_core_eth_destroy_cq(),
radix_tree_delete() will return NULL and fail with -ENOENT without
destroying the firmware CQ.

[ ... ]

> +int xsc_core_destroy_cq(struct xsc_core_device *xdev, struct xsc_core_cq *cq)
> +{
> +	struct xsc_cq_table *table = &xdev->dev_res->cq_table;
> +	struct xsc_destroy_cq_mbox_in in;
> +	struct xsc_destroy_cq_mbox_out out;
> +	struct xsc_core_cq *tmp;
> +	int err;
> +
> +	spin_lock_irq(&table->lock);
> +	tmp = radix_tree_delete(&table->tree, cq->cqn);
> +	spin_unlock_irq(&table->lock);
> +	if (!tmp || tmp != cq)
> +		pci_warn(xdev->pdev, "cq 0x%x not found in tree\n", cq->cqn);

[Severity: Medium]
If !tmp or tmp != cq here, shouldn't xsc_core_destroy_cq() abort and return
an error instead of continuing after pci_warn()?

If the entry was not found (for instance, if a prior destroy call deleted it
from table->tree before failing in xsc_cmd_exec(), or on a duplicate
destroy), continuing will execute XSC_CMD_OP_DESTROY_CQ and then call
atomic_dec_and_test(&cq->refcount) followed by
wait_for_completion(&cq->free). If cq->refcount was already decremented, it
will underflow to -1 and hang indefinitely in wait_for_completion().

Also, in both xsc_core_eth_destroy_cq() and xsc_core_destroy_cq(), calling
radix_tree_delete(&table->tree, cq->cqn) before verifying that the stored
entry matches cq will remove an unrelated active CQ (tmp) from table->tree
when tmp != cq. Would it be safer to check radix_tree_lookup() under
table->lock before deleting?

> +
> +	/* wait for lockless completion readers as in
> +	 * xsc_core_eth_destroy_cq()
> +	 */
> +	synchronize_rcu();
> +
> +	memset(&in, 0, sizeof(in));
> +	memset(&out, 0, sizeof(out));
> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_DESTROY_CQ);
> +	in.cqn = cpu_to_be32(cq->cqn);
> +
> +	err = xsc_cmd_exec(xdev, &in, sizeof(in), &out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	if (atomic_dec_and_test(&cq->refcount))
> +		complete(&cq->free);
> +	wait_for_completion(&cq->free);
> +
> +	return 0;
> +}

[Severity: Low]
This isn't a bug, but xsc_core_destroy_cq() open-codes xsc_destroy_cq() and
duplicates the radix-tree removal, synchronize_rcu(), and completion wait
sequence from xsc_core_eth_destroy_cq(). Could xsc_core_destroy_cq() and
xsc_core_eth_destroy_cq() share this teardown logic?

[ ... ]

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/qp.c b/drivers/net/ethernet/yunsilicon/xsc/pci/qp.c
> new file mode 100644
> index 0000000000000..bbcdb18e1b84f
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/qp.c
> @@ -0,0 +1,355 @@

[ ... ]

> +int xsc_core_destroy_qp(struct xsc_core_device *xdev,
> +			struct xsc_core_qp *qp)
> +{
> +	struct xsc_destroy_qp_mbox_in in;
> +	struct xsc_destroy_qp_mbox_out out;
> +	int err;
> +
> +	xsc_core_destroy_resource_common(xdev, qp);
> +
> +	memset(&in, 0, sizeof(in));
> +	memset(&out, 0, sizeof(out));
> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_DESTROY_QP);
> +	in.qpn = cpu_to_be32(qp->qpn);
> +
> +	err = xsc_cmd_exec(xdev, &in, sizeof(in), &out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	atomic_dec(&xdev->num_qps);
> +	return 0;
> +}

[Severity: Medium]
If xsc_cmd_exec() fails in xsc_core_destroy_qp(), xsc_core_destroy_resource_common()
has already removed qp from qp_table.tree and consumed its initial refcount,
while xdev->num_qps remains incremented. How should a caller recover if
XSC_CMD_OP_DESTROY_QP fails after the QP has been removed from the resource
table?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=6