[PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[Jason Gunthorpe]

The set of entry->driver_removed is missing locking, protect it with
xa_lock() which is held by the only reader.

Otherwise readers may continue to see driver_removed = false after
rdma_user_mmap_entry_remove() returns and may continue to try and
establish new mmaps.

Fixes: 3411f9f01b76 ("RDMA/core: Create mmap database and cookie helper functions")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/infiniband/core/ib_core_uverbs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/core/ib_core_uverbs.c b/drivers/infiniband/core/ib_core_uverbs.c
index b7cb59844ece45..b51bd7087a881f 100644
--- a/drivers/infiniband/core/ib_core_uverbs.c
+++ b/drivers/infiniband/core/ib_core_uverbs.c
@@ -232,7 +232,9 @@ void rdma_user_mmap_entry_remove(struct rdma_user_mmap_entry *entry)
 	if (!entry)
 		return;
 
+	xa_lock(&entry->ucontext->mmap_xa);
 	entry->driver_removed = true;
+	xa_unlock(&entry->ucontext->mmap_xa);
 	kref_put(&entry->ref, rdma_user_mmap_entry_free);
 }
 EXPORT_SYMBOL(rdma_user_mmap_entry_remove);
-- 
2.24.1

Re: [PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[Gal Pressman]

On 15/01/2020 22:20, Jason Gunthorpe wrote:
> The set of entry->driver_removed is missing locking, protect it with
> xa_lock() which is held by the only reader.
> 
> Otherwise readers may continue to see driver_removed = false after
> rdma_user_mmap_entry_remove() returns and may continue to try and
> establish new mmaps.

That's kind of an inherent race regardless, isn't it?

LGTM,
Reviewed-by: Gal Pressman <galpress@amazon.com>

Re: [PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[Jason Gunthorpe]

On Thu, Jan 16, 2020 at 10:51:52AM +0200, Gal Pressman wrote:
> On 15/01/2020 22:20, Jason Gunthorpe wrote:
> > The set of entry->driver_removed is missing locking, protect it with
> > xa_lock() which is held by the only reader.
> > 
> > Otherwise readers may continue to see driver_removed = false after
> > rdma_user_mmap_entry_remove() returns and may continue to try and
> > establish new mmaps.
> 
> That's kind of an inherent race regardless, isn't it?

How do you mean? Once rdma_user_mmap_entry_remove() returns no new
mmaps can be established, existing mmaps remain. The driver can rely
on this, by, for instance, calling free_page

Jason

RE: [PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[Michal Kalderon]

> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Jason Gunthorpe
> 
> The set of entry->driver_removed is missing locking, protect it with
> xa_lock() which is held by the only reader.
> 
> Otherwise readers may continue to see driver_removed = false after
> rdma_user_mmap_entry_remove() returns and may continue to try and
> establish new mmaps.
> 
> Fixes: 3411f9f01b76 ("RDMA/core: Create mmap database and cookie helper
> functions")
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  drivers/infiniband/core/ib_core_uverbs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/core/ib_core_uverbs.c
> b/drivers/infiniband/core/ib_core_uverbs.c
> index b7cb59844ece45..b51bd7087a881f 100644
> --- a/drivers/infiniband/core/ib_core_uverbs.c
> +++ b/drivers/infiniband/core/ib_core_uverbs.c
> @@ -232,7 +232,9 @@ void rdma_user_mmap_entry_remove(struct
> rdma_user_mmap_entry *entry)
>  	if (!entry)
>  		return;
> 
> +	xa_lock(&entry->ucontext->mmap_xa);
>  	entry->driver_removed = true;
> +	xa_unlock(&entry->ucontext->mmap_xa);
>  	kref_put(&entry->ref, rdma_user_mmap_entry_free);  }
> EXPORT_SYMBOL(rdma_user_mmap_entry_remove);
> --

Thanks, 

Acked-by: Michal Kalderon <michal.kalderon@marvell.com>


> 2.24.1

Re: [PATCH] RDMA/core: Ensure that rdma_user_mmap_entry_remove() is a fence

[Jason]

On Wed, Jan 15, 2020 at 08:20:44PM +0000, Jason Gunthorpe wrote:
> The set of entry->driver_removed is missing locking, protect it with
> xa_lock() which is held by the only reader.
> 
> Otherwise readers may continue to see driver_removed = false after
> rdma_user_mmap_entry_remove() returns and may continue to try and
> establish new mmaps.
> 
> Fixes: 3411f9f01b76 ("RDMA/core: Create mmap database and cookie helper functions")
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Gal Pressman <galpress@amazon.com>
> Acked-by: Michal Kalderon <michal.kalderon@marvell.com>
> ---
>  drivers/infiniband/core/ib_core_uverbs.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied to for-next

Jason