Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Bob Pearson <rpearsonhpe@gmail.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: xyzxyz2000@gmail.com, linux-rdma@vger.kernel.org,
	Bob Pearson <rpearson@hpe.com>
Subject: Re: [PATCH for-next 1/9] RDMA/rxe: Add bind MW fields to rxe_send_wr
Date: Tue, 13 Apr 2021 21:50:28 -0500
Message-ID: <eba02326-013f-1707-0db7-209413d2cd6f@gmail.com> (raw)
In-Reply-To: <20210413231136.GA1385646@nvidia.com>

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 21:40 [PATCH for-next 0/9] RDMA/rxe: Implement memory windows Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 1/9] RDMA/rxe: Add bind MW fields to rxe_send_wr Bob Pearson
2021-04-13 23:11   ` Jason Gunthorpe
2021-04-14  2:50     ` Bob Pearson [this message]
2021-04-08 21:40 ` [PATCH for-next 2/9] RDMA/rxe: Return errors for add index and key Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 3/9] RDMA/rxe: Enable MW object pool Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 4/9] RDMA/rxe: Add ib_alloc_mw and ib_dealloc_mw verbs Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 5/9] RDMA/rxe: Replace WR_REG_MASK by WR_LOCAL_OP_MASK Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 6/9] RDMA/rxe: Move local ops to subroutine Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 7/9] RDMA/rxe: Add support for bind MW work requests Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 8/9] RDMA/rxe: Implement invalidate MW operations Bob Pearson
2021-04-08 21:40 ` [PATCH for-next 9/9] RDMA/rxe: Implement memory access through MWs Bob Pearson
2021-04-13 23:22 ` [PATCH for-next 0/9] RDMA/rxe: Implement memory windows Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eba02326-013f-1707-0db7-209413d2cd6f@gmail.com \
    --to=rpearsonhpe@gmail.com \
    --cc=jgg@nvidia.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=rpearson@hpe.com \
    --cc=xyzxyz2000@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git