[PATCH 1/2] cxl: Fix + cleanup error paths in cxl_dev_context_init

[PATCH 1/2] cxl: Fix + cleanup error paths in cxl_dev_context_init

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

If the cxl_context_alloc() call fails, we return immediately without
releasing the reference on the AFU device, allowing it to leak.

This patch switches to using goto style error handling so that the
device is released in common code for both error paths, and will also
simplify things if we add additional initialisation in this function in
the future.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/api.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index f49e3e5..005adc7 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -25,19 +25,24 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
 
 	get_device(&afu->dev);
 	ctx = cxl_context_alloc();
-	if (IS_ERR(ctx))
-		return ctx;
+	if (IS_ERR(ctx)) {
+		rc = PTR_ERR(ctx);
+		goto err_dev;
+	}
 
 	/* Make it a slave context.  We can promote it later? */
 	rc = cxl_context_init(ctx, afu, false, NULL);
-	if (rc) {
-		kfree(ctx);
-		put_device(&afu->dev);
-		return ERR_PTR(-ENOMEM);
-	}
+	if (rc)
+		goto err_ctx;
 	cxl_assign_psn_space(ctx);
 
 	return ctx;
+
+err_ctx:
+	kfree(ctx);
+err_dev:
+	put_device(&afu->dev);
+	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(cxl_dev_context_init);
 
-- 
2.1.4

[PATCH 2/2] cxl: Fix force unmapping mmaps of contexts allocated through the kernel api

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

The cxl user api uses the address_space associated with the file when we
need to force unmap all cxl mmap regions (e.g. on eeh, driver detach,
etc). Currently, contexts allocated through the kernel api do not do
this and instead skip the mmap invalidation, potentially allowing them
to poke at the hardware after such an event, which may cause all sorts
of trouble.

This patch allocates an address_space for cxl contexts allocated through
the kernel api so that the same invalidate path will for these contexts
as well. We don't use the anonymous inode's address_space, as doing so
could invalidate any mmaps of completely unrelated drivers using
anonymous file descriptors.

This patch also introduces a kernelapi flag, so we know when freeing the
context if the address_space was allocated by us and needs to be freed.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/api.c     | 33 ++++++++++++++++++++++++++++++---
 drivers/misc/cxl/context.c |  2 ++
 drivers/misc/cxl/cxl.h     |  1 +
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 005adc7..8af12c8 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -12,11 +12,13 @@
 #include <linux/anon_inodes.h>
 #include <linux/file.h>
 #include <misc/cxl.h>
+#include <linux/fs.h>
 
 #include "cxl.h"
 
 struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
 {
+	struct address_space *mapping;
 	struct cxl_afu *afu;
 	struct cxl_context  *ctx;
 	int rc;
@@ -30,14 +32,32 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
 		goto err_dev;
 	}
 
+	ctx->kernelapi = true;
+
+	/*
+	 * Make our own address space since we won't have one from the
+	 * filesystem like the user api has, and even if we do associate a file
+	 * with this context we don't want to use the global anonymous inode's
+	 * address space as that can invalidate unrelated users:
+	 */
+	mapping = kmalloc(sizeof(struct address_space), GFP_KERNEL);
+	if (!mapping) {
+		rc = -ENOMEM;
+		goto err_ctx;
+	}
+	address_space_init_once(mapping);
+
 	/* Make it a slave context.  We can promote it later? */
-	rc = cxl_context_init(ctx, afu, false, NULL);
+	rc = cxl_context_init(ctx, afu, false, mapping);
 	if (rc)
-		goto err_ctx;
+		goto err_mapping;
+
 	cxl_assign_psn_space(ctx);
 
 	return ctx;
 
+err_mapping:
+	kfree(mapping);
 err_ctx:
 	kfree(ctx);
 err_dev:
@@ -260,9 +280,16 @@ struct file *cxl_get_fd(struct cxl_context *ctx, struct file_operations *fops,
 
 	file = anon_inode_getfile("cxl", fops, ctx, flags);
 	if (IS_ERR(file))
-		put_unused_fd(fdtmp);
+		goto err_fd;
+
+	file->f_mapping = ctx->mapping;
+
 	*fd = fdtmp;
 	return file;
+
+err_fd:
+	put_unused_fd(fdtmp);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(cxl_get_fd);
 
diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
index 941fda0..e762f85 100644
--- a/drivers/misc/cxl/context.c
+++ b/drivers/misc/cxl/context.c
@@ -272,6 +272,8 @@ static void reclaim_ctx(struct rcu_head *rcu)
 	if (ctx->ff_page)
 		__free_page(ctx->ff_page);
 	ctx->sstp = NULL;
+	if (ctx->kernelapi)
+		kfree(ctx->mapping);
 
 	kfree(ctx);
 }
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index e7af256..d6566c6 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -420,6 +420,7 @@ struct cxl_context {
 	struct mutex mapping_lock;
 	struct page *ff_page;
 	bool mmio_err_ff;
+	bool kernelapi;
 
 	spinlock_t sste_lock; /* Protects segment table entries */
 	struct cxl_sste *sstp;
-- 
2.1.4

Re: [2/2] cxl: Fix force unmapping mmaps of contexts allocated through the kernel api

[Michael Ellerman]

On Thu, 2015-27-08 at 09:50:19 UTC, Ian Munsie wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> The cxl user api uses the address_space associated with the file when we
> need to force unmap all cxl mmap regions (e.g. on eeh, driver detach,
> etc). Currently, contexts allocated through the kernel api do not do
> this and instead skip the mmap invalidation, potentially allowing them
> to poke at the hardware after such an event, which may cause all sorts
> of trouble.
> 
> This patch allocates an address_space for cxl contexts allocated through
> the kernel api so that the same invalidate path will for these contexts
> as well. We don't use the anonymous inode's address_space, as doing so
> could invalidate any mmaps of completely unrelated drivers using
> anonymous file descriptors.
> 
> This patch also introduces a kernelapi flag, so we know when freeing the
> context if the address_space was allocated by us and needs to be freed.
> 
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/55e07668fbba9466e6a9ef76

cheers

Re: [1/2] cxl: Fix + cleanup error paths in cxl_dev_context_init

[Michael Ellerman]

On Thu, 2015-27-08 at 09:50:18 UTC, Ian Munsie wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> If the cxl_context_alloc() call fails, we return immediately without
> releasing the reference on the AFU device, allowing it to leak.
> 
> This patch switches to using goto style error handling so that the
> device is released in common code for both error paths, and will also
> simplify things if we add additional initialisation in this function in
> the future.
> 
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/af2a50bb0ce1ca7a9c478481

cheers