[PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Chuck Lever]

Other similar functions (mlx5_ib_post_recv being the closest)
return EINVAL in this case.

Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
Hi Matan, Leon-

I noticed this nit while debugging a problem in xprtrdma.

 drivers/infiniband/hw/mlx5/qp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index ce0a7ab..b0e5498 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		num_sge = wr->num_sge;
 		if (unlikely(num_sge > qp->sq.max_gs)) {
 			mlx5_ib_warn(dev, "\n");
-			err = -ENOMEM;
+			err = -EINVAL;
 			*bad_wr = wr;
 			goto out;
 		}

--
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

Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
> Other similar functions (mlx5_ib_post_recv being the closest)
> return EINVAL in this case.
> 
> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Thanks Chuck,
You are absolutely right, it should be EINVAL error.
Do you mind if I submit it by myself together with other mlx5 fixes?

Looks ok, except title :)
Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

> ---
> Hi Matan, Leon-
> 
> I noticed this nit while debugging a problem in xprtrdma.
> 
>  drivers/infiniband/hw/mlx5/qp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index ce0a7ab..b0e5498 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  		num_sge = wr->num_sge;
>  		if (unlikely(num_sge > qp->sq.max_gs)) {
>  			mlx5_ib_warn(dev, "\n");
> -			err = -ENOMEM;
> +			err = -EINVAL;
>  			*bad_wr = wr;
>  			goto out;
>  		}
> 
> --
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Eli Cohen]

I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.

-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
Sent: Thursday, July 28, 2016 12:23 AM
To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
> Other similar functions (mlx5_ib_post_recv being the closest) return 
> EINVAL in this case.
> 
> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Thanks Chuck,
You are absolutely right, it should be EINVAL error.
Do you mind if I submit it by myself together with other mlx5 fixes?

Looks ok, except title :)
Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

> ---
> Hi Matan, Leon-
> 
> I noticed this nit while debugging a problem in xprtrdma.
> 
>  drivers/infiniband/hw/mlx5/qp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  		num_sge = wr->num_sge;
>  		if (unlikely(num_sge > qp->sq.max_gs)) {
>  			mlx5_ib_warn(dev, "\n");
> -			err = -ENOMEM;
> +			err = -EINVAL;
>  			*bad_wr = wr;
>  			goto out;
>  		}
> 
> --
> 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
--
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

Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Chuck Lever]

Hi Eli-

> On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 
> I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.

All other drivers I reviewed return EINVAL in this case. So
there is an implied API contract already.

The functional issue here is that the consumer has attempted
to post a Send WR with an incorrect num_sge value. sq.max_gs
is a limit also specified by the consumer. There's no dynamic
memory allocation here to fail.

ENOMEM would be correct if a dynamic memory allocation failed,
where posting again with the same arguments is likely to
succeed. In this case, an invalid parameter has been provided
by the consumer, which is a permanent error.

Therefore EINVAL is the correct return.


> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> Sent: Thursday, July 28, 2016 12:23 AM
> To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs
> 
> On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
>> Other similar functions (mlx5_ib_post_recv being the closest) return 
>> EINVAL in this case.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> Thanks Chuck,
> You are absolutely right, it should be EINVAL error.
> Do you mind if I submit it by myself together with other mlx5 fixes?
> 
> Looks ok, except title :)
> Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
>> ---
>> Hi Matan, Leon-
>> 
>> I noticed this nit while debugging a problem in xprtrdma.
>> 
>> drivers/infiniband/hw/mlx5/qp.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
>> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
>> --- a/drivers/infiniband/hw/mlx5/qp.c
>> +++ b/drivers/infiniband/hw/mlx5/qp.c
>> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>> 		num_sge = wr->num_sge;
>> 		if (unlikely(num_sge > qp->sq.max_gs)) {
>> 			mlx5_ib_warn(dev, "\n");
>> -			err = -ENOMEM;
>> +			err = -EINVAL;
>> 			*bad_wr = wr;
>> 			goto out;
>> 		}
>> 
>> --
>> 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

--
Chuck Lever



--
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

RE: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Eli Cohen]

OK, sounds reasonable. So if we follow this reasoning there are more places to fix in the post send function:

static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
                     struct mlx5_wqe_ctrl_seg **ctrl,
                     struct ib_send_wr *wr, unsigned *idx,
                     int *size, int nreq)
{
        int err = 0;

        if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq))) {
                err = -ENOMEM;
                return err;
        }


-----Original Message-----
From: Chuck Lever [mailto:chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org] 
Sent: Thursday, July 28, 2016 8:19 AM
To: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

Hi Eli-

> On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 
> I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.

All other drivers I reviewed return EINVAL in this case. So there is an implied API contract already.

The functional issue here is that the consumer has attempted to post a Send WR with an incorrect num_sge value. sq.max_gs is a limit also specified by the consumer. There's no dynamic memory allocation here to fail.

