[Cbe-oss-dev] RFC: Fix race in SPU coredump handling

[Michael Ellerman]

Hi folks,

I just wanted to run this by people before I post it.

There's a race in the SPU coredump code where two crashing processes can
corrupt each other's coredump. This is caused by the fact that we do the
coredump logic in two steps.

First we calculate the size of the ELF notes we'll write, as part of
this process we add ctx_info structs to a global list. Then later we
actually write the ELF notes, based on the contents of the list. As we
go through the list we delete the ctx_info structs we've processed.

If two processes crash around the same time, they'll both add ctx_info
structs to the list, but then the first process to do the writing will
delete all the ctx_info structs. The second process will then come along
and find nothing in the list, and so write nothing. This leads to a
corrupt core dump for the second process.

The fix as I see it is just to get rid of the list altogether. Instead
we just iterate through our file descriptors a second time (in the write
routine) to find the spu_contexts again.

AFAICT there's no reason to believe file descriptors will go away
between the size and the write routines, so doing the loop through the
file descriptors twice is 100% safe. But I wanted to check no one thinks
otherwise - or perhaps someone can remember some other motivation for
the list.

Here's my patch so far as well.

cheers

ps. for mainline I'm planning on totally reworking this so that the size
and write callbacks are unified - that should be less code, and also
guarantees that we never write more or less than we said we would.

diff --git a/arch/powerpc/platforms/cell/spufs/coredump.c b/arch/powerpc/platforms/cell/spufs/coredump.c
index 5e31799..318ced2 100644
--- a/arch/powerpc/platforms/cell/spufs/coredump.c
+++ b/arch/powerpc/platforms/cell/spufs/coredump.c
@@ -31,15 +31,6 @@
 
 #include "spufs.h"
 
-struct spufs_ctx_info {
-	struct list_head list;
-	int dfd;
-	int memsize; /* in bytes */
-	struct spu_context *ctx;
-};
-
-static LIST_HEAD(ctx_info_list);
-
 static ssize_t do_coredump_read(int num, struct spu_context *ctx, void __user *buffer,
 				size_t size, loff_t *off)
 {
@@ -73,30 +64,24 @@ static int spufs_dump_seek(struct file *file, loff_t off)
 	return 1;
 }
 
-static void spufs_fill_memsize(struct spufs_ctx_info *ctx_info)
+static u64 spufs_memsize(struct spu_context *ctx)
 {
-	struct spu_context *ctx;
-	unsigned long long lslr;
-
-	ctx = ctx_info->ctx;
-	lslr = ctx->csa.priv2.spu_lslr_RW;
-	ctx_info->memsize = lslr + 1;
+	return ctx->csa.priv2.spu_lslr_RW + 1;
 }
 
