Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
On 22/08/2015 11:25, Christoph Hellwig wrote: On Sat, Aug 22, 2015 at 06:38:47AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
On Sat, Aug 22, 2015 at 06:38:47AM +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
On Thursday, August 20, 2015 11:52 AM, linux-rdma-ow...@vger.kernel.org
linux-rdma-ow...@vger.kernel.org on behalf of Sagi Grimberg
sa...@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: sta...@vger.kernel.org
Signed-off-by: Christoph Hellwig h...@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 sa...@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--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
On Wed, Aug 19, 2015 at 07:50:23PM +, 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.. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 jguntho...@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... -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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: sta...@vger.kernel.org
Signed-off-by: Christoph Hellwig h...@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 sa...@mellanox.com
Haggai, can you also have a look?
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 jguntho...@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. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote: Reviewed-by: Jason Gunthorpe jguntho...@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. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 jguntho...@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 -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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: sta...@vger.kernel.org Signed-off-by: Christoph Hellwig h...@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 jguntho...@obsidianresearch.com 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
