diff mbox

[5/6] ide: fix races in handling of user-space SET XFER commands

Message ID 200906232335.51621.bzolnier@gmail.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Bartlomiej Zolnierkiewicz June 23, 2009, 9:35 p.m. UTC 
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: fix races in handling of user-space SET XFER commands

* Make cmd->tf_flags field 'u16' and add IDE_TFLAG_SET_XFER taskfile flag.

* Update ide_finish_cmd() to set xfer / re-read id if the new flag is set.

* Convert set_xfer_rate() (write handler for /proc/ide/hd?/current_speed)
  and ide_cmd_ioctl() (HDIO_DRIVE_CMD ioctl handler) to use the new flag.

* Remove no longer needed disable_irq_nosync() + enable_irq() from
  ide_config_drive_speed().

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
These races are easily triggable (i.e. 'hdparm -Xmode' results in the "bad
status" error during ide_driveid_update() phase) and I was no longer able to
reproduce them with this patch applied.

 drivers/ide/ide-ioctls.c   |    8 ++------
 drivers/ide/ide-iops.c     |   10 ----------
 drivers/ide/ide-proc.c     |   10 ++--------
 drivers/ide/ide-taskfile.c |    9 ++++++++-
 include/linux/ide.h        |    3 ++-
 5 files changed, 14 insertions(+), 26 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller June 23, 2009, 11:40 p.m. UTC | #1 
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 23 Jun 2009 23:35:51 +0200

I just noticed the following while re-reading this patch.

> @@ -322,10 +322,17 @@ static void ide_error_cmd(ide_drive_t *d
>  void ide_finish_cmd(ide_drive_t *drive, struct ide_cmd *cmd, u8 stat)
>  {
>  	struct request *rq = drive->hwif->rq;
> -	u8 err = ide_read_error(drive);
> +	u8 err = ide_read_error(drive), nsect = cmd->tf.nsect;
> +	u8 set_xfer = !!(cmd->tf_flags & IDE_TFLAG_SET_XFER);
>  

There is no reason to use 'u8' for set_xfer.  It's a boolean
so use 'bool' and then you can eliminate that "!!()" double
negate.

The only reason you would need that double-negate is because
cmd->tf_flags is a u16 and IDE_TFLAG_SET_XFER is in fact one
of those upper 8-bits.

Further confirming that a bool would be a better choice here.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: b/drivers/ide/ide-ioctls.c
===================================================================
--- a/drivers/ide/ide-ioctls.c
+++ b/drivers/ide/ide-ioctls.c
@@ -166,6 +166,8 @@  static int ide_cmd_ioctl(ide_drive_t *dr
 			err = -EINVAL;
 			goto abort;
 		}
+
+		cmd.tf_flags |= IDE_TFLAG_SET_XFER;
 	}
 
 	err = ide_raw_taskfile(drive, &cmd, buf, args[3]);
@@ -173,12 +175,6 @@  static int ide_cmd_ioctl(ide_drive_t *dr
 	args[0] = tf->status;
 	args[1] = tf->error;
 	args[2] = tf->nsect;
-
-	if (!err && xfer_rate) {
-		/* active-retuning-calls future */
-		ide_set_xfer_rate(drive, xfer_rate);
-		ide_driveid_update(drive);
-	}
 abort:
 	if (copy_to_user((void __user *)arg, &args, 4))
 		err = -EFAULT;
Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -363,14 +363,6 @@  int ide_config_drive_speed(ide_drive_t *
 	 * this point (lost interrupt).
 	 */
 
-	/*
-	 *	FIXME: we race against the running IRQ here if
-	 *	this is called from non IRQ context. If we use
-	 *	disable_irq() we hang on the error path. Work
-	 *	is needed.
-	 */
-	disable_irq_nosync(hwif->irq);
-
 	udelay(1);
 	tp_ops->dev_select(drive);
 	SELECT_MASK(drive, 1);
@@ -394,8 +386,6 @@  int ide_config_drive_speed(ide_drive_t *
 
 	SELECT_MASK(drive, 0);
 
-	enable_irq(hwif->irq);
-
 	if (error) {
 		(void) ide_dump_status(drive, "set_drive_speed_status", stat);
 		return error;
Index: b/drivers/ide/ide-proc.c
===================================================================
--- a/drivers/ide/ide-proc.c
+++ b/drivers/ide/ide-proc.c
@@ -195,7 +195,6 @@  ide_devset_get(xfer_rate, current_speed)
 static int set_xfer_rate (ide_drive_t *drive, int arg)
 {
 	struct ide_cmd cmd;
-	int err;
 
 	if (arg < XFER_PIO_0 || arg > XFER_UDMA_6)
 		return -EINVAL;
@@ -206,14 +205,9 @@  static int set_xfer_rate (ide_drive_t *d
 	cmd.tf.nsect   = (u8)arg;
 	cmd.valid.out.tf = IDE_VALID_FEATURE | IDE_VALID_NSECT;
 	cmd.valid.in.tf  = IDE_VALID_NSECT;
+	cmd.tf_flags   = IDE_TFLAG_SET_XFER;
 
-	err = ide_no_data_taskfile(drive, &cmd);
-
-	if (!err) {
-		ide_set_xfer_rate(drive, (u8) arg);
-		ide_driveid_update(drive);
-	}
-	return err;
+	return ide_no_data_taskfile(drive, &cmd);
 }
 
 ide_devset_rw(current_speed, xfer_rate);
Index: b/drivers/ide/ide-taskfile.c
===================================================================
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -322,10 +322,17 @@  static void ide_error_cmd(ide_drive_t *d
 void ide_finish_cmd(ide_drive_t *drive, struct ide_cmd *cmd, u8 stat)
 {
 	struct request *rq = drive->hwif->rq;
-	u8 err = ide_read_error(drive);
+	u8 err = ide_read_error(drive), nsect = cmd->tf.nsect;
+	u8 set_xfer = !!(cmd->tf_flags & IDE_TFLAG_SET_XFER);
 
 	ide_complete_cmd(drive, cmd, stat, err);
 	rq->errors = err;
+
+	if (err == 0 && set_xfer) {
+		ide_set_xfer_rate(drive, nsect);
+		ide_driveid_update(drive);
+	}
+
 	ide_complete_rq(drive, err ? -EIO : 0, blk_rq_bytes(rq));
 }
 
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -258,6 +258,7 @@  enum {
 	IDE_TFLAG_DYN			= (1 << 5),
 	IDE_TFLAG_FS			= (1 << 6),
 	IDE_TFLAG_MULTI_PIO		= (1 << 7),
+	IDE_TFLAG_SET_XFER		= (1 << 8),
 };
 
 enum {
@@ -294,7 +295,7 @@  struct ide_cmd {
 		} out, in;
 	} valid;
 
-	u8			tf_flags;
+	u16			tf_flags;
 	u8			ftf_flags;	/* for TASKFILE ioctl */
 	int			protocol;