lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240823152501.GB2342875@nvidia.com>
Date: Fri, 23 Aug 2024 12:25:01 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Junxian Huang <huangjunxian6@...ilicon.com>
Cc: leon@...nel.org, linux-rdma@...r.kernel.org, linuxarm@...wei.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 for-next 1/3] RDMA/core: Provide
 rdma_user_mmap_disassociate() to disassociate mmap pages

On Mon, Aug 12, 2024 at 08:56:38PM +0800, Junxian Huang wrote:
> From: Chengchang Tang <tangchengchang@...wei.com>
> 
> Provide a new api rdma_user_mmap_disassociate() for drivers to
> disassociate mmap pages for ucontext.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@...wei.com>
> Signed-off-by: Junxian Huang <huangjunxian6@...ilicon.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