Re: [PATCH rdma-next v2 10/13] RDMA/core: queue pair attributes for unconnected transports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] RDMA/core: unmasked semantics fields used and copied in create_qp_opt_attrs()
- [Medium] RDMA/core: missing IB_DEVICE_RU check for IB_QPT_RU in create_qp_opt_attrs()
--

commit 814ed695a9e745ffe7fb729da61fd57b0d746109
Author: Eric Davis <eric.davis@broadcom.com>

RDMA/core: queue pair attributes for unconnected transports

This commit adds support for reliable unconnected (IB_QPT_RU) queue pair
creation attributes in uverbs and ib_verbs, including source ID, job key
(jkey), and ordering/usage semantics. It also tracks the jkey use count
over the lifetime of the QP.

> diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
> index c6225156fe30e..47ca994f9ac96 100644
> --- a/drivers/infiniband/core/uverbs_std_types_qp.c
> +++ b/drivers/infiniband/core/uverbs_std_types_qp.c
> @@ -79,6 +79,139 @@ static void set_caps(struct ib_qp_init_attr *attr,
>  	}
>  }
>  
> +/* the semantics fields, ordering bits and usage flags uverbs knows of */
> +#define QP_SEMANTICS_MASK_ALL	((IB_UVERBS_QP_SEMANTICS_MASK_USAGE << 1) - 1)
> +#define QP_ORDER_ALL		((IB_UVERBS_QP_ORDER_WAW << 1) - 1)
> +#define QP_USAGE_ALL		IB_UVERBS_QP_USAGE_IMM_DATA_RQ
> +
> +/* A QP may relax what its type guarantees on this device, but not ask for
> + * more than the device advertises for it.
> + */
> +static int create_qp_semantics_check(struct ib_device *device,
> +				     const struct ib_qp_semantics *req)
> +{
> +	struct ib_qp_semantics adv = {};
> +	u32 m = req->comp_mask;
> +	int ret;
> +
> +	if (!device->ops.query_qp_semantics)
> +		return -EOPNOTSUPP;
> +
> +	ret = device->ops.query_qp_semantics(device, IB_QPT_RU, &adv);
> +	if (ret)
> +		return ret;
> +
> +	if (m & ~adv.comp_mask)
> +		return -EINVAL;
> +
> +	if (((m & IB_UVERBS_QP_SEMANTICS_MASK_MSG_ORDER) &&
> +	     (req->msg_order & ~adv.msg_order)) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_RAW) &&
> +	     req->max_rdma_raw_size > adv.max_rdma_raw_size) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_WAR) &&
> +	     req->max_rdma_war_size > adv.max_rdma_war_size) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_WAW) &&
> +	     req->max_rdma_waw_size > adv.max_rdma_waw_size) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_PDU) &&
> +	     req->max_pdu > adv.max_pdu) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_IMM) &&
> +	     req->imm_data_size > adv.imm_data_size) ||
> +	    ((m & IB_UVERBS_QP_SEMANTICS_MASK_USAGE) &&
> +	     (req->usage_flags & ~adv.usage_flags)))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/* Process the optional creation attributes. A job to act within, a source
> + * identity, a set of ordering guarantees to ask for. They and the checks on
> + * a requested QP number belong to a reliable unconnected QP.
> + */
> +static int create_qp_opt_attrs(struct uverbs_attr_bundle *attrs,
> +			       struct ib_device *device, struct ib_pd *pd,
> +			       struct ib_qp_init_attr *attr,
> +			       struct ib_qp_semantics *sem)
> +{
> +	struct ib_uverbs_qp_semantics usem = {};
> +	struct ib_jkey *jkey;
> +	bool has_semantics;
> +	int ret;
> +
> +	has_semantics = uverbs_attr_is_valid(attrs,
> +					     UVERBS_ATTR_CREATE_QP_SEMANTICS);
> +
> +	if (attr->qp_type != IB_QPT_RU ||
> +	    !(device->attrs.device_cap_flags & IB_DEVICE_RU)) {
> +		if (has_semantics ||
> +		    uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_QP_SRC_ID) ||
> +		    uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_QP_JOB_KEY))
> +			return -EOPNOTSUPP;
> +
> +		return 0;
> +	}

[Severity: Medium]
What happens in UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE) ->
create_qp_opt_attrs() when userspace requests attr.qp_type == IB_QPT_RU on
a device that does not advertise IB_DEVICE_RU, and omits the optional
UVERBS_ATTR_CREATE_QP_SEMANTICS, UVERBS_ATTR_CREATE_QP_SRC_ID, and
UVERBS_ATTR_CREATE_QP_JOB_KEY attributes?

Because UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE) adds IB_UVERBS_QPT_RU to
the switch statement without checking IB_DEVICE_RU, and this check combines
attr->qp_type != IB_QPT_RU with !(device->attrs.device_cap_flags &
IB_DEVICE_RU), create_qp_opt_attrs() returns 0 instead of -EOPNOTSUPP.

