ide: fix races in handling of user-space SET XFER commands
PATCH 5/6 ide: fix races in handling of user-space SET XFER commands
From: Bartlomiej Zolnierkiewicz <[email protected]>
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().
---
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(-)
drivers/ide/ide-ioctls.c +2 -6
@@ -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 ;
drivers/ide/ide-iops.c +0 -10
@@ -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;
drivers/ide/ide-proc.c +2 -8
@@ -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);
drivers/ide/ide-taskfile.c +8 -1
@@ -322,10 +322,17 @@ static void ide_error_cmd(ide_drive_t *dvoid 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)); }
include/linux/ide.h +2 -1
@@ -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;
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 23:35:51 +0200
I just noticed the following while re-reading this patch.
drivers/ide/ide-taskfile.c static void ide_error_cmd(ide_drive_t *d · line 327
Quoted 8 lines
drivers/ide/ide-taskfile.c +2
-1@@ -322,10 +322,17 @@ static void ide_error_cmd(ide_drive_t *dvoid 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.