shrink struct ib_send_wr

shrink struct ib_send_wr

[Christoph Hellwig]

This series shrinks the WR size by splitting out the different WR
types.

Patch number two is too large for the mailinglist, so if you didn't
get it grab it here:

	http://git.infradead.org/users/hch/rdma.git/commitdiff/30e522ee6c1d7adb614d7308f09fbfd71c6d3e07

or the full git tree at:

	git://git.infradead.org/users/hch/rdma.git wr-cleanup

Changes since the version sent around a couple of times:

 - new patch 1 which rejects invalid opcodes.  Patch 2 was doing
   this implicitly except for UD QPs, but I think we should a)
   do this explicitly and b) ensure this goes into 4.2 and -stable
   as I can see quite a lot of harm from submitting such malformed
   operations
 - patch 2 now covers all drivers including those in staging to
   side step any sort of discussions on the staging tree.
 - patch 2 now explicitly replaces the weird overloading in the mlx5
   driver with an explicit embedding of struct ib_send_wr, similar
   to what we do for all other MRs.
 - new patch to drop another unused send_wr field.

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

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..f9f3921 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.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

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..f9f3921 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.1

[PATCH 3/3] IB: remove xrc_remote_srq_num from struct ib_send_wr

[Christoph Hellwig]

The field is only initialized in mlx, but never used.

If we want to add proper XRC support it should be done with a new
struct ib_xrc_wr.

This shrinks the various WR structures by another 4 bytes.

Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/qp.c | 1 -
 include/rdma/ib_verbs.h         | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 04df156..83a290f 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -2634,7 +2634,6 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		switch (ibqp->qp_type) {
 		case IB_QPT_XRC_INI:
 			xrc = seg;
-			xrc->xrc_srqn = htonl(wr->xrc_remote_srq_num);
 			seg += sizeof(*xrc);
 			size += sizeof(*xrc) / 16;
 			/* fall through */
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 9b29c78..b855189 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1100,7 +1100,6 @@ struct ib_send_wr {
 		__be32		imm_data;
 		u32		invalidate_rkey;
 	} ex;
-	u32			xrc_remote_srq_num;	/* XRC TGT QPs only */
 };
 
 struct ib_rdma_wr {
-- 
1.9.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

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 06:37:32PM +0200, Christoph Hellwig wrote:
> We have many WR opcodes that are only supported in kernel space
> and/or require optional information to be copied into the WR
> structure.  Reject all those not explicitly handled so that we
> can't pass invalid information to drivers.
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
>  drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Oh yes, this is absolutely needed for -stable.

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

AFAIK, this path is rarely (never?) actually used. I think all the
drivers we have can post directly from userspace.

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

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 06:37:32PM +0200, Christoph Hellwig wrote:
> We have many WR opcodes that are only supported in kernel space
> and/or require optional information to be copied into the WR
> structure.  Reject all those not explicitly handled so that we
> can't pass invalid information to drivers.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>  drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Oh yes, this is absolutely needed for -stable.

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

AFAIK, this path is rarely (never?) actually used. I think all the
drivers we have can post directly from userspace.

Jason

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> AFAIK, this path is rarely (never?) actually used. I think all the
> drivers we have can post directly from userspace.

Oh, interesting.  Is there any chance to deprecate it?  Not having
to care for the uvers command would really help with some of the
upcoming changes I have in my mind.

Re: [PATCH 3/3] IB: remove xrc_remote_srq_num from struct ib_send_wr

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 06:37:34PM +0200, Christoph Hellwig wrote:
> The field is only initialized in mlx, but never used.
> 
> If we want to add proper XRC support it should be done with a new
> struct ib_xrc_wr.
> 
> This shrinks the various WR structures by another 4 bytes.

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

I've never really been entirely clear on how much of this stuff needs
to be kernel side. Even the switch doesn't really make alot of sense..

> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -2634,7 +2634,6 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  		switch (ibqp->qp_type) {
>  		case IB_QPT_XRC_INI:
>  			xrc = seg;
> -			xrc->xrc_srqn = htonl(wr->xrc_remote_srq_num);


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

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
> > Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > 
> > AFAIK, this path is rarely (never?) actually used. I think all the
> > drivers we have can post directly from userspace.
> 
> Oh, interesting.  Is there any chance to deprecate it?  Not having
> to care for the uvers command would really help with some of the
> upcoming changes I have in my mind.

Hmm, we'd need a survey of the userspace side to see if it is rarely
or never...

And we'd have to talk to the soft XXX guys to see if they plan to use
it..

I always like to see cruft go away, especially if it is broken cruft..

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

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
> > Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > AFAIK, this path is rarely (never?) actually used. I think all the
> > drivers we have can post directly from userspace.
> 
> Oh, interesting.  Is there any chance to deprecate it?  Not having
> to care for the uvers command would really help with some of the
> upcoming changes I have in my mind.

Hmm, we'd need a survey of the userspace side to see if it is rarely
or never...

And we'd have to talk to the soft XXX guys to see if they plan to use
it..

I always like to see cruft go away, especially if it is broken cruft..

Jason

RE: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Hefty, Sean]

> AFAIK, this path is rarely (never?) actually used. I think all the
> drivers we have can post directly from userspace.

I didn't think the ipath or qib drivers post from userspace.
--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Hefty, Sean]

