[PATCH] sbp2: fix spinlock recursion

[PATCH] sbp2: fix spinlock recursion

[Stefan Richter]

sbp2util_mark_command_completed takes a lock which was already taken by
sbp2scsi_complete_all_commands.  This is a regression in Linux 2.6.15.
Reported by Kristian Harms at
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=187394

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

--- linux-2.6.16.1/drivers/ieee1394/sbp2.c.orig	2006-04-01 19:06:31.000000000 +0200
+++ linux-2.6.16.1/drivers/ieee1394/sbp2.c	2006-04-01 19:07:00.000000000 +0200
@@ -495,22 +495,17 @@ static struct sbp2_command_info *sbp2uti
 /*
  * This function finds the sbp2_command for a given outstanding SCpnt.
  * Only looks at the inuse list.
+ * Must be called with scsi_id->sbp2_command_orb_lock held.
  */
-static struct sbp2_command_info *sbp2util_find_command_for_SCpnt(struct scsi_id_instance_data *scsi_id, void *SCpnt)
+static struct sbp2_command_info *sbp2util_find_command_for_SCpnt(
+		struct scsi_id_instance_data *scsi_id, void *SCpnt)
 {
 	struct sbp2_command_info *command;
-	unsigned long flags;
 
-	spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
-	if (!list_empty(&scsi_id->sbp2_command_orb_inuse)) {
-		list_for_each_entry(command, &scsi_id->sbp2_command_orb_inuse, list) {
-			if (command->Current_SCpnt == SCpnt) {
-				spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
+	if (!list_empty(&scsi_id->sbp2_command_orb_inuse))
+		list_for_each_entry(command, &scsi_id->sbp2_command_orb_inuse, list)
+			if (command->Current_SCpnt == SCpnt)
 				return command;
-			}
-		}
-	}
-	spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 	return NULL;
 }
 
@@ -579,17 +574,15 @@ static void sbp2util_free_command_dma(st
 
 /*
  * This function moves a command to the completed orb list.
+ * Must be called with scsi_id->sbp2_command_orb_lock held.
  */
-static void sbp2util_mark_command_completed(struct scsi_id_instance_data *scsi_id,
-					    struct sbp2_command_info *command)
+static void sbp2util_mark_command_completed(
+		struct scsi_id_instance_data *scsi_id,
+		struct sbp2_command_info *command)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
 	list_del(&command->list);
 	sbp2util_free_command_dma(command);
 	list_add_tail(&command->list, &scsi_id->sbp2_command_orb_completed);
-	spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 }
 
 /*
@@ -2177,7 +2170,9 @@ static int sbp2_handle_status_write(stru
 		 * Matched status with command, now grab scsi command pointers and check status
 		 */
 		SCpnt = command->Current_SCpnt;
+		spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
 		sbp2util_mark_command_completed(scsi_id, command);
+		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 		if (SCpnt) {
 
@@ -2513,6 +2508,7 @@ static int sbp2scsi_abort(struct scsi_cm
 		(struct scsi_id_instance_data *)SCpnt->device->host->hostdata[0];
 	struct sbp2scsi_host_info *hi = scsi_id->hi;
 	struct sbp2_command_info *command;
+	unsigned long flags;
 
 	SBP2_ERR("aborting sbp2 command");
 	scsi_print_command(SCpnt);
@@ -2523,6 +2519,7 @@ static int sbp2scsi_abort(struct scsi_cm
 		 * Right now, just return any matching command structures
 		 * to the free pool.
 		 */
+		spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
 		command = sbp2util_find_command_for_SCpnt(scsi_id, SCpnt);
 		if (command) {
 			SBP2_DEBUG("Found command to abort");
@@ -2540,6 +2537,7 @@ static int sbp2scsi_abort(struct scsi_cm
 				command->Current_done(command->Current_SCpnt);
 			}
 		}
+		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 		/*
 		 * Initiate a fetch agent reset.

Re: [PATCH] sbp2: fix spinlock recursion

[Andrew Morton]

Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>
> @@ -2540,6 +2537,7 @@ static int sbp2scsi_abort(struct scsi_cm
>   				command->Current_done(command->Current_SCpnt);
>   			}
>   		}
>  +		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);

This changes the call environment for all implementations of
->Current_done().  Are they all safe to call under this lock?

Re: [PATCH] sbp2: fix spinlock recursion

[Stefan Richter]

Andrew Morton wrote:
> Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
>>@@ -2540,6 +2537,7 @@ static int sbp2scsi_abort(struct scsi_cm
>>  				command->Current_done(command->Current_SCpnt);
>>  			}
>>  		}
>> +		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
> 
> 
> This changes the call environment for all implementations of
> ->Current_done().  Are they all safe to call under this lock?

Short answer: Yes, trust me. ;-) Long answer:

The done() callbacks are passed on to sbp2 from the SCSI stack along 
with each SCSI command via the queuecommand hook. The done() callback is 
safe to call in atomic context. So does 
Documentation/scsi/scsi_mid_low_api.txt say, and many if not all SCSI 
low-level handlers rely on this fact. So whatever this callback does, it 
is "self-contained" and it won't conflict with sbp2's internal ORB list 
handling. In particular, it won't race with the sbp2_command_orb_lock.

Moreover, sbp2 already calls the done() handler with 
sbp2_command_orb_lock taken in sbp2scsi_complete_all_commands(). I admit 
this is ultimately no proof of correctness, especially since this 
portion of code introduced the spinlock recursion in the first place and 
we didn't realize it since this code's submission before 2.6.15 until 
now. (I have learned a lesson from this.)

I stress-tested my patch on x86 uniprocessor with a preemptible SMP 
kernel (alas I have no SMP machine yet) and made sure that all code 
paths which involve the sbp2_command_orb_lock were gone through multiple 
times. Which is of course also no proof.
-- 
Stefan Richter
-=====-=-==- -=-- ---=-
http://arcgraph.de/sr/

Re: [stable] [PATCH] sbp2: fix spinlock recursion

[Greg KH]

On Sat, Apr 01, 2006 at 09:11:41PM +0200, Stefan Richter wrote:
> sbp2util_mark_command_completed takes a lock which was already taken by
> sbp2scsi_complete_all_commands.  This is a regression in Linux 2.6.15.
> Reported by Kristian Harms at
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=187394
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

queued to -stable, thanks.

greg k-h