ENOMEM would be correct if a dynamic memory allocation failed, where posting again with the same arguments is likely to succeed. In this case, an invalid parameter has been provided by the consumer, which is a permanent error.

Therefore EINVAL is the correct return.


> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
> [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> Sent: Thursday, July 28, 2016 12:23 AM
> To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too 
> many SGEs
> 
> On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
>> Other similar functions (mlx5_ib_post_recv being the closest) return 
>> EINVAL in this case.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> Thanks Chuck,
> You are absolutely right, it should be EINVAL error.
> Do you mind if I submit it by myself together with other mlx5 fixes?
> 
> Looks ok, except title :)
> Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
>> ---
>> Hi Matan, Leon-
>> 
>> I noticed this nit while debugging a problem in xprtrdma.
>> 
>> drivers/infiniband/hw/mlx5/qp.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
>> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
>> --- a/drivers/infiniband/hw/mlx5/qp.c
>> +++ b/drivers/infiniband/hw/mlx5/qp.c
>> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>> 		num_sge = wr->num_sge;
>> 		if (unlikely(num_sge > qp->sq.max_gs)) {
>> 			mlx5_ib_warn(dev, "\n");
>> -			err = -ENOMEM;
>> +			err = -EINVAL;
>> 			*bad_wr = wr;
>> 			goto out;
>> 		}
>> 
>> --
>> 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

--
Chuck Lever



--
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

Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Chuck Lever]

> On Jul 28, 2016, at 9:24 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 
> OK, sounds reasonable. So if we follow this reasoning there are more places to fix in the post send function:
> 
> static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
>                     struct mlx5_wqe_ctrl_seg **ctrl,
>                     struct ib_send_wr *wr, unsigned *idx,
>                     int *size, int nreq)
> {
>        int err = 0;
> 
>        if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq))) {
>                err = -ENOMEM;
>                return err;
>        }

I will let mlx5 driver experts sort those out. Feel free to fold my
initial fix into a larger patch (or patch series).


> -----Original Message-----
> From: Chuck Lever [mailto:chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org] 
> Sent: Thursday, July 28, 2016 8:19 AM
> To: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs
> 
> Hi Eli-
> 
>> On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> 
>> I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.
> 
> All other drivers I reviewed return EINVAL in this case. So there is an implied API contract already.
> 
> The functional issue here is that the consumer has attempted to post a Send WR with an incorrect num_sge value. sq.max_gs is a limit also specified by the consumer. There's no dynamic memory allocation here to fail.
> 
> ENOMEM would be correct if a dynamic memory allocation failed, where posting again with the same arguments is likely to succeed. In this case, an invalid parameter has been provided by the consumer, which is a permanent error.
> 
> Therefore EINVAL is the correct return.
> 
> 
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
>> [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
>> Sent: Thursday, July 28, 2016 12:23 AM
>> To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too 
>> many SGEs
>> 
>> On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
>>> Other similar functions (mlx5_ib_post_recv being the closest) return 
>>> EINVAL in this case.
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> 
>> Thanks Chuck,
>> You are absolutely right, it should be EINVAL error.
>> Do you mind if I submit it by myself together with other mlx5 fixes?
>> 
>> Looks ok, except title :)
>> Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> 
>>> ---
>>> Hi Matan, Leon-
>>> 
>>> I noticed this nit while debugging a problem in xprtrdma.
>>> 
>>> drivers/infiniband/hw/mlx5/qp.c |    2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
>>> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
>>> --- a/drivers/infiniband/hw/mlx5/qp.c
>>> +++ b/drivers/infiniband/hw/mlx5/qp.c
>>> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>>> 		num_sge = wr->num_sge;
>>> 		if (unlikely(num_sge > qp->sq.max_gs)) {
>>> 			mlx5_ib_warn(dev, "\n");
>>> -			err = -ENOMEM;
>>> +			err = -EINVAL;
>>> 			*bad_wr = wr;
>>> 			goto out;
>>> 		}
>>> 
>>> --
>>> 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
> 
> --
> Chuck Lever
> 
> 
> 

--
Chuck Lever



--
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

Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3418 bytes --]

On Thu, Jul 28, 2016 at 09:18:33AM -0400, Chuck Lever wrote:
> Hi Eli-
> 
> > On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> > 
> > I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.
> 
> All other drivers I reviewed return EINVAL in this case. So
> there is an implied API contract already.
> 
> The functional issue here is that the consumer has attempted
> to post a Send WR with an incorrect num_sge value. sq.max_gs
> is a limit also specified by the consumer. There's no dynamic
> memory allocation here to fail.
> 
> ENOMEM would be correct if a dynamic memory allocation failed,
> where posting again with the same arguments is likely to
> succeed. In this case, an invalid parameter has been provided
> by the consumer, which is a permanent error.
> 
> Therefore EINVAL is the correct return.

