Re: [PATCH for-next v2 2/2] RDMA/erdma: Support non-4K page size in doorbell allocation

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Mar 15, 2023 at 12:22:10PM +0200, Leon Romanovsky wrote:
> On Wed, Mar 15, 2023 at 09:58:06AM +0800, Cheng Xu wrote:
> > 
> > 
> > On 3/14/23 10:10 PM, Leon Romanovsky wrote:
> > > On Tue, Mar 14, 2023 at 07:50:19PM +0800, Cheng Xu wrote:
> > >>
> > >>
> > <...>
> > >>
> > >> Our doorbell space is aligned to 4096, this works fine when PAGE_SIZE is
> > >> also 4096, and the doorbell space starts from the mapped page. When
> > >> PAGE_SIZE is not 4096, the doorbell space may starts from the middle of
> > >> the mapped page.
> > >>
> > >> For example, our SQ doorbell starts from the offset 4096 in PCIe bar 0.
> > >> When we map the first SQ doorbell to userspace when PAGE_SIZE is 64K,
> > >> the doorbell space starts from the offset 4096 in mmap returned address.
> > >>
> > >> So the userspace needs to know the doorbell space offset in mmaped page.
> > > 
> > > And can't you preserve same alignment in the kernel for doorbells for every page size?
> > > Just always start from 0.
> > > 
> > 
> > I've considered this option before, but unfortunately can't, at least for CQ DB.
> > The size of our PCIe bar 0 is 512K, and offset [484K, 508K] are CQ doorbells.
> > CQ doorbell space is located in offset [36K, 60K] when PAGE_SIZE = 64K, and can't
> > start from offset 0 in this case.
> > 
> > Another reason is that we want to organize SQ doorbell space in unit of 4096.
> > In current implementation, each ucontext will be assigned a SQ doorbell space
> > for both normal doorbell and direct wqe usage. Unit of 4096, compared with
> > larger unit, more ucontexts can be assigned exclusive doorbell space for direct
> > wqe.
> 
> I have a feeling that there is an existing API for it already.
> Let's give a chance for Jason to chime in.

This sounds similar to how mlx5 chops up its doorbell space

But I don't understand your device security model.

In mlx5 it is not allowed to share doorbells between unrelated
processes. Doorbells have built in security and a doorbell can only
trigger QP/CQ's that are explicitly linked to that doorbell.

Thus mlx5 is careful to only mmap doorbells that are linked to the
QPs. On 64K page size userspace receives alot of doorbells per mmap,
all linked to the same security context.

Improperly sharing MMIO pages can result in various security problems
if a hostile userspace can write arbitary things to MMIO space.

Here you seem to be talking about overmapping stuff. What is the
security argument that it is safe to leak to userspace parts of the
device MMIO BAR beyond that strictly cotnained to the single doorbell?

This has to be clearly explained in the commit message and a comment.

Jason