| Message ID | 1440002254-795-2-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } }
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(-)