Re: [PATCH rdma-next 21/23] RDMA/mlx5: Clamp log_max_* shifts in query_device and ucontext paths

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:25, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] mlx5_ib_create_srq() (drivers/infiniband/hw/mlx5/srq.c) now
>    takes its SRQ size limit from MLX5_CAP_GEN_LOGSZ_U32(log_max_srq_sz).
> - [Low] The commit message does not fully match the diff.
> - [Low] When FW reports log_max_cq_sz >= 31, mlx5_ib_query_device() now
>    advertises props->max_cqe = MLX5_CAP_GEN_LOGSZ_U32(log_max_cq_sz) - 1…
> 
> --
> 
> RDMA/mlx5: Clamp log_max_* shifts in query_device and ucontext paths
> 
> This commit replaces the open-coded 1 << cap computations of FW log_max_*
> capabilities with MLX5_CAP_GEN_LOGSZ_U32() and MLX5_CAP_GEN_LOGSZ_U64().
> This covers the query_device, query_port, ucontext and SRQ create paths.
> It also computes max_qp_wr in 64 bits and saturates the max_res_rd_atom
> and max_total_mcast_qp_attach products at U32_MAX.
> 
>> The device, port and ucontext attributes and the SRQ size limit are
>> computed as 1 << cap from 8-bit log_max_* FW capabilities. An untrusted
>> NIC can report 31 or more, which makes the shift undefined. The same
>> applies to 1ull << log_pg_sz with a value of 64 or more.
> 
> [Severity: Low]
> This isn't a bug, but is the "8-bit log_max_*" description accurate? In
> mlx5_ifc.h several of the converted fields are narrower, for example:
> 
> 	u8         log_max_msg[0x5];
> 
> log_tag_matching_list_sz is also 5 bits wide. log_max_klm_list_size and
> log_max_ra_req_qp are 6 bits.
> 
> The diff also changes the tso_caps computation in mlx5_ib_query_device():
> 
> 	resp.tso_caps.max_tso = BIT(max_tso);
> 
> Here max_tso comes from the 5-bit Ethernet cap MLX5_CAP_ETH(max_lso_cap),
> not from a log_max_* general cap, and the commit message doesn't mention
> this change. The BIT() conversion itself looks correct.
> 
> Could the message be updated to cover this change and to fix the field
> width?

Right, will fix the commit message in V1.

> 
>> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
>> index c29102260989f..ace4cdc95c9da 100644
>> --- a/drivers/infiniband/hw/mlx5/main.c
>> +++ b/drivers/infiniband/hw/mlx5/main.c
> 
> [ ... ]
> 
>> @@ -1111,29 +1112,31 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,
>>   	props->max_send_sge = max_sq_sg;
>>   	props->max_recv_sge = max_rq_sg;
>>   	props->max_sge_rd	   = MLX5_MAX_SGE_RD;
>> -	props->max_cq		   = 1 << MLX5_CAP_GEN(mdev, log_max_cq);
>> -	props->max_cqe = (1 << MLX5_CAP_GEN(mdev, log_max_cq_sz)) - 1;
>> +	props->max_cq		   = MLX5_CAP_GEN_LOGSZ_U32(mdev, log_max_cq);
>> +	props->max_cqe = MLX5_CAP_GEN_LOGSZ_U32(mdev, log_max_cq_sz) - 1;
> 
> [Severity: Low]
> If FW reports log_max_cq_sz >= 31, this advertises max_cqe as 2^31 - 1.
> Can the CQ create path actually honour that value?
> 
> mlx5_ib_create_user_cq() in cq.c rounds the requested size up and then
> checks it against INT_MAX:
> 
> 	if (entries > max_cq_entries || entries > INT_MAX)
> 		return -EINVAL;
> 
> cq_roundup_entries() is 1ULL << fls64(cqe), so any cqe in
> [2^30, 2^31 - 1] rounds up to 2^31 and gets -EINVAL. That makes the real
> maximum 2^30 - 1.
> 
> Would a consumer that sizes its CQ from attrs.max_cqe then fail to create
> the CQ? For log_max_cq_sz <= 30 the two values agree.
> 

Right, V1 will cap the advertised max_cqe at the largest CQ size the 
create path accepts.

> [ ... ]
> 
>> diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
>> index 6a4a937d97ce9..3fbb0eaf12558 100644
>> --- a/drivers/infiniband/hw/mlx5/srq.c
>> +++ b/drivers/infiniband/hw/mlx5/srq.c
>> @@ -198,7 +198,7 @@ int mlx5_ib_create_srq(struct ib_srq *ib_srq,
>>   	size_t buf_size;
>>   	int err;
>>   	struct mlx5_srq_attr in = {};
>> -	__u32 max_srq_wqes = 1 << MLX5_CAP_GEN(dev->mdev, log_max_srq_sz);
>> +	__u32 max_srq_wqes = MLX5_CAP_GEN_LOGSZ_U32(dev->mdev, log_max_srq_sz);
> 
> [Severity: Medium]
> With log_max_srq_sz >= 31 this limit now saturates at 2^31. Can the rest
> of mlx5_ib_create_srq() handle queue sizes that large?
> 
> In adversarial mode the caps filter passes log_max_srq_sz through
> unchanged (MLX5_HCA_CAP_ALLOW in caps_filter.c). A uverbs user could then
> request max_wr = 0x7fffffff, which passes the
> init_attr->attr.max_wr >= max_srq_wqes check. After that:
> 
> 	srq->msrq.max    = roundup_pow_of_two(init_attr->attr.max_wr + 1);
> 
> This stores 2^31 into the int max field of struct mlx5_core_srq, which
> becomes INT_MIN.
> 
> 	buf_size = srq->msrq.max * desc_size;
> 	if (buf_size < desc_size)
> 		return -EINVAL;
> 
> When msrq.max >= 2^26, this size_t product is a power of two >= 2^31. It
> passes the wrap check. However, create_srq_user() and create_srq_kernel()
> take int buf_size, so the value is truncated to 0 or INT_MIN.
> 
> On the user path the truncated value becomes the min_size passed to
> ib_umem_get_desc_check(), so a dmabuf descriptor of any size is accepted.
> 
> in.log_size = ilog2(srq->msrq.max) still describes the full queue to FW.
> mlx5_cmd_create_srq()->create_srq_cmd()->__set_srq_page_size() then calls
> get_pas_size(), which computes:
> 
> 	u32 rq_sz	  = 1 << (log_srq_size + 4 + log_rq_stride);
> 
> Here the shift count is 32 to 36. That is the same kind of undefined shift
> this commit removes.
> 
> The garbage result feeds the WARN_ON() that compares against
> ib_umem_num_dma_blocks() in __set_srq_page_size(). A local user could
> make that WARN fire, or create an SRQ whose FW-declared size doesn't match
> the pinned buffer.
> 
> Finally, init_attr->attr.max_wr = srq->msrq.max - 1 is a signed overflow
> when msrq.max is INT_MIN.
> 
> The downstream overflow could already be reached with log_max_srq_sz
> values of 26 to 30. However, the commit message notes "the limits can now
> reach 2^31" and fixes other consumers of 2^31 values.
> 
> Should the SRQ path get a tighter bound as well? For example, it could
> reject msrq.max > INT_MAX / desc_size, or keep
> log_size + 4 + wqe_shift below 32. 

Right, V1 rejects an SRQ size and a buffer size above INT_MAX, computing
the buffer size with check_mul_overflow(). This also keeps log_size + 4 
+ wqe_shift below 32.

Yishai