* [PATCH] IB/mlx5: Fix binary compatibility with libmlx5
@ 2014-01-29 14:27 Eli Cohen
[not found] ` <1391005649-17932-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Eli Cohen @ 2014-01-29 14:27 UTC (permalink / raw)
To: roland-DgEjT+Ai2ygdnm+yROfE0A
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen
Commit c1be523 "Fix micro UAR allocator" broke binary compatibility between
libmlx5 and mlx5_ib since it defines a different value to the number of micro
UARs per page, leading to wrong calculation in libmlx5. This patch defines
struct mlx5_ib_alloc_ucontext_req_v2 as an extension to struct
mlx5_ib_alloc_ucontext_req. The extended size is determined in
mlx5_ib_alloc_ucontext() and in case of old library we use uuarn 0 which works
fine. For new libraries we use the more sophisticated allocation algorithm.
Fixes: c1be523 ('Fix micro UAR allocator')
Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
Hi Roland,
This fixes a regression introduced during the 3.14 merge window, and must
be applied for 3.14, thanks
Eli
drivers/infiniband/hw/mlx5/main.c | 19 +++++++++++++++++--
drivers/infiniband/hw/mlx5/qp.c | 11 +++++++++--
drivers/infiniband/hw/mlx5/user.h | 7 +++++++
include/linux/mlx5/driver.h | 1 +
4 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 9660d09..e24d912 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -536,24 +536,38 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
struct ib_udata *udata)
{
struct mlx5_ib_dev *dev = to_mdev(ibdev);
- struct mlx5_ib_alloc_ucontext_req req;
+ struct mlx5_ib_alloc_ucontext_req_v2 req;
struct mlx5_ib_alloc_ucontext_resp resp;
struct mlx5_ib_ucontext *context;
struct mlx5_uuar_info *uuari;
struct mlx5_uar *uars;
int gross_uuars;
int num_uars;
+ int ver;
int uuarn;
int err;
int i;
+ int reqlen;
if (!dev->ib_active)
return ERR_PTR(-EAGAIN);
- err = ib_copy_from_udata(&req, udata, sizeof(req));
+ memset(&req, 0, sizeof(req));
+ reqlen = udata->inlen - sizeof(struct ib_uverbs_get_context);
+ if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req))
+ ver = 0;
+ else if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req_v2))
+ ver = 2;
+ else
+ return ERR_PTR(-EINVAL);
+
+ err = ib_copy_from_udata(&req, udata, reqlen);
if (err)
return ERR_PTR(err);
+ if (req.flags || req.reserved)
+ return ERR_PTR(-EINVAL);
+
if (req.total_num_uuars > MLX5_MAX_UUARS)
return ERR_PTR(-ENOMEM);
@@ -626,6 +640,7 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
if (err)
goto out_uars;
+ uuari->ver = ver;
uuari->num_low_latency_uuars = req.num_low_latency_uuars;
uuari->uars = uars;
uuari->num_uars = num_uars;
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 492dc33..300475c 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -430,11 +430,17 @@ static int alloc_uuar(struct mlx5_uuar_info *uuari,
break;
case MLX5_IB_LATENCY_CLASS_MEDIUM:
- uuarn = alloc_med_class_uuar(uuari);
+ if (uuari->ver < 2)
+ uuarn = -ENOMEM;
+ else
+ uuarn = alloc_med_class_uuar(uuari);
break;
case MLX5_IB_LATENCY_CLASS_HIGH:
- uuarn = alloc_high_class_uuar(uuari);
+ if (uuari->ver < 2)
+ uuarn = -ENOMEM;
+ else
+ uuarn = alloc_high_class_uuar(uuari);
break;
case MLX5_IB_LATENCY_CLASS_FAST_PATH:
@@ -559,6 +565,7 @@ static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
}
}
+
uar_index = uuarn_to_uar_index(&context->uuari, uuarn);
mlx5_ib_dbg(dev, "uuarn 0x%x, uar_index 0x%x\n", uuarn, uar_index);
diff --git a/drivers/infiniband/hw/mlx5/user.h b/drivers/infiniband/hw/mlx5/user.h
index 32a2a5d..0f4f8e4 100644
--- a/drivers/infiniband/hw/mlx5/user.h
+++ b/drivers/infiniband/hw/mlx5/user.h
@@ -62,6 +62,13 @@ struct mlx5_ib_alloc_ucontext_req {
__u32 num_low_latency_uuars;
};
+struct mlx5_ib_alloc_ucontext_req_v2 {
+ __u32 total_num_uuars;
+ __u32 num_low_latency_uuars;
+ __u32 flags;
+ __u32 reserved;
+};
+
struct mlx5_ib_alloc_ucontext_resp {
__u32 qp_tab_size;
__u32 bf_reg_size;
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 554548c..32cb18c 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -227,6 +227,7 @@ struct mlx5_uuar_info {
* protect uuar allocation data structs
*/
struct mutex lock;
+ u32 ver;
};
struct mlx5_bf {
--
1.8.5.2
--
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] 4+ messages in thread
* Re: [PATCH] IB/mlx5: Fix binary compatibility with libmlx5
[not found] ` <1391005649-17932-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2014-01-29 20:48 ` Yann Droneaud
[not found] ` <1391028523.23180.63.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Yann Droneaud @ 2014-01-29 20:48 UTC (permalink / raw)
To: Eli Cohen
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen
Hi,
Le mercredi 29 janvier 2014 à 16:27 +0200, Eli Cohen a écrit :
> Commit c1be523 "Fix micro UAR allocator" broke binary compatibility between
^^^^^^^
12 digits identifier are the norm for kernel. Please update your git
configuration:
git config --global core.abbrev 12
See http://lwn.net/Articles/571980/
http://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
> libmlx5 and mlx5_ib since it defines a different value to the number of micro
> UARs per page, leading to wrong calculation in libmlx5. This patch defines
> struct mlx5_ib_alloc_ucontext_req_v2 as an extension to struct
> mlx5_ib_alloc_ucontext_req. The extended size is determined in
> mlx5_ib_alloc_ucontext() and in case of old library we use uuarn 0 which works
> fine. For new libraries we use the more sophisticated allocation algorithm.
>
> Fixes: c1be523 ('Fix micro UAR allocator')
^^^^^^^
Likewise
> Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>
> Hi Roland,
>
> This fixes a regression introduced during the 3.14 merge window, and must
> be applied for 3.14, thanks
>
> Eli
>
> drivers/infiniband/hw/mlx5/main.c | 19 +++++++++++++++++--
> drivers/infiniband/hw/mlx5/qp.c | 11 +++++++++--
> drivers/infiniband/hw/mlx5/user.h | 7 +++++++
> include/linux/mlx5/driver.h | 1 +
> 4 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 9660d09..e24d912 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -536,24 +536,38 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
> struct ib_udata *udata)
> {
> struct mlx5_ib_dev *dev = to_mdev(ibdev);
> - struct mlx5_ib_alloc_ucontext_req req;
> + struct mlx5_ib_alloc_ucontext_req_v2 req;
> struct mlx5_ib_alloc_ucontext_resp resp;
> struct mlx5_ib_ucontext *context;
> struct mlx5_uuar_info *uuari;
> struct mlx5_uar *uars;
> int gross_uuars;
> int num_uars;
> + int ver;
> int uuarn;
> int err;
> int i;
> + int reqlen;
>
> if (!dev->ib_active)
> return ERR_PTR(-EAGAIN);
>
> - err = ib_copy_from_udata(&req, udata, sizeof(req));
> + memset(&req, 0, sizeof(req));
> + reqlen = udata->inlen - sizeof(struct ib_uverbs_get_context);
I'm not sure how this could work without subtracting sizeof(struct
ib_uverbs_cmd_hdr).
As I explained in "Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow
fixes for v3.13" [1] ib_uverbs_write() does not decrement input length:
it gives hdr.in_words * 4 to the uverbs function, here
ib_uverbs_get_context(). Then, the function built struct ib_udata
without taking care of the extra bytes count in in_len:
struct ib_uverbs_get_context cmd;
...
INIT_UDATA(&udata, buf + sizeof cmd,
(unsigned long) cmd.response + sizeof resp,
in_len - sizeof cmd, out_len - sizeof resp);
Driver mthca does some handling which look like to what's proposed in
your patch, but takes care of subtracting the header size from the input
length, see mthca_reg_user_mr()[2].
[1]
<http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
[2]
<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/mthca/mthca_provider.c?id=0e47c969c65e213421450c31043353ebe3c67e0c#n988>
> + if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req))
> + ver = 0;
> + else if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req_v2))
> + ver = 2;
> + else
> + return ERR_PTR(-EINVAL);
> +
Doing so introduce a subtle regression: there was no check on the length
before, so it was legal to pass a input buffer far larger than needed,
aka. trailing garbage.
With such new test in place, it's no more allowed, and this is a
regression. It's not a big issue, but a little departure from current
behavor.
BTW, this is the correct way to handle the request, every other uverbs
functions should behave like this, eg. being strict on its accepted
input.
> + err = ib_copy_from_udata(&req, udata, reqlen);
> if (err)
> return ERR_PTR(err);
>
> + if (req.flags || req.reserved)
> + return ERR_PTR(-EINVAL);
> +
Just like this :)
> if (req.total_num_uuars > MLX5_MAX_UUARS)
> return ERR_PTR(-ENOMEM);
>
> @@ -626,6 +640,7 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
> if (err)
> goto out_uars;
>
> + uuari->ver = ver;
> uuari->num_low_latency_uuars = req.num_low_latency_uuars;
> uuari->uars = uars;
> uuari->num_uars = num_uars;
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index 492dc33..300475c 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -430,11 +430,17 @@ static int alloc_uuar(struct mlx5_uuar_info *uuari,
> break;
>
> case MLX5_IB_LATENCY_CLASS_MEDIUM:
> - uuarn = alloc_med_class_uuar(uuari);
> + if (uuari->ver < 2)
> + uuarn = -ENOMEM;
In the commit message, you specified that uuarn is set to 0 when v1 is
used. But here it's set to -ENOMEM.
> + else
> + uuarn = alloc_med_class_uuar(uuari);
> break;
>
> case MLX5_IB_LATENCY_CLASS_HIGH:
> - uuarn = alloc_high_class_uuar(uuari);
> + if (uuari->ver < 2)
> + uuarn = -ENOMEM;
Likewise.
> + else
> + uuarn = alloc_high_class_uuar(uuari);
> break;
>
> case MLX5_IB_LATENCY_CLASS_FAST_PATH:
> @@ -559,6 +565,7 @@ static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> }
> }
>
> +
Remove this white space.
> uar_index = uuarn_to_uar_index(&context->uuari, uuarn);
> mlx5_ib_dbg(dev, "uuarn 0x%x, uar_index 0x%x\n", uuarn, uar_index);
>
> diff --git a/drivers/infiniband/hw/mlx5/user.h b/drivers/infiniband/hw/mlx5/user.h
> index 32a2a5d..0f4f8e4 100644
> --- a/drivers/infiniband/hw/mlx5/user.h
> +++ b/drivers/infiniband/hw/mlx5/user.h
> @@ -62,6 +62,13 @@ struct mlx5_ib_alloc_ucontext_req {
> __u32 num_low_latency_uuars;
> };
>
> +struct mlx5_ib_alloc_ucontext_req_v2 {
> + __u32 total_num_uuars;
> + __u32 num_low_latency_uuars;
> + __u32 flags;
> + __u32 reserved;
> +};
> +
> struct mlx5_ib_alloc_ucontext_resp {
> __u32 qp_tab_size;
> __u32 bf_reg_size;
> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> index 554548c..32cb18c 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -227,6 +227,7 @@ struct mlx5_uuar_info {
> * protect uuar allocation data structs
> */
> struct mutex lock;
> + u32 ver;
> };
>
> struct mlx5_bf {
Regards.
--
Yann Droneaud
OPTEYA
--
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] 4+ messages in thread
* Re: [PATCH] IB/mlx5: Fix binary compatibility with libmlx5
[not found] ` <1391028523.23180.63.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2014-01-29 23:33 ` Eli Cohen
2014-01-30 9:27 ` Yann Droneaud
0 siblings, 1 reply; 4+ messages in thread
From: Eli Cohen @ 2014-01-29 23:33 UTC (permalink / raw)
To: Yann Droneaud
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen
On Wed, Jan 29, 2014 at 09:48:43PM +0100, Yann Droneaud wrote:
Yann,
thanks for reviewing, your comments are helpful :-)
>
> 12 digits identifier are the norm for kernel. Please update your git
> configuration:
>
> git config --global core.abbrev 12
>
> See http://lwn.net/Articles/571980/
> http://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
>
> > libmlx5 and mlx5_ib since it defines a different value to the number of micro
> > UARs per page, leading to wrong calculation in libmlx5. This patch defines
> > struct mlx5_ib_alloc_ucontext_req_v2 as an extension to struct
> > mlx5_ib_alloc_ucontext_req. The extended size is determined in
> > mlx5_ib_alloc_ucontext() and in case of old library we use uuarn 0 which works
> > fine. For new libraries we use the more sophisticated allocation algorithm.
> >
> > Fixes: c1be523 ('Fix micro UAR allocator')
> ^^^^^^^
> Likewise
Will fix.
>
> I'm not sure how this could work without subtracting sizeof(struct
> ib_uverbs_cmd_hdr).
struct ib_uverbs_get_context happens to have the same size as struct
ib_uverbs_cmd_hdr so it passed all my sanity tests. Correct, will fix
that too.
>
> As I explained in "Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow
> fixes for v3.13" [1] ib_uverbs_write() does not decrement input length:
> it gives hdr.in_words * 4 to the uverbs function, here
> ib_uverbs_get_context(). Then, the function built struct ib_udata
> without taking care of the extra bytes count in in_len:
>
> struct ib_uverbs_get_context cmd;
> ...
> INIT_UDATA(&udata, buf + sizeof cmd,
> (unsigned long) cmd.response + sizeof resp,
> in_len - sizeof cmd, out_len - sizeof resp);
So this just seems broken and the fix is to do the subtraction here so
the hardware driver gets the correct size without needing to subtract
the extra bytes, like this:
INIT_UDATA(&udata, buf + sizeof cmd,
(unsigned long) cmd.response + sizeof resp,
in_len - sizeof cmd - sizeof (struct ib_uverbs_cmd_hdr),
out_len - sizeof resp);
And the hardware driver gets the correct size for its struct.
>
> Driver mthca does some handling which look like to what's proposed in
> your patch, but takes care of subtracting the header size from the input
> length, see mthca_reg_user_mr()[2].
>
> [1]
> <http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
>
> [2]
> <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/mthca/mthca_provider.c?id=0e47c969c65e213421450c31043353ebe3c67e0c#n988>
>
> > + if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req))
> > + ver = 0;
> > + else if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req_v2))
> > + ver = 2;
> > + else
> > + return ERR_PTR(-EINVAL);
> > +
>
> Doing so introduce a subtle regression: there was no check on the length
> before, so it was legal to pass a input buffer far larger than needed,
> aka. trailing garbage.
>
> With such new test in place, it's no more allowed, and this is a
> regression. It's not a big issue, but a little departure from current
> behavor.
Well it's a regression if there was out there another library for mlx5
out there which misbehaves and there is any as far as I know. So I
think it's safe to add this strict check.
>
> BTW, this is the correct way to handle the request, every other uverbs
> functions should behave like this, eg. being strict on its accepted
> input.
>
> > + err = ib_copy_from_udata(&req, udata, reqlen);
> > if (err)
> > return ERR_PTR(err);
> >
> > + if (req.flags || req.reserved)
> > + return ERR_PTR(-EINVAL);
> > +
>
> Just like this :)
>
> > if (req.total_num_uuars > MLX5_MAX_UUARS)
> > return ERR_PTR(-ENOMEM);
> >
> > @@ -626,6 +640,7 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
> > if (err)
> > goto out_uars;
> >
> > + uuari->ver = ver;
> > uuari->num_low_latency_uuars = req.num_low_latency_uuars;
> > uuari->uars = uars;
> > uuari->num_uars = num_uars;
> > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > index 492dc33..300475c 100644
> > --- a/drivers/infiniband/hw/mlx5/qp.c
> > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > @@ -430,11 +430,17 @@ static int alloc_uuar(struct mlx5_uuar_info *uuari,
> > break;
> >
> > case MLX5_IB_LATENCY_CLASS_MEDIUM:
> > - uuarn = alloc_med_class_uuar(uuari);
> > + if (uuari->ver < 2)
> > + uuarn = -ENOMEM;
>
> In the commit message, you specified that uuarn is set to 0 when v1 is
> used. But here it's set to -ENOMEM.
>
If you look at qp.c you can see that the code falls back from high to
medium to low class. Low class always succeeds and gives 0.
> > + else
> > + uuarn = alloc_med_class_uuar(uuari);
> > break;
> >
> > case MLX5_IB_LATENCY_CLASS_HIGH:
> > - uuarn = alloc_high_class_uuar(uuari);
> > + if (uuari->ver < 2)
> > + uuarn = -ENOMEM;
>
> Likewise.
>
> > + else
> > + uuarn = alloc_high_class_uuar(uuari);
> > break;
> >
> > case MLX5_IB_LATENCY_CLASS_FAST_PATH:
> > @@ -559,6 +565,7 @@ static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> > }
> > }
> >
> > +
>
> Remove this white space.
>
> > uar_index = uuarn_to_uar_index(&context->uuari, uuarn);
> > mlx5_ib_dbg(dev, "uuarn 0x%x, uar_index 0x%x\n", uuarn, uar_index);
> >
> > diff --git a/drivers/infiniband/hw/mlx5/user.h b/drivers/infiniband/hw/mlx5/user.h
> > index 32a2a5d..0f4f8e4 100644
> > --- a/drivers/infiniband/hw/mlx5/user.h
> > +++ b/drivers/infiniband/hw/mlx5/user.h
> > @@ -62,6 +62,13 @@ struct mlx5_ib_alloc_ucontext_req {
> > __u32 num_low_latency_uuars;
> > };
> >
> > +struct mlx5_ib_alloc_ucontext_req_v2 {
> > + __u32 total_num_uuars;
> > + __u32 num_low_latency_uuars;
> > + __u32 flags;
> > + __u32 reserved;
> > +};
> > +
>
> > struct mlx5_ib_alloc_ucontext_resp {
> > __u32 qp_tab_size;
> > __u32 bf_reg_size;
> > diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> > index 554548c..32cb18c 100644
> > --- a/include/linux/mlx5/driver.h
> > +++ b/include/linux/mlx5/driver.h
> > @@ -227,6 +227,7 @@ struct mlx5_uuar_info {
> > * protect uuar allocation data structs
> > */
> > struct mutex lock;
> > + u32 ver;
> > };
> >
> > struct mlx5_bf {
>
> Regards.
>
> --
> Yann Droneaud
> OPTEYA
>
--
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] 4+ messages in thread
* Re: [PATCH] IB/mlx5: Fix binary compatibility with libmlx5
2014-01-29 23:33 ` Eli Cohen
@ 2014-01-30 9:27 ` Yann Droneaud
0 siblings, 0 replies; 4+ messages in thread
From: Yann Droneaud @ 2014-01-30 9:27 UTC (permalink / raw)
To: Eli Cohen
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, Eli Cohen
Hi,
Le jeudi 30 janvier 2014 à 01:33 +0200, Eli Cohen a écrit :
> On Wed, Jan 29, 2014 at 09:48:43PM +0100, Yann Droneaud wrote:
> Yann,
> thanks for reviewing, your comments are helpful :-)
>
> >
> > 12 digits identifier are the norm for kernel. Please update your git
> > configuration:
> >
> > git config --global core.abbrev 12
> >
> > See http://lwn.net/Articles/571980/
> > http://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
> >
> > > libmlx5 and mlx5_ib since it defines a different value to the number of micro
> > > UARs per page, leading to wrong calculation in libmlx5. This patch defines
> > > struct mlx5_ib_alloc_ucontext_req_v2 as an extension to struct
> > > mlx5_ib_alloc_ucontext_req. The extended size is determined in
> > > mlx5_ib_alloc_ucontext() and in case of old library we use uuarn 0 which works
> > > fine. For new libraries we use the more sophisticated allocation algorithm.
> > >
> > > Fixes: c1be523 ('Fix micro UAR allocator')
> > ^^^^^^^
> > Likewise
>
> Will fix.
> >
> > I'm not sure how this could work without subtracting sizeof(struct
> > ib_uverbs_cmd_hdr).
>
> struct ib_uverbs_get_context happens to have the same size as struct
> ib_uverbs_cmd_hdr so it passed all my sanity tests. Correct, will fix
> that too.
> >
> > As I explained in "Re: [PATCHv4 for-3.13 00/10] create_flow/destroy_flow
> > fixes for v3.13" [1] ib_uverbs_write() does not decrement input length:
> > it gives hdr.in_words * 4 to the uverbs function, here
> > ib_uverbs_get_context(). Then, the function built struct ib_udata
> > without taking care of the extra bytes count in in_len:
> >
> > struct ib_uverbs_get_context cmd;
> > ...
> > INIT_UDATA(&udata, buf + sizeof cmd,
> > (unsigned long) cmd.response + sizeof resp,
> > in_len - sizeof cmd, out_len - sizeof resp);
>
> So this just seems broken and the fix is to do the subtraction here so
> the hardware driver gets the correct size without needing to subtract
> the extra bytes, like this:
> INIT_UDATA(&udata, buf + sizeof cmd,
> (unsigned long) cmd.response + sizeof resp,
> in_len - sizeof cmd - sizeof (struct ib_uverbs_cmd_hdr),
> out_len - sizeof resp);
>
> And the hardware driver gets the correct size for its struct.
>
It's not the place to address the issue: the proper fix is to subtract
it in ib_uverbs_write().
I have a patch for this but I'm not going to submit it for 3.14: it's
part of a patchset that need testing and polishing. I will submit this
large patchset for 3.15 (after v3.14-rc1).
So you have to subtract the header size in your driver, just like mthca
does. And don't worry I will take care of removing it in my patch that
remove the header size from input length.
> >
> > Driver mthca does some handling which look like to what's proposed in
> > your patch, but takes care of subtracting the header size from the input
> > length, see mthca_reg_user_mr()[2].
> >
> > [1]
> > <http://marc.info/?i=1387493822.11925.217.camel-bi+AKbBUZKY6gyzm1THtWWGXanvQGlWp@public.gmane.orgmain>
> >
> > [2]
> > <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/mthca/mthca_provider.c?id=0e47c969c65e213421450c31043353ebe3c67e0c#n988>
> >
> > > + if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req))
> > > + ver = 0;
> > > + else if (reqlen == sizeof(struct mlx5_ib_alloc_ucontext_req_v2))
> > > + ver = 2;
> > > + else
> > > + return ERR_PTR(-EINVAL);
> > > +
> >
> > Doing so introduce a subtle regression: there was no check on the length
> > before, so it was legal to pass a input buffer far larger than needed,
> > aka. trailing garbage.
> >
> > With such new test in place, it's no more allowed, and this is a
> > regression. It's not a big issue, but a little departure from current
> > behavor.
> Well it's a regression if there was out there another library for mlx5
> out there which misbehaves and there is any as far as I know. So I
> think it's safe to add this strict check.
Yes I think so.
But as I'm used to write it here, there's no clear line drawn: where is
the ABI that we should enforce ? Is it the kernel uverbs data structure
for response and request or libuverbs/lib<hw> ? In the first case, your
change add a regression, in the second case, it's not. It's just a
theoretical question. In practice, as you wrote, we can say there's no
other library that implement the userspace part of mlx5 rdma driver.
> >
> > BTW, this is the correct way to handle the request, every other uverbs
> > functions should behave like this, eg. being strict on its accepted
> > input.
> >
> > > + err = ib_copy_from_udata(&req, udata, reqlen);
> > > if (err)
> > > return ERR_PTR(err);
> > >
> > > + if (req.flags || req.reserved)
> > > + return ERR_PTR(-EINVAL);
> > > +
> >
> > Just like this :)
> >
> > > if (req.total_num_uuars > MLX5_MAX_UUARS)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > @@ -626,6 +640,7 @@ static struct ib_ucontext *mlx5_ib_alloc_ucontext(struct ib_device *ibdev,
> > > if (err)
> > > goto out_uars;
> > >
> > > + uuari->ver = ver;
> > > uuari->num_low_latency_uuars = req.num_low_latency_uuars;
> > > uuari->uars = uars;
> > > uuari->num_uars = num_uars;
> > > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > > index 492dc33..300475c 100644
> > > --- a/drivers/infiniband/hw/mlx5/qp.c
> > > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > > @@ -430,11 +430,17 @@ static int alloc_uuar(struct mlx5_uuar_info *uuari,
> > > break;
> > >
> > > case MLX5_IB_LATENCY_CLASS_MEDIUM:
> > > - uuarn = alloc_med_class_uuar(uuari);
> > > + if (uuari->ver < 2)
> > > + uuarn = -ENOMEM;
> >
> > In the commit message, you specified that uuarn is set to 0 when v1 is
> > used. But here it's set to -ENOMEM.
> >
> If you look at qp.c you can see that the code falls back from high to
> medium to low class. Low class always succeeds and gives 0.
>
It was not clear from the commit message.
> > > + else
> > > + uuarn = alloc_med_class_uuar(uuari);
> > > break;
> > >
> > > case MLX5_IB_LATENCY_CLASS_HIGH:
> > > - uuarn = alloc_high_class_uuar(uuari);
> > > + if (uuari->ver < 2)
> > > + uuarn = -ENOMEM;
> >
> > Likewise.
> >
> > > + else
> > > + uuarn = alloc_high_class_uuar(uuari);
> > > break;
> > >
Regards
--
Yann Droneaud
OPTEYA
--
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] 4+ messages in thread
end of thread, other threads:[~2014-01-30 9:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-29 14:27 [PATCH] IB/mlx5: Fix binary compatibility with libmlx5 Eli Cohen
[not found] ` <1391005649-17932-1-git-send-email-eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2014-01-29 20:48 ` Yann Droneaud
[not found] ` <1391028523.23180.63.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2014-01-29 23:33 ` Eli Cohen
2014-01-30 9:27 ` Yann Droneaud
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.