Returning 0 early here also skips the IB_QP_CREATE_SOURCE_QPN validation
below (checking attr->source_qpn against IB_UVERBS_QPN_UET_RESERVED_MASK
and requiring attr->jkey for relative QPNs) and allows QP creation to
proceed to ib_create_qp_user() on a device without IB_DEVICE_RU.

Should create_qp_opt_attrs() return -EOPNOTSUPP whenever attr->qp_type ==
IB_QPT_RU and !(device->attrs.device_cap_flags & IB_DEVICE_RU)?

> +
> +	if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_QP_SRC_ID)) {
> +		ret = uverbs_copy_from(&attr->src_id, attrs,
> +				       UVERBS_ATTR_CREATE_QP_SRC_ID);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_QP_JOB_KEY)) {
> +		jkey = uverbs_attr_get_obj(attrs,
> +					   UVERBS_ATTR_CREATE_QP_JOB_KEY);
> +		if (IS_ERR(jkey))
> +			return PTR_ERR(jkey);
> +
> +		/* a key acts within the PD it was made on */
> +		if (jkey->pd != pd)
> +			return -EINVAL;
> +
> +		attr->jkey = jkey;
> +	}
> +
> +	if (attr->create_flags & IB_QP_CREATE_SOURCE_QPN) {
> +		if (attr->source_qpn & IB_UVERBS_QPN_UET_RESERVED_MASK)
> +			return -EINVAL;
> +
> +		/* a relative QP is named within a job, so it needs a key */
> +		if (!(attr->source_qpn &
> +		      IB_UVERBS_QPN_UET_ABSOLUTE_ADDR_BIT) &&
> +		    !attr->jkey)
> +			return -EINVAL;
> +	}
> +
> +	if (has_semantics) {
> +		ret = uverbs_copy_from_or_zero(&usem, attrs,
> +					       UVERBS_ATTR_CREATE_QP_SEMANTICS);
> +		if (ret)
> +			return ret;
> +
> +		if ((usem.comp_mask & ~QP_SEMANTICS_MASK_ALL) ||
> +		    (usem.msg_order & ~QP_ORDER_ALL) ||
> +		    (usem.usage_flags & ~QP_USAGE_ALL) ||
> +		    memchr_inv(usem.reserved, 0, sizeof(usem.reserved)))
> +			return -EINVAL;
> +
> +		sem->comp_mask = usem.comp_mask;
> +		sem->msg_order = usem.msg_order;
> +		sem->max_rdma_raw_size = usem.max_rdma_raw_size;
> +		sem->max_rdma_war_size = usem.max_rdma_war_size;
> +		sem->max_rdma_waw_size = usem.max_rdma_waw_size;
> +		sem->max_pdu = usem.max_pdu;
> +		sem->usage_flags = usem.usage_flags;
> +		sem->imm_data_size = usem.imm_data_size;
> +
> +		ret = create_qp_semantics_check(device, sem);
> +		if (ret)
> +			return ret;
> +
> +		attr->qp_semantics = sem;
> +	}

[Severity: Medium]
Should the checks and assignments on the fields of usem in
create_qp_opt_attrs() be gated by their corresponding bits in
usem.comp_mask?

When UVERBS_ATTR_CREATE_QP_SEMANTICS is provided, individual fields in
struct ib_uverbs_qp_semantics are gated by IB_UVERBS_QP_SEMANTICS_MASK_*
bits in usem.comp_mask.

However, create_qp_opt_attrs() checks (usem.msg_order & ~QP_ORDER_ALL) and
(usem.usage_flags & ~QP_USAGE_ALL) unconditionally even when
IB_UVERBS_QP_SEMANTICS_MASK_MSG_ORDER or IB_UVERBS_QP_SEMANTICS_MASK_USAGE
is not set in usem.comp_mask, which can reject valid requests with -EINVAL
if unused fields in the user structure were not zeroed.

In addition, create_qp_opt_attrs() copies all fields from usem into sem
regardless of usem.comp_mask, whereas create_qp_semantics_check() only
validates the fields in sem whose bits are set in req->comp_mask. As a
result, any field whose bit is clear in usem.comp_mask bypasses
create_qp_semantics_check() and is passed with unvalidated user values in
attr->qp_semantics to dev->ops.create_qp().

[ ... ]

> @@ -197,6 +331,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
>  	case IB_UVERBS_QPT_UC:
>  	case IB_UVERBS_QPT_UD:
>  	case IB_UVERBS_QPT_XRC_INI:
> +	case IB_UVERBS_QPT_RU:
>  	case IB_UVERBS_QPT_DRIVER:
>  		if (uverbs_attr_is_valid(attrs,
>  					 UVERBS_ATTR_CREATE_QP_XRCD_HANDLE) ||
> @@ -271,6 +406,10 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
>  		attr.create_flags |= IB_QP_CREATE_SOURCE_QPN;
>  	}
>  
> +	ret = create_qp_opt_attrs(attrs, device, pd, &attr, &qp_semantics);
> +	if (ret)
> +		return ret;
> +
>  	ret = create_qp_attr_set(attrs, device, &attr, &qp_attr,
>  				 &qp_attr_mask);
>  	if (ret)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-uet-ib-core-v2-0-8bd654f25370@broadcom.com?part=10