You are absolutely right, this is why I added my Acked-By.

> 
> 
> > -----Original Message-----
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-fy+rA21nqHI@public.gmane.orgrnel.org] On Behalf Of Leon Romanovsky
> > Sent: Thursday, July 28, 2016 12:23 AM
> > To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs
> > 
> > On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
> >> Other similar functions (mlx5_ib_post_recv being the closest) return 
> >> EINVAL in this case.
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > 
> > Thanks Chuck,
> > You are absolutely right, it should be EINVAL error.
> > Do you mind if I submit it by myself together with other mlx5 fixes?
> > 
> > Looks ok, except title :)
> > Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > 
> >> ---
> >> Hi Matan, Leon-
> >> 
> >> I noticed this nit while debugging a problem in xprtrdma.
> >> 
> >> drivers/infiniband/hw/mlx5/qp.c |    2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
> >> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
> >> --- a/drivers/infiniband/hw/mlx5/qp.c
> >> +++ b/drivers/infiniband/hw/mlx5/qp.c
> >> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> >> 		num_sge = wr->num_sge;
> >> 		if (unlikely(num_sge > qp->sq.max_gs)) {
> >> 			mlx5_ib_warn(dev, "\n");
> >> -			err = -ENOMEM;
> >> +			err = -EINVAL;
> >> 			*bad_wr = wr;
> >> 			goto out;
> >> 		}
> >> 
> >> --
> >> 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
> 
> --
> Chuck Lever
> 
> 
> 
> --
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 4871 bytes --]

On Thu, Jul 28, 2016 at 09:39:57AM -0400, Chuck Lever wrote:
> 
> > On Jul 28, 2016, at 9:24 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> > 
> > OK, sounds reasonable. So if we follow this reasoning there are more places to fix in the post send function:
> > 
> > static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
> >                     struct mlx5_wqe_ctrl_seg **ctrl,
> >                     struct ib_send_wr *wr, unsigned *idx,
> >                     int *size, int nreq)
> > {
> >        int err = 0;
> > 
> >        if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq))) {
> >                err = -ENOMEM;
> >                return err;
> >        }
> 
> I will let mlx5 driver experts sort those out. Feel free to fold my
> initial fix into a larger patch (or patch series).

No, this place should return ENOMEM because overflow was due to not
enough memory and not because of incorrectly filled wqe.

> 
> 
> > -----Original Message-----
> > From: Chuck Lever [mailto:chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org] 
> > Sent: Thursday, July 28, 2016 8:19 AM
> > To: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Cc: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Matan Barak <matanb@mellanox.com>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs
> > 
> > Hi Eli-
> > 
> >> On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> >> 
> >> I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.
> > 
> > All other drivers I reviewed return EINVAL in this case. So there is an implied API contract already.
> > 
> > The functional issue here is that the consumer has attempted to post a Send WR with an incorrect num_sge value. sq.max_gs is a limit also specified by the consumer. There's no dynamic memory allocation here to fail.
> > 
> > ENOMEM would be correct if a dynamic memory allocation failed, where posting again with the same arguments is likely to succeed. In this case, an invalid parameter has been provided by the consumer, which is a permanent error.
> > 
> > Therefore EINVAL is the correct return.
> > 
> > 
> >> -----Original Message-----
> >> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
> >> [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> >> Sent: Thursday, July 28, 2016 12:23 AM
> >> To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too 
> >> many SGEs
> >> 
> >> On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
> >>> Other similar functions (mlx5_ib_post_recv being the closest) return 
> >>> EINVAL in this case.
> >>> 
> >>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >> 
> >> Thanks Chuck,
> >> You are absolutely right, it should be EINVAL error.
> >> Do you mind if I submit it by myself together with other mlx5 fixes?
> >> 
> >> Looks ok, except title :)
> >> Acked-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> 
> >>> ---
> >>> Hi Matan, Leon-
> >>> 
> >>> I noticed this nit while debugging a problem in xprtrdma.
> >>> 
> >>> drivers/infiniband/hw/mlx5/qp.c |    2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/infiniband/hw/mlx5/qp.c 
> >>> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
> >>> --- a/drivers/infiniband/hw/mlx5/qp.c
> >>> +++ b/drivers/infiniband/hw/mlx5/qp.c
> >>> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> >>> 		num_sge = wr->num_sge;
> >>> 		if (unlikely(num_sge > qp->sq.max_gs)) {
> >>> 			mlx5_ib_warn(dev, "\n");
> >>> -			err = -ENOMEM;
> >>> +			err = -EINVAL;
> >>> 			*bad_wr = wr;
> >>> 			goto out;
> >>> 		}
> >>> 
> >>> --
> >>> 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
> > 
> > --
> > Chuck Lever
> > 
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> --
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]