> AFAIK, this path is rarely (never?) actually used. I think all the
> drivers we have can post directly from userspace.

I didn't think the ipath or qib drivers post from userspace.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Sagi Grimberg]

On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>
>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>> drivers we have can post directly from userspace.
>>
>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>> to care for the uvers command would really help with some of the
>> upcoming changes I have in my mind.
>
> Hmm, we'd need a survey of the userspace side to see if it is rarely
> or never...
>
> And we'd have to talk to the soft XXX guys to see if they plan to use
> it..

Checked in librxe (user-space softroce). Looks like posts are going via
this path...
--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Sagi Grimberg]

On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>
>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>> drivers we have can post directly from userspace.
>>
>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>> to care for the uvers command would really help with some of the
>> upcoming changes I have in my mind.
>
> Hmm, we'd need a survey of the userspace side to see if it is rarely
> or never...
>
> And we'd have to talk to the soft XXX guys to see if they plan to use
> it..

Checked in librxe (user-space softroce). Looks like posts are going via
this path...

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Sagi Grimberg]

On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
> We have many WR opcodes that are only supported in kernel space
> and/or require optional information to be copied into the WR
> structure.  Reject all those not explicitly handled so that we
> can't pass invalid information to drivers.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index a15318a..f9f3921 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>   		next->send_flags = user_wr->send_flags;
>
>   		if (is_ud) {
> +			if (next->opcode != IB_WR_SEND &&
> +			    next->opcode != IB_WR_SEND_WITH_IMM) {
> +				ret = -EINVAL;
> +				goto out_put;
> +			}
> +
>   			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
>   						     file->ucontext);
>   			if (!next->wr.ud.ah) {
> @@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>   				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
>   				break;
>   			default:
> -				break;
> +				ret = -EINVAL;
> +				goto out_put;
>   			}
>   		}
>
>

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Haggai, can you also have a look?

Re: [PATCH 3/3] IB: remove xrc_remote_srq_num from struct ib_send_wr

[Sagi Grimberg]

On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
> The field is only initialized in mlx, but never used.
>
> If we want to add proper XRC support it should be done with a new
> struct ib_xrc_wr.
>
> This shrinks the various WR structures by another 4 bytes.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
>   drivers/infiniband/hw/mlx5/qp.c | 1 -
>   include/rdma/ib_verbs.h         | 1 -
>   2 files changed, 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index 04df156..83a290f 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -2634,7 +2634,6 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>   		switch (ibqp->qp_type) {
>   		case IB_QPT_XRC_INI:
>   			xrc = seg;
> -			xrc->xrc_srqn = htonl(wr->xrc_remote_srq_num);
>   			seg += sizeof(*xrc);
>   			size += sizeof(*xrc) / 16;
>   			/* fall through */
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 9b29c78..b855189 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1100,7 +1100,6 @@ struct ib_send_wr {
>   		__be32		imm_data;
>   		u32		invalidate_rkey;
>   	} ex;
> -	u32			xrc_remote_srq_num;	/* XRC TGT QPs only */
>   };
>
>   struct ib_rdma_wr {
>

Looks OK to me,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This will need Eli's ack though...
--
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: shrink struct ib_send_wr

[Sagi Grimberg]

>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>     driver with an explicit embedding of struct ib_send_wr, similar
>     to what we do for all other MRs.

This is on the user-space memory registration path.

Haggai, can you grab it for a Tested-by tag?
--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Wed, Aug 19, 2015 at 07:50:23PM +0000, Hefty, Sean wrote:
> > AFAIK, this path is rarely (never?) actually used. I think all the
> > drivers we have can post directly from userspace.
> 
> I didn't think the ipath or qib drivers post from userspace.

Makes sense with their software IB stack.  Guess the idea to get rid
of this path is dead, would have been too nice..

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Steve Wise]

