Re: [PATCH v2 for-next 1/3] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Aug 12, 2024 at 08:56:38PM +0800, Junxian Huang wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Provide a new api rdma_user_mmap_disassociate() for drivers to
> disassociate mmap pages for ucontext.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> ---
>  drivers/infiniband/core/uverbs_main.c | 21 +++++++++++++++++++++
>  include/rdma/ib_verbs.h               |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index bc099287de9a..00dab5bfb78c 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -880,6 +880,27 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
>  	}
>  }
>  
> +/**
> + * rdma_user_mmap_disassociate() - disassociate the mmap from the ucontext.
> + *
> + * @ucontext: associated user context.
> + *
> + * This function should be called by drivers that need to disable mmap for
> + * some ucontexts.
> + */
> +void rdma_user_mmap_disassociate(struct ib_ucontext *ucontext)
> +{
> +	struct ib_uverbs_file *ufile = ucontext->ufile;
> +
> +	/* Racing with uverbs_destroy_ufile_hw */
> +	if (!down_read_trylock(&ufile->hw_destroy_rwsem))
> +		return;

This is not quite right here, in the other cases lock failure is
aborting an operation that is about to start, in this case we are must
ensure the zap completed otherwise we break the contract of no mmaps
upon return.

So at least this needs to be a naked down_read()

But..

That lock lockdep assertion in uverbs_user_mmap_disassociate() I think
was supposed to say the write side is held, which we can't actually
get here.

This is because the nasty algorithm works by pulling things off the
list, if we don't have a lock then one thread could be working on an
item while another thread is unaware which will also break the
contract.

You may need to add a dedicated mutex inside
uverbs_user_mmap_disassociate() and not try to re-use
hw_destroy_rwsem.

Jason