[jeynmann]

What?

Add token query support in ucx.

How?

Implement rx/tx token query via qp query.
Cleanup plugin related code.

[evgeny-leksikov]

pls align by =

[evgeny-leksikov]

LGTM besides few minor comments

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit 0bb751f3

TL;DR: The Coverity devel job failed with 2 new FORWARD_NULL defects introduced by this PR's token-query code: the new gtest test_uct_query.query_token_support passes a zero-initialized uct_iface_attr_v2_t (so tx_token/rx_token are NULL) into uct_iface_query_v2(), and Coverity's model of uct_rc_mlx5_iface_query_rx_token() dereferences those pointers. Fix by adding explicit NULL validation for tx_token/rx_token in uct_rc_mlx5_iface_query_rx_token() (and/or passing real token buffers in the test).

Full analysis

Summary: coverity devel on coverity_rh7 exited non-zero — cov-analyze reported 2 new "Explicit null dereferenced (FORWARD_NULL)" defects, so the pipeline step failed (##[error]Coverity found 2 issues).

Root cause: Both defects originate at test/gtest/uct/v2/test_uct_query.cc:119 (uct_iface_attr_v2_t attr = {}; → attr.tx_token = NULL, attr.rx_token = NULL) and are reported at the call on line 125 (uct_iface_query_v2(get_iface(), &attr)). Coverity resolves the function pointer iface->internal_ops->iface_query_v2 (src/uct/base/uct_iface.c:204) to uct_rc_mlx5_iface_query_v2, and its path model takes the iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN branch (rc_mlx5_iface.c:1104) as true, reaching uct_rc_mlx5_iface_query_rx_token() which unconditionally dereferences tx_token (*tx_token at rc_mlx5_iface.c:1068) and rx_token (*rx_token at rc_mlx5_iface.c:1086). Coverity does not correlate the field_mask bits with the pointer being valid — the new code only guards on field_mask and never validates the pointers themselves, so the checker has no reason to consider them non-NULL. This is a new-code report: the only recent commits to rc_mlx5_iface.c are this PR's own (f6010c77 "impl token query", 9b1ba919, 88929cb3, 0bb751f3), and the flagged test query_token_support is added by the same PR.

Implicated commit: f6010c77 "UCT/IB/MLX5: impl token query" (Zihao Zhao), refined by 0bb751f3 (Zihao Zhao) — the head commit of this build

File: src/uct/ib/mlx5/rc/rc_mlx5_iface.c:1061-1086 (dereferences at :1068 and :1086); triggering test at test/gtest/uct/v2/test_uct_query.cc:119,125

Suggested fix: Make the pointer contract explicit so both the code is safer and Coverity's path is pruned:

static ucs_status_t
uct_rc_mlx5_iface_query_rx_token(uct_iface_h tl_iface,
                                 uct_iface_attr_v2_t *iface_attr)
{
    ...
    if (!(iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN)) {
        ucs_error("rc mlx5: tx token is required to query rx token");
        return UCS_ERR_INVALID_PARAM;
    }

    if ((iface_attr->tx_token == NULL) || (iface_attr->rx_token == NULL)) {
        ucs_error("rc mlx5: tx/rx token pointer is NULL");
        return UCS_ERR_INVALID_PARAM;
    }
    ...
}

Secondary hardening (optional but recommended): in uct_rc_mlx5_iface_query_v2() only enter the RX-token path when both UCT_IFACE_ATTR_FIELD_RX_TOKEN and UCT_IFACE_ATTR_FIELD_TX_TOKEN are set, and in the gtest either point attr.tx_token/attr.rx_token at real stack objects or don't leave them NULL while the struct is passed to a function that can consume them — this removes the ambiguity Coverity is complaining about. Re-run the Coverity job after the change; the cov_build_devel/output/errors artifact (published as coverity_devel) can be used to confirm the count drops to 0.

Related: PR #11804 (failover_ext_token_query) — the PR under test is the source of both defects; no pre-existing issue found.

[roiedanino]

@jeynmann shouldn't we remove uct_ib_mlx5_ext_ep_query now that no one is calling it?
same for uct_ib_mlx5_ext_ep_query_check_param(), uct_ib_mlx5_ext_ep_query_attr_t, uct_ib_mlx5_ext_ep_query_func_t, UCT_IB_MLX5_EXT_EP_QUERY_ATTR_FIELD_TX_TOKEN, and the ep_query member of uct_ib_mlx5_ext_ops_t

[jeynmann]

@jeynmann shouldn't we remove uct_ib_mlx5_ext_ep_query now that no one is calling it? same for uct_ib_mlx5_ext_ep_query_check_param(), uct_ib_mlx5_ext_ep_query_attr_t, uct_ib_mlx5_ext_ep_query_func_t, UCT_IB_MLX5_EXT_EP_QUERY_ATTR_FIELD_TX_TOKEN, and the ep_query member of uct_ib_mlx5_ext_ops_t

Hi, @roiedanino Removed leftovers.

[roiedanino]

maybe we should check remote_qpn (smaller than UCS_BIT(UCT_IB_QPN_ORDER)) is in range? this is not on fast-path

[roiedanino]

It would be better to check UCT_IB_MLX5_MD_FLAG_DEVX at runtime rather than (or in addition to) how UCX was compiled

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

NULL token pointers are only guarded by ucs_assert, which is compiled out in release builds. The removed uct_ib_mlx5_ext_iface_query_check_param returned UCS_ERR_INVALID_PARAM when tx_token/rx_token were NULL while their field-mask bits were set. Now a caller that sets UCT_IFACE_ATTR_FIELD_TX_TOKEN | UCT_IFACE_ATTR_FIELD_RX_TOKEN but leaves the pointers NULL will dereference NULL at be32toh(*tx_token) / *rx_token = .... The EP path (uct_rc_mlx5_ep_query_tx_token) kept a real runtime check for ep_attr->tx_token == NULL, so this is an inconsistent regression. Suggest replacing the assert with an UCS_ERR_INVALID_PARAM check like the ep path.

[svc-nvidia-pr-review]

Double blank line after the new typedefs before the uct_rc_mlx5_ep_t comment block; drop one blank line to match surrounding style.

[svc-nvidia-pr-review]

In test_rc_mlx5_token_query.am_short, EXPECT_EQ(NUM_MESSAGES, be32toh(rx_token)) assumes next_rcv_psn equals exactly the number of AM messages. This holds only if no other packets (e.g. FC control, keepalive) are received on that QP. With default config and 10 short AMs this is likely fine, but it is a latent flakiness if FC or other traffic ever lands on the RX side; worth confirming FC is not triggered here.

[svc-nvidia-pr-review]

Coverage gaps to confirm: the new iface/ep token paths are only exercised on DEVX-capable HW (test skips otherwise), and there is no negative test for the removed NULL-pointer validation (related to the iface blocker) or for the "tx/rx must be set together" UCS_ERR_INVALID_PARAM path in uct_rc_mlx5_iface_query_v2. Adding an error-path test would replace the coverage lost when test_uct_ib_mlx5_ext_rc.iface_query/ep_query were deleted.