* [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h
@ 2017-12-20 23:13 Jason Gunthorpe
[not found] ` <20171220231350.GA19130-uk2M96/98Pc@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-12-20 23:13 UTC (permalink / raw)
To: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Yishai Hadas
In the case of ibv_create_cq_ex this duplicates a check already done
by ibv_cmd_create_cq_ex.
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.
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h
[not found] ` <20171220231350.GA19130-uk2M96/98Pc@public.gmane.org>
@ 2017-12-21 5:42 ` Leon Romanovsky
[not found] ` <20171221054216.GF2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2017-12-21 5:42 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Yishai Hadas
[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h
[not found] ` <20171221054216.GF2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
@ 2017-12-21 16:24 ` Jason Gunthorpe
[not found] ` <20171221162442.GC20015-uk2M96/98Pc@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-12-21 16:24 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Yishai Hadas
On Thu, Dec 21, 2017 at 07:42:16AM +0200, Leon Romanovsky wrote:
> 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.
Bugs bugs everywhere.. This is going to be another patch. I would drop
the errno set into check_comp_mask and change all comp_mask users to
call it, and probably drop the _RESERVED uglyness too, but another
patch.
> > 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
K.. Are you going to collect the loose patches on the list? Can you
fix the typo when you grab this one :)
Jason
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h
[not found] ` <20171221162442.GC20015-uk2M96/98Pc@public.gmane.org>
@ 2017-12-21 17:19 ` Leon Romanovsky
[not found] ` <20171221171947.GK2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2017-12-21 17:19 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Yishai Hadas
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
On Thu, Dec 21, 2017 at 09:24:42AM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 21, 2017 at 07:42:16AM +0200, Leon Romanovsky wrote:
>
> > 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.
>
> Bugs bugs everywhere.. This is going to be another patch. I would drop
> the errno set into check_comp_mask and change all comp_mask users to
> call it, and probably drop the _RESERVED uglyness too, but another
> patch.
>
> > > 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
>
> K.. Are you going to collect the loose patches on the list? Can you
> fix the typo when you grab this one :)
No problem, I scheduled for myself time on Sunday to sync list,
patchworks and rdma-core.
Thanks
>
> Jason
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h
[not found] ` <20171221171947.GK2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
@ 2017-12-25 14:32 ` Leon Romanovsky
0 siblings, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2017-12-25 14:32 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Yishai Hadas
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
On Thu, Dec 21, 2017 at 07:19:47PM +0200, Leon Romanovsky wrote:
> On Thu, Dec 21, 2017 at 09:24:42AM -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 21, 2017 at 07:42:16AM +0200, Leon Romanovsky wrote:
> >
> > > 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.
> >
> > Bugs bugs everywhere.. This is going to be another patch. I would drop
> > the errno set into check_comp_mask and change all comp_mask users to
> > call it, and probably drop the _RESERVED uglyness too, but another
> > patch.
> >
> > > > 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
> >
> > K.. Are you going to collect the loose patches on the list? Can you
> > fix the typo when you grab this one :)
>
> No problem, I scheduled for myself time on Sunday to sync list,
> patchworks and rdma-core.
I fixed the patch and changed it to preserve the errno in mlx4 to be EINVAL.
Thanks
>
> Thanks
>
> >
> > Jason
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-25 14:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-20 23:13 [PATCH rdma-core] verbs: Remove comp_mask checks from verbs.h Jason Gunthorpe
[not found] ` <20171220231350.GA19130-uk2M96/98Pc@public.gmane.org>
2017-12-21 5:42 ` Leon Romanovsky
[not found] ` <20171221054216.GF2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
2017-12-21 16:24 ` Jason Gunthorpe
[not found] ` <20171221162442.GC20015-uk2M96/98Pc@public.gmane.org>
2017-12-21 17:19 ` Leon Romanovsky
[not found] ` <20171221171947.GK2942-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
2017-12-25 14:32 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.