[Oracleasm-commits] jlbec commits r357 - trunk/kernel

[svn-commits@oss.oracle.com]

Author: jlbec
Date: 2006-07-28 15:15:17 -0500 (Fri, 28 Jul 2006)
New Revision: 357

Modified:
   trunk/kernel/oracleasm.c
Log:

Port the fix from revision 255 of the oracleasm-1.0 branch:
===========================================================

ASM error testing uncovered a very tight race affecting the
asm_request->r_status field.  The asm_update_user_ioc() call would pull
the userland status field in, because userspace might have set
ASM_CANCELLED, and it would add the values to r_status:

    get_user(tmp_status, user_ioc->status_asm_ioc);
    r->r_status |= tmp_status

The function would then use the asm_request to update the user_ioc with
all the new information.  Note, however, that this r_status update was
done without any locking (bad Joel!).

Under normal I/O, this is never a problem.  asm_update_user_ioc() is
called in two places - right after a submit and when userspace picks
up a completion.  If userspace is picking up a completion, the I/O is
done as far as the kernel is concerned, and no other thread can be
touching it.  Safe.  Right after a submit, it's pretty much safe to
assume that the I/O will take some time, even a milisecond is enough.
The update to userspace happens before the I/O paths can touch r_status.

However, the testing was explicitly pulling a disk.  So, any I/O
submitted to the block/scsi layers would immediately complete.  This
happens in irq or softirq context, so it happens on another cpu.  And
we race:

    cpu1			cpu2
    ----			----
    submit_bh()
    asm_update_user_ioc()
        get_user(tmp_status)
				asm_finish_io()
				spin_lock_irqsave()
    r_status |= tmp_status	r_status |= ASM_COMPLETED
				spin_unlock_irqrestore()

Without locking, those two updates can happen at the same time, and
there is nothing keeping the CPU caches in sync.  cpu1 never sees the
ASM_COMPLETED set on r_status, and squashes right over it.  Effectively,
the I/O never completes, and hangs around in the driver forever.

The fix is twofold.  First, we lock the r_status update in
asm_update_user_ioc().  That's fine and dandy, but we really need to
have a consistent view of the request as we update userspace.  Since
asm_update_user_ioc() is full of put_user() calls, we can't take a
lock around them.  Instead, under the lock still, we copy off the
request.  We can then drop the lock and use the copy to update
userspace.  This gives us a safe update, and gives userspace a
consistent view of the request.



Modified: trunk/kernel/oracleasm.c
===================================================================
--- trunk/kernel/oracleasm.c	2006-07-28 20:13:58 UTC (rev 356)
+++ trunk/kernel/oracleasm.c	2006-07-28 20:15:17 UTC (rev 357)
@@ -974,11 +974,13 @@
 	return bdev;
 }
 
-static int asm_update_user_ioc(struct asm_request *r)
+static int asm_update_user_ioc(struct file *file, struct asm_request *r)
 {
 	int ret = 0;
+	struct asm_request copy;
 	asm_ioc __user *ioc;
 	u16 tmp_status;
+	unsigned long flags;
 
 	mlog_entry("(0x%p)\n", r);
 
@@ -991,38 +993,51 @@
 		ret = -EFAULT;
 		goto out;
 	}
+
+	/*
+	 * We're going to store off a copy of the request so we can
+	 * provide a consistent view to userspace.
+	 *
+	 * And so we can get/put_user() without locking :-)
+	 */
+	spin_lock_irqsave(&ASMFS_FILE(file)->f_lock, flags);
 	r->r_status |= tmp_status;
-	mlog(ML_IOC, "Putting r_status (0x%08X)\n", r->r_status);
-	if (put_user(r->r_status, &(ioc->status_asm_ioc))) {
+	copy = *r;
+	spin_unlock_irqrestore(&ASMFS_FILE(file)->f_lock, flags);
+
+	/* From here on, ONLY TRUST copy */
+
+	mlog(ML_IOC, "Putting r_status (0x%08X)\n", copy.r_status);
+	if (put_user(copy.r_status, &(ioc->status_asm_ioc))) {
 		ret = -EFAULT;
 		goto out;
 	}
-	if (r->r_status & ASM_ERROR) {
-		mlog(ML_IOC, "Putting r_error (0x%08X)\n", r->r_error);
-		if (put_user(r->r_error, &(ioc->error_asm_ioc))) {
+	if (copy.r_status & ASM_ERROR) {
+		mlog(ML_IOC, "Putting r_error (0x%08X)\n", copy.r_error);
+		if (put_user(copy.r_error, &(ioc->error_asm_ioc))) {
 			ret = -EFAULT;
 			goto out;
 		}
 	}
-	if (r->r_status & ASM_COMPLETED) {
-		if (put_user(r->r_elapsed, &(ioc->elaptime_asm_ioc))) {
+	if (copy.r_status & ASM_COMPLETED) {
+		if (put_user(copy.r_elapsed, &(ioc->elaptime_asm_ioc))) {
 			ret = -EFAULT;
 			goto out;
 		}
 	}
 	mlog(ML_IOC,
 	     "r_status:0x%08X, bitmask:0x%08X, combined:0x%08X\n",
-	     r->r_status,
+	     copy.r_status,
 	     (ASM_SUBMITTED | ASM_COMPLETED | ASM_ERROR),
-	     (r->r_status & (ASM_SUBMITTED | ASM_COMPLETED | ASM_ERROR)));
-	if (r->r_status & ASM_FREE) {
+	     (copy.r_status & (ASM_SUBMITTED | ASM_COMPLETED | ASM_ERROR)));
+	if (copy.r_status & ASM_FREE) {
 		u64 z = 0ULL;
 		if (copy_to_user(&(ioc->reserved_asm_ioc),
 				 &z, sizeof(ioc->reserved_asm_ioc))) {
 			ret = -EFAULT;
 			goto out;
 		}
-	} else if (r->r_status &
+	} else if (copy.r_status &
 		   (ASM_SUBMITTED | ASM_ERROR)) {
 		u64 key = (u64)(unsigned long)r;
 		mlog(ML_IOC, "Putting key 0x%p on asm_ioc 0x%p\n",
@@ -1371,7 +1386,7 @@
 	submit_bio(rw, r->r_bio);
 
 out:
-	ret = asm_update_user_ioc(r);
+	ret = asm_update_user_ioc(file, r);
 
 	mlog_exit(ret);
 	return ret;
@@ -1493,7 +1508,7 @@
 
 	spin_unlock_irq(&afi->f_lock);
 
-	ret = asm_update_user_ioc(r);
+	ret = asm_update_user_ioc(file, r);
 
 	mlog(ML_REQUEST, "Freeing request 0x%p\n", r);
 	asm_request_free(r);
@@ -1533,7 +1548,7 @@
 
 	*ioc = r->r_ioc;
 	
-	ret = asm_update_user_ioc(r);
+	ret = asm_update_user_ioc(file, r);
 
 	asm_request_free(r);