[Cbe-oss-dev] [PATCH] spufs: avoid accessing to released inode

Christoph Hellwig hch at lst.de
Fri Mar 9 22:44:49 EST 2007


On Tue, Mar 06, 2007 at 12:44:21PM +0900, Masato Noguchi wrote:
> 
> This patch modifies the kernel not to access to an address_space
> of already released inode. Originally, at closing spe context
> directory, the kernel called unbind_context() and spu_unmap_mappings(),
> and accessed ctx->local_store (and so on.) after ctx's files purned.
> 
> FYI, This bug can reproduce reliably by running libspe2 application
> on the kernel with CONFIG_DEBUG_SLAB and CONFIG_DEBUG_SPINLOCK.

Thanks, I can reproduce this bug.  I don't think your patch is the
right fix.  The problem we're hitting is that the local_store, cntl,
etc.. pointers in struct spu_context are still set after the inode
has gone away, and I suspect the right fix is to refcount openers
of these files and clear the pointers when no one has them open, so
that we don't try to invalidate them anymore.

This is the patch I've come up with which works for me:


Index: linux-2.6.20/arch/powerpc/platforms/cell/spufs/file.c
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/cell/spufs/file.c	2007-03-08 21:57:27.000000000 +0100
+++ linux-2.6.20/arch/powerpc/platforms/cell/spufs/file.c	2007-03-09 12:41:20.000000000 +0100
@@ -44,9 +44,26 @@
 {
 	struct spufs_inode_info *i = SPUFS_I(inode);
 	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->local_store = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->local_store = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
+static int
+spufs_mem_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->local_store = NULL;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return 0;
 }
@@ -203,6 +220,7 @@
 
 static struct file_operations spufs_mem_fops = {
 	.open			= spufs_mem_open,
+	.release		= spufs_mem_release,
 	.read			= spufs_mem_read,
 	.write			= spufs_mem_write,
 	.llseek			= generic_file_llseek,
@@ -295,17 +313,35 @@
 	struct spufs_inode_info *i = SPUFS_I(inode);
 	struct spu_context *ctx = i->i_ctx;
 
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->cntl = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->cntl = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return simple_attr_open(inode, file, spufs_cntl_get,
 					spufs_cntl_set, "0x%08lx");
 }
 
+static int
+spufs_cntl_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	simple_attr_close(inode, file);
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->cntl = NULL;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
 static struct file_operations spufs_cntl_fops = {
 	.open = spufs_cntl_open,
-	.release = simple_attr_close,
+	.release = spufs_cntl_release,
 	.read = simple_attr_read,
 	.write = simple_attr_write,
 	.mmap = spufs_cntl_mmap,
@@ -781,13 +817,29 @@
 {
 	struct spufs_inode_info *i = SPUFS_I(inode);
 	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->signal1 = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->signal1 = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return nonseekable_open(inode, file);
 }
 
+static int
+spufs_signal1_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->signal1 = NULL;
+	smp_wmb();
+	return 0;
+}
+
 static ssize_t __spufs_signal1_read(struct spu_context *ctx, char __user *buf,
 			size_t len, loff_t *pos)
 {
@@ -880,6 +932,7 @@
 
 static struct file_operations spufs_signal1_fops = {
 	.open = spufs_signal1_open,
+	.release = spufs_signal1_release,
 	.read = spufs_signal1_read,
 	.write = spufs_signal1_write,
 	.mmap = spufs_signal1_mmap,
@@ -889,13 +942,30 @@
 {
 	struct spufs_inode_info *i = SPUFS_I(inode);
 	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->signal2 = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->signal2 = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return nonseekable_open(inode, file);
 }
 
+static int
+spufs_signal2_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->signal2 = NULL;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
 static ssize_t __spufs_signal2_read(struct spu_context *ctx, char __user *buf,
 			size_t len, loff_t *pos)
 {
@@ -992,6 +1062,7 @@
 
 static struct file_operations spufs_signal2_fops = {
 	.open = spufs_signal2_open,
+	.release = spufs_signal2_release,
 	.read = spufs_signal2_read,
 	.write = spufs_signal2_write,
 	.mmap = spufs_signal2_mmap,
@@ -1091,14 +1162,32 @@
 	struct spu_context *ctx = i->i_ctx;
 
 	file->private_data = i->i_ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->mss = inode->i_mapping;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!i->i_openers++)
+		ctx->mss = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return nonseekable_open(inode, file);
 }
 
+static int
+spufs_mss_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->mss = NULL;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
 static struct file_operations spufs_mss_fops = {
 	.open	 = spufs_mss_open,
+	.release = spufs_mss_release,
 	.mmap	 = spufs_mss_mmap,
 };
 
@@ -1133,15 +1222,32 @@
 	struct spufs_inode_info *i = SPUFS_I(inode);
 	struct spu_context *ctx = i->i_ctx;
 
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = i->i_ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->psmap = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->psmap = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return nonseekable_open(inode, file);
 }
 
+static int
+spufs_psmap_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->psmap = NULL;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
 static struct file_operations spufs_psmap_fops = {
 	.open	 = spufs_psmap_open,
+	.release = spufs_psmap_release,
 	.mmap	 = spufs_psmap_mmap,
 };
 
@@ -1188,13 +1294,29 @@
 	if (atomic_read(&inode->i_count) != 1)
 		return -EBUSY;
 
+	spin_lock(&ctx->mapping_lock);
 	file->private_data = ctx;
-	file->f_mapping = inode->i_mapping;
-	ctx->mfc = inode->i_mapping;
+	if (!i->i_openers++)
+		ctx->mfc = inode->i_mapping;
+	spin_unlock(&ctx->mapping_lock);
 	smp_wmb();
 	return nonseekable_open(inode, file);
 }
 
+static int
+spufs_mfc_release(struct inode *inode, struct file *file)
+{
+	struct spufs_inode_info *i = SPUFS_I(inode);
+	struct spu_context *ctx = i->i_ctx;
+
+	spin_lock(&ctx->mapping_lock);
+	if (!--i->i_openers)
+		ctx->mfc = NULL;
+	spin_unlock(&ctx->mapping_lock);
+	smp_wmb();
+	return 0;
+}
+
 /* interrupt-level mfc callback function. */
 void spufs_mfc_callback(struct spu *spu)
 {
@@ -1462,6 +1584,7 @@
 
 static struct file_operations spufs_mfc_fops = {
 	.open	 = spufs_mfc_open,
+	.release = spufs_mfc_release,
 	.read	 = spufs_mfc_read,
 	.write	 = spufs_mfc_write,
 	.poll	 = spufs_mfc_poll,
Index: linux-2.6.20/arch/powerpc/platforms/cell/spufs/inode.c
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/cell/spufs/inode.c	2007-03-08 21:57:12.000000000 +0100
+++ linux-2.6.20/arch/powerpc/platforms/cell/spufs/inode.c	2007-03-08 21:57:32.000000000 +0100
@@ -54,6 +54,7 @@
 
 	ei->i_gang = NULL;
 	ei->i_ctx = NULL;
+	ei->i_openers = 0;
 
 	return &ei->vfs_inode;
 }
Index: linux-2.6.20/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/cell/spufs/spufs.h	2007-03-08 21:57:27.000000000 +0100
+++ linux-2.6.20/arch/powerpc/platforms/cell/spufs/spufs.h	2007-03-09 09:27:46.000000000 +0100
@@ -56,6 +56,7 @@
 	struct address_space *signal2; 	   /* 'signal2' area mappings. */
 	struct address_space *mss;	   /* 'mss' area mappings. */
 	struct address_space *psmap;	   /* 'psmap' area mappings. */
+	spinlock_t mapping_lock;
 	u64 object_id;		   /* user space pointer for oprofile */
 
 	enum { SPU_STATE_RUNNABLE, SPU_STATE_SAVED } state;
@@ -168,6 +169,7 @@
 	struct spu_context *i_ctx;
 	struct spu_gang *i_gang;
 	struct inode vfs_inode;
+	int i_openers;
 };
 #define SPUFS_I(inode) \
 	container_of(inode, struct spufs_inode_info, vfs_inode)
Index: linux-2.6.20/arch/powerpc/platforms/cell/spufs/context.c
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/cell/spufs/context.c	2007-03-09 09:28:01.000000000 +0100
+++ linux-2.6.20/arch/powerpc/platforms/cell/spufs/context.c	2007-03-09 09:28:14.000000000 +0100
@@ -40,6 +40,7 @@
 	if (spu_init_csa(&ctx->csa))
 		goto out_free;
 	spin_lock_init(&ctx->mmio_lock);
+	spin_lock_init(&ctx->mapping_lock);
 	kref_init(&ctx->kref);
 	mutex_init(&ctx->state_mutex);
 	init_MUTEX(&ctx->run_sema);



More information about the cbe-oss-dev mailing list