| Message ID | 20210408214040.2956-2-rpearson@hpe.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Implement memory windows | expand |
On Thu, Apr 08, 2021 at 04:40:33PM -0500, Bob Pearson wrote: > Add fields to struct rxe_send_wr in rdma_user_rxe.h to > support bind MW work requests from user space and kernel. > > Signed-off-by: Bob Pearson <rpearson@hpe.com> > include/uapi/rdma/rdma_user_rxe.h | 34 ++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/rdma/rdma_user_rxe.h b/include/uapi/rdma/rdma_user_rxe.h > index 068433e2229d..b9c80ca73473 100644 > +++ b/include/uapi/rdma/rdma_user_rxe.h > @@ -99,7 +99,39 @@ struct rxe_send_wr { > __u32 remote_qkey; > __u16 pkey_index; > } ud; > - /* reg is only used by the kernel and is not part of the uapi */ > + struct { > + __aligned_u64 addr; > + __aligned_u64 length; > + union { > + __u32 mr_lkey; > + __aligned_u64 reserved1; > + }; > + union { > + __u32 mw_rkey; > + __aligned_u64 reserved2; > + }; > + __u32 rkey; > + __u32 access; > + __u32 flags; > + } umw; > + /* The following are only used by the kernel > + * and are not part of the uapi > + */ > + struct { > + __aligned_u64 addr; > + __aligned_u64 length; > + union { > + struct ib_mr *mr; A kernel struct should not appear in a uapi header, nor should secure kernel data ever be stored in user memory. I'm completely lost at how all this thinks it is working, we don't even have kernel verbs support for memory windows?? Jason
On 4/13/21 6:11 PM, Jason Gunthorpe wrote: > On Thu, Apr 08, 2021 at 04:40:33PM -0500, Bob Pearson wrote: >> Add fields to struct rxe_send_wr in rdma_user_rxe.h to >> support bind MW work requests from user space and kernel. >> >> Signed-off-by: Bob Pearson <rpearson@hpe.com> >> include/uapi/rdma/rdma_user_rxe.h | 34 ++++++++++++++++++++++++++++++- >> 1 file changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/include/uapi/rdma/rdma_user_rxe.h b/include/uapi/rdma/rdma_user_rxe.h >> index 068433e2229d..b9c80ca73473 100644 >> +++ b/include/uapi/rdma/rdma_user_rxe.h >> @@ -99,7 +99,39 @@ struct rxe_send_wr { >> __u32 remote_qkey; >> __u16 pkey_index; >> } ud; >> - /* reg is only used by the kernel and is not part of the uapi */ >> + struct { >> + __aligned_u64 addr; >> + __aligned_u64 length; >> + union { >> + __u32 mr_lkey; >> + __aligned_u64 reserved1; >> + }; >> + union { >> + __u32 mw_rkey; >> + __aligned_u64 reserved2; >> + }; >> + __u32 rkey; >> + __u32 access; >> + __u32 flags; >> + } umw; >> + /* The following are only used by the kernel >> + * and are not part of the uapi >> + */ >> + struct { >> + __aligned_u64 addr; >> + __aligned_u64 length; >> + union { >> + struct ib_mr *mr; > > A kernel struct should not appear in a uapi header, nor should secure > kernel data ever be stored in user memory. > > I'm completely lost at how all this thinks it is working, we don't > even have kernel verbs support for memory windows?? > > Jason > Hard to see from context but I was copying the existing header file which is used to define the WQE layout for rxe for *both* user and kernel space. The structs shown in the hunk are contained in a union. The existing version of the driver has one struct union { ... struct { ... } reg; } wr; which is only used in kernel space. Like the old song about the 8x10 glossy pictures, I figured I'd just add another one and avoid looking up the MWs and MRs for bind in the kernel. This is before I discovered that you all deleted MW support from the kernel a while back. Not really sure why. Not used I suppose but it is in the spec. Clearly only user data for user WRs appear in this for user QPs and only kernel data for kernel QPs. There isn't any mixing. Since it is a union there is no overhead by including the kernel WQEs. There is no secret about what the kernel structs are. How to fix this? Since rxe was written the new variable size WQEs in ib_verbs.h (ib_send_wr and friends) were added and they could be a better choice. Rxe could take these as its own native wqe and that would reduce the amount of work on kernel WRs and reduce the amount of code dealing with them from user space. It would break ABI unfortunately. The other approach is to, as you suggest, ifdef out the kernel stuff in user space. It works anyway because you can always add a pointer to an undefined struct as long as you don't try to use it. Also as you point out ib_bind_mw doesn't exist anymore so I could get rid of the kmw struct and just use the rkeys. Could still support a kernel mw implementation later without a change if one showed up. There are some interesting articles about the security of RDMA wire protocols that strongly suggest using type2 MWs with randomly chosen rkeys and qp numbers instead of MRs. It's not perfect either but orders of magnitude stronger protection. bob
diff --git a/include/uapi/rdma/rdma_user_rxe.h b/include/uapi/rdma/rdma_user_rxe.h index 068433e2229d..b9c80ca73473 100644 --- a/include/uapi/rdma/rdma_user_rxe.h +++ b/include/uapi/rdma/rdma_user_rxe.h @@ -99,7 +99,39 @@ struct rxe_send_wr { __u32 remote_qkey; __u16 pkey_index; } ud; - /* reg is only used by the kernel and is not part of the uapi */ + struct { + __aligned_u64 addr; + __aligned_u64 length; + union { + __u32 mr_lkey; + __aligned_u64 reserved1; + }; + union { + __u32 mw_rkey; + __aligned_u64 reserved2; + }; + __u32 rkey; + __u32 access; + __u32 flags; + } umw; + /* The following are only used by the kernel + * and are not part of the uapi + */ + struct { + __aligned_u64 addr; + __aligned_u64 length; + union { + struct ib_mr *mr; + __aligned_u64 reserved1; + }; + union { + struct ib_mw *mw; + __aligned_u64 reserved2; + }; + __u32 rkey; + __u32 access; + __u32 flags; + } kmw; struct { union { struct ib_mr *mr;
Add fields to struct rxe_send_wr in rdma_user_rxe.h to support bind MW work requests from user space and kernel. Signed-off-by: Bob Pearson <rpearson@hpe.com> --- include/uapi/rdma/rdma_user_rxe.h | 34 ++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)