On Wed, Dec 20, 2017 at 04:13:50PM -0700, Jason Gunthorpe wrote:
> In the case of ibv_create_cq_ex this duplicates a check already done
> by ibv_cmd_create_cq_ex.
>

Yes and no.

Yes, the analysis is right and it is should be fixed.
No, the ibv_cmd_create_cq_ex check is performed after we entered into
vctx->create_cq_ex(context, cq_attr) callback, which is translated into
mlx4_create_cq_ex and mlx5_create_cq_ex.

Both these functions are performing their checks of cq_attr->comp_mask,
but return different errno. I think that it is perfect timing to start
organize and unite our return errors. They should return the same errno.

mlx4:
470         if (cq_attr->comp_mask & ~CREATE_CQ_SUPPORTED_COMP_MASK) {
471                 errno = ENOTSUP;
472                 return NULL;
473         }
474
475         if (cq_attr->comp_mask & IBV_CQ_INIT_ATTR_MASK_FLAGS &&
476             cq_attr->flags & ~CREATE_CQ_SUPPORTED_FLAGS) {
477                 errno = ENOTSUP;
478                 return NULL;
479         }

mlx5:
365         if (cq_attr->comp_mask & ~CREATE_CQ_SUPPORTED_COMP_MASK) {
366                 mlx5_dbg(fp, MLX5_DBG_CQ,
367                          "Unsupported comp_mask for create_cq\n");
368                 errno = EINVAL;
369                 return NULL;
370         }

> In the case of ibv_queryrt_values_ex, there is no check in the function
> op, which is an ABI mistake. So move the check there.

ibv_queryrt_values_ex -> ibv_query_rt_values_ex

>
> comp_mask should never be tested in the inline trampolines to generate
> EINVAL as the inlines are part of the app and could be from a future
> libibverbs with an expanded comp_mask.
>
> Signed-off-by: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  libibverbs/verbs.h     | 8 --------
>  providers/mlx4/verbs.c | 3 +++
>  providers/mlx5/verbs.c | 3 +++
>  3 files changed, 6 insertions(+), 8 deletions(-)
>
> I missed these when reviewing the earlier patches, and now people are
> cargo-cult copying them into new APIs.
>
> diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
> index 775bad52601781..7b53a6f5dff51f 100644
> --- a/libibverbs/verbs.h
> +++ b/libibverbs/verbs.h
> @@ -1967,11 +1967,6 @@ struct ibv_cq_ex *ibv_create_cq_ex(struct ibv_context *context,
>  		return NULL;
>  	}
>
> -	if (cq_attr->comp_mask & ~(IBV_CQ_INIT_ATTR_MASK_RESERVED - 1)) {
> -		errno = EINVAL;
> -		return NULL;
> -	}
> -
>  	return vctx->create_cq_ex(context, cq_attr);
>  }
>
> @@ -2200,9 +2195,6 @@ ibv_query_rt_values_ex(struct ibv_context *context,
>  	if (!vctx)
>  		return ENOSYS;
>
> -	if (values->comp_mask & ~(IBV_VALUES_MASK_RESERVED - 1))
> -		return EINVAL;
> -
>  	return vctx->query_rt_values(context, values);
>  }
>
> diff --git a/providers/mlx4/verbs.c b/providers/mlx4/verbs.c
> index ec02bd4642b2d2..0a4ed1fde001eb 100644
> --- a/providers/mlx4/verbs.c
> +++ b/providers/mlx4/verbs.c
> @@ -135,6 +135,9 @@ int mlx4_query_rt_values(struct ibv_context *context,
>  	uint32_t comp_mask = 0;
>  	int err = 0;
>
> +	if (!check_comp_mask(values->comp_mask, IBV_VALUES_MASK_RAW_CLOCK))
> +		return EINVAL;
> +
>  	if (values->comp_mask & IBV_VALUES_MASK_RAW_CLOCK) {
>  		uint64_t cycles;
>
> diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
> index a3323fd47651a7..579c0f3402d54d 100644
> --- a/providers/mlx5/verbs.c
> +++ b/providers/mlx5/verbs.c
> @@ -108,6 +108,9 @@ int mlx5_query_rt_values(struct ibv_context *context,
>  	uint32_t comp_mask = 0;
>  	int err = 0;
>
> +	if (!check_comp_mask(values->comp_mask, IBV_VALUES_MASK_RAW_CLOCK))
> +		return EINVAL;
> +
>  	if (values->comp_mask & IBV_VALUES_MASK_RAW_CLOCK) {
>  		uint64_t cycles;
>
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html