On 8/20/2015 3:49 AM, Sagi Grimberg wrote:
> On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
>> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>>> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>>
>>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>>> drivers we have can post directly from userspace.
>>>
>>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>>> to care for the uvers command would really help with some of the
>>> upcoming changes I have in my mind.
>>
>> Hmm, we'd need a survey of the userspace side to see if it is rarely
>> or never...
>>
>> And we'd have to talk to the soft XXX guys to see if they plan to use
>> it..
>
> Checked in librxe (user-space softroce). Looks like posts are going via
> this path...

Ditto for the soft iWARP stack, which is still out-of-linux.


--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Steve Wise]

On 8/20/2015 3:49 AM, Sagi Grimberg wrote:
> On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
>> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>>> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>>
>>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>>> drivers we have can post directly from userspace.
>>>
>>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>>> to care for the uvers command would really help with some of the
>>> upcoming changes I have in my mind.
>>
>> Hmm, we'd need a survey of the userspace side to see if it is rarely
>> or never...
>>
>> And we'd have to talk to the soft XXX guys to see if they plan to use
>> it..
>
> Checked in librxe (user-space softroce). Looks like posts are going via
> this path...

Ditto for the soft iWARP stack, which is still out-of-linux.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On Thursday, August 20, 2015 11:52 AM, linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> on behalf of Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
>> We have many WR opcodes that are only supported in kernel space
>> and/or require optional information to be copied into the WR
>> structure.  Reject all those not explicitly handled so that we
>> can't pass invalid information to drivers.
>>
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
>> ---
>>   drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>> index a15318a..f9f3921 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>               next->send_flags = user_wr->send_flags;
>>
>>               if (is_ud) {
>> +                     if (next->opcode != IB_WR_SEND &&
>> +                         next->opcode != IB_WR_SEND_WITH_IMM) {
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>> +                     }
>> +
>>                       next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
>>                                                    file->ucontext);
>>                       if (!next->wr.ud.ah) {
>> @@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>                               next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
>>                               break;
>>                       default:
>> -                             break;
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>>                       }
>>               }
>>
>>
> 
> Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Haggai, can you also have a look?

It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.

Haggai--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On Thursday, August 20, 2015 11:52 AM, linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
> On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
>> We have many WR opcodes that are only supported in kernel space
>> and/or require optional information to be copied into the WR
>> structure.  Reject all those not explicitly handled so that we
>> can't pass invalid information to drivers.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>> index a15318a..f9f3921 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>               next->send_flags = user_wr->send_flags;
>>
>>               if (is_ud) {
>> +                     if (next->opcode != IB_WR_SEND &&
>> +                         next->opcode != IB_WR_SEND_WITH_IMM) {
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>> +                     }
>> +
>>                       next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
>>                                                    file->ucontext);
>>                       if (!next->wr.ud.ah) {
>> @@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>                               next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
>>                               break;
>>                       default:
>> -                             break;
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>>                       }
>>               }
>>
>>
> 
> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
> 
> Haggai, can you also have a look?

It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.

Haggai

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Sat, Aug 22, 2015 at 06:38:47AM +0000, Haggai Eran wrote:
> It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.