-static int spufs_ctx_note_size(struct spufs_ctx_info *ctx_info)
+static int spufs_ctx_note_size(struct spu_context *ctx, int dir_fd)
 {
-	int dfd, memsize, i, sz, total = 0;
+	int memsize, i, sz, total = 0;
 	char *name;
 	char fullname[80];
 
-	dfd = ctx_info->dfd;
-	memsize = ctx_info->memsize;
+	memsize = spufs_memsize(ctx);
 
 	for (i = 0; spufs_coredump_read[i].name; i++) {
 		name = spufs_coredump_read[i].name;
 		sz = spufs_coredump_read[i].size;
 
-		sprintf(fullname, "SPU/%d/%s", dfd, name);
+		sprintf(fullname, "SPU/%d/%s", dir_fd, name);
 
 		total += sizeof(struct elf_note);
 		total += roundup(strlen(fullname) + 1, 4);
@@ -109,27 +94,17 @@ static int spufs_ctx_note_size(struct spufs_ctx_info *ctx_info)
 	return total;
 }
 
-static int spufs_add_one_context(struct file *file, int dfd)
+static int spufs_size_one_context(struct file *file, int dir_fd)
 {
 	struct spu_context *ctx;
-	struct spufs_ctx_info *ctx_info;
 	int size;
 
 	ctx = SPUFS_I(file->f_dentry->d_inode)->i_ctx;
 	if (ctx->flags & SPU_CREATE_NOSCHED)
 		return 0;
 
-	ctx_info = kzalloc(sizeof(*ctx_info), GFP_KERNEL);
-	if (unlikely(!ctx_info))
-		return -ENOMEM;
-
-	ctx_info->dfd = dfd;
-	ctx_info->ctx = ctx;
+	size = spufs_ctx_note_size(ctx, dir_fd);
 
-	spufs_fill_memsize(ctx_info);
-
-	size = spufs_ctx_note_size(ctx_info);
-	list_add(&ctx_info->list, &ctx_info_list);
 	return size;
 }
 
@@ -152,7 +127,7 @@ static int spufs_arch_notes_size(void)
 			struct file *file = fcheck(fd);
 
 			if (file && file->f_op == &spufs_context_fops) {
-				int rval = spufs_add_one_context(file, fd);
+				int rval = spufs_size_one_context(file, fd);
 				if (rval < 0)
 					break;
 				size += rval;
@@ -163,12 +138,11 @@ static int spufs_arch_notes_size(void)
 	return size;
 }
 
-static void spufs_arch_write_note(struct spufs_ctx_info *ctx_info, int i,
-				struct file *file)
+static void spufs_arch_write_note(struct spu_context *ctx, int i,
+				  struct file *file, int dir_fd)
 {
-	struct spu_context *ctx;
 	loff_t pos = 0;
-	int sz, dfd, rc, total = 0;
+	int sz, rc, total = 0;
 	const int bufsz = PAGE_SIZE;
 	char *name;
 	char fullname[80], *buf;
@@ -178,19 +152,14 @@ static void spufs_arch_write_note(struct spufs_ctx_info *ctx_info, int i,
 	if (!buf)
 		return;
 
-	dfd = ctx_info->dfd;
 	name = spufs_coredump_read[i].name;
 
 	if (!strcmp(name, "mem"))
-		sz = ctx_info->memsize;
+		sz = spufs_memsize(ctx);
 	else
 		sz = spufs_coredump_read[i].size;
 
-	ctx = ctx_info->ctx;
-	if (!ctx)
-		goto out;
-
-	sprintf(fullname, "SPU/%d/%s", dfd, name);
+	sprintf(fullname, "SPU/%d/%s", dir_fd, name);
 	en.n_namesz = strlen(fullname) + 1;
 	en.n_descsz = sz;
 	en.n_type = NT_SPU;
@@ -217,18 +186,39 @@ out:
 	free_page((unsigned long)buf);
 }
 
-static void spufs_arch_write_notes(struct file *file)
+static void spufs_arch_write_ctx_notes(struct file *file, struct file *fd_file,
+				       int dir_fd)
 {
+	struct spu_context *ctx;
 	int j;
-	struct spufs_ctx_info *ctx_info, *next;
-
-	list_for_each_entry_safe(ctx_info, next, &ctx_info_list, list) {
-		spu_acquire_saved(ctx_info->ctx);
-		for (j = 0; j < spufs_coredump_num_notes; j++)
-			spufs_arch_write_note(ctx_info, j, file);
-		spu_release_saved(ctx_info->ctx);
-		list_del(&ctx_info->list);
-		kfree(ctx_info);
+
+	ctx = SPUFS_I(fd_file->f_dentry->d_inode)->i_ctx;
+
+	if (ctx->flags & SPU_CREATE_NOSCHED)
+		return;	/* FIXME */
+
+
+	spu_acquire_saved(ctx);
+
+	for (j = 0; j < spufs_coredump_num_notes; j++)
+		spufs_arch_write_note(ctx, j, file, dir_fd);
+
+	spu_release_saved(ctx);
+}
+
+static void spufs_arch_write_notes(struct file *file)
+{
+	struct fdtable *fdt = files_fdtable(current->files);
+	int dir_fd;
+
+	for (dir_fd = 0; dir_fd < fdt->max_fds; dir_fd++) {
+		if (FD_ISSET(dir_fd, fdt->open_fds)) {
+			struct file *fd_file = fcheck(dir_fd);
+
+			if (fd_file && fd_file->f_op == &spufs_context_fops)
+				spufs_arch_write_ctx_notes(file, fd_file,
+							   dir_fd);
+		}
 	}
 }