On Fri, Feb 01, 2019 at 08:21:35PM +0000, Arumugam, Kamenee wrote:
>
> > > I think you should change the definition to be:
> >
> > > #define RDMA_ATOMIC_UAPI(_type, _name) struct {_type atomic_val;}
> > > _name
> >
> > > And then add a kernel function
> >
> > > #define RDMA_READ_UAPI_ATOMIC(_member)
> > > READ_ONCE((_memb)->atomic_val)
> >
> > > So that the compiler protects against this mistake.
> >
> > There is already a producer and consumer lock for the circular buffer
> > and only reading the opposition index needs the READ_ONCE control.
>
> > No.
> > Any read of memory mmap'd into user space must use READ_ONCE or risk a security problem due to control-flow execution corruption.
>
> As per your suggestion above, I have draft a quick prototype to
> understand and clear any assumption beforehand. Below are 3 macros
> that will be introduce in the rvt-abi header file. The
> RDMA_READ_UAPI_ATOMIC and RDMA_WRITE_UAPI_ATOMIC macro are only
> applied to struct mmaped to userspace.
>
> #define RDMA_ATOMIC_UAPI(_type, _name)struct{ _type val;}_name
> #define RDMA_READ_UAPI_ATOMIC(member) READ_ONCE((member).val)
> #define RDMA_WRITE_UAPI_ATOMIC(member, x) WRITE_ONCE((member).val, x)
It seems like siw (and rxe) needs this too lets see if Bernard likes it, and if
so it should probably go in a commonish header.
> struct rvt_cq_wc {
> /* index of next entry to fill */
> RDMA_ATOMIC_UAPI(__u32, head);
> /* index of next ib_poll_cq() entry */
> RDMA_ATOMIC_UAPI(__u32, tail);
>
> /* these are actually size ibcq.cqe + 1 */
> struct ib_uverbs_wc uqueue[];
> };
>
> Here is the use case of RDMA_READ_UAPI_ATOMIC macro in cq.c file.
> The READ_ONCE will be applied for any read on the indices of mmaped
> to user space.
> if (cq->ip) {
> u_wc = cq->queue;
> uqueue = &u_wc->uqueue[0];
> head = RDMA_READ_UAPI_ATOMIC(u_wc->head);
> tail = RDMA_READ_UAPI_ATOMIC(u_wc->tail);
> } else {
> k_wc = cq->kqueue;
> kqueue = &k_wc->kqueue[0];
> head = k_wc->head;
> tail = k_wc->tail;
> }
>
>
> >Same story for WRITE_ONCE.
>
> Here is the usage for RDMA_WRITE_UAPI_ATOMIC macro. For cq enter, we
> kept smp_wmb() in the code as it is now and we didn't adopt it into
> this macro. This macro Is meant for compile barrier.
That should be using smp_store_release()/load_acquire(), not open
coding barriers.
When working with unlocked data like this it is important to be
careful with these things. READ/WRITE_ONCE are only correct if the
single value is required, if you are doing anything that has a
dependency on other values then you have to use acquire/release
semantics to ensure global ordering is achieved.
These mirror to the userspace stdatomic definitions, so userspace must
also use the proper pairs - ie a smp_loac_acquire() in kernel must be
paired with an atomic_store_explicit(memory_order_release) - and vice
versa
Similar for the READ/WRITE_ONCE but it should use
memory_order_relaxed.
Bernard, same expectation for siw.
Jason