Indeed.  It's handled fine in patch 2 which splits up the case, but
will be incorrectly rejected with just this patch applied.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On 22/08/2015 11:25, Christoph Hellwig wrote:
> On Sat, Aug 22, 2015 at 06:38:47AM +0000, Haggai Eran wrote:
>> It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.
> 
> Indeed.  It's handled fine in patch 2 which splits up the case, but
> will be incorrectly rejected with just this patch applied.

Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
avoid hurting bisectability.

Looking at the uverbs part in patch 2, I think the changes are okay. I
noticed there's a (__be32 __force) cast of the immediate data from
userspace (it was already in the existing code). I wonder, why not
define the field in the uapi struct as __be32 in the first place?

Haggai

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
> avoid hurting bisectability.

I've done this already, just waiting for more feedback before resending:

http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

> Looking at the uverbs part in patch 2, I think the changes are okay. I
> noticed there's a (__be32 __force) cast of the immediate data from
> userspace (it was already in the existing code). I wonder, why not
> define the field in the uapi struct as __be32 in the first place?

It looks odd to me as well, but it's not really something I want to
change in this series.  Note that sparse annoted types like __be32
aren't really common in userspace, but with a bit of effort they can
be supported.  We have them and regularly run sparse for xfsprogs for
example.
--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
> avoid hurting bisectability.

I've done this already, just waiting for more feedback before resending:

http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

> Looking at the uverbs part in patch 2, I think the changes are okay. I
> noticed there's a (__be32 __force) cast of the immediate data from
> userspace (it was already in the existing code). I wonder, why not
> define the field in the uapi struct as __be32 in the first place?

It looks odd to me as well, but it's not really something I want to
change in this series.  Note that sparse annoted types like __be32
aren't really common in userspace, but with a bit of effort they can
be supported.  We have them and regularly run sparse for xfsprogs for
example.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On 24/08/2015 09:55, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
>> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
>> avoid hurting bisectability.
> 
> I've done this already, just waiting for more feedback before resending:
> 
> http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

Great.

>> Looking at the uverbs part in patch 2, I think the changes are okay. I
>> noticed there's a (__be32 __force) cast of the immediate data from
>> userspace (it was already in the existing code). I wonder, why not
>> define the field in the uapi struct as __be32 in the first place?
> 
> It looks odd to me as well, but it's not really something I want to
> change in this series.  Note that sparse annoted types like __be32
> aren't really common in userspace, but with a bit of effort they can
> be supported.  We have them and regularly run sparse for xfsprogs for
> example.

I have to try it with libibverbs sometime. It doesn't use uapi yet
though IIRC - it has its own version of the header files.

--
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 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On 24/08/2015 09:55, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
>> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
>> avoid hurting bisectability.
> 
> I've done this already, just waiting for more feedback before resending:
> 
> http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

Great.

>> Looking at the uverbs part in patch 2, I think the changes are okay. I
>> noticed there's a (__be32 __force) cast of the immediate data from
>> userspace (it was already in the existing code). I wonder, why not
>> define the field in the uapi struct as __be32 in the first place?
> 
> It looks odd to me as well, but it's not really something I want to
> change in this series.  Note that sparse annoted types like __be32
> aren't really common in userspace, but with a bit of effort they can
> be supported.  We have them and regularly run sparse for xfsprogs for
> example.

I have to try it with libibverbs sometime. It doesn't use uapi yet
though IIRC - it has its own version of the header files.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Mon, Aug 24, 2015 at 10:59:21AM +0300, Haggai Eran wrote:
> > It looks odd to me as well, but it's not really something I want to
> > change in this series.  Note that sparse annoted types like __be32
> > aren't really common in userspace, but with a bit of effort they can
> > be supported.  We have them and regularly run sparse for xfsprogs for
> > example.
> 
> I have to try it with libibverbs sometime. It doesn't use uapi yet
> though IIRC - it has its own version of the header files.

Yes, I noticed that.  And the WR opcodes aren't even exported in the
uapi header, and use a shared namespace with the in-kernel only ones.

It's all a giant mess unfortunately.

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..be4cb9f 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2411,9 +2417,11 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 					user_wr->wr.atomic.compare_add;
 				next->wr.atomic.swap = user_wr->wr.atomic.swap;
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
+			case IB_WR_SEND:
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.1