Messages in this thread | | | Date | Tue, 27 Sep 2005 15:59:14 +0200 | From | Bartlomiej Zolnierkiewicz <> | Subject | Re: Potential concurrency bug in ide-disk.c ? |
| |
On 9/2/05, Tushar Adeshara <adesharatushar@gmail.com> wrote: > Hi, > The way file ide-disk.c handles usage count, it seems to me that its > concurrency bug. > In open method and release, it uses code as follows > > > static int idedisk_open(struct inode *inode, struct file *filp) > { > ide_drive_t *drive = inode->i_bdev->bd_disk->private_data; > drive->usage++; > if (drive->removable && drive->usage == 1) { > ide_task_t args; > memset(&args, 0, sizeof(ide_task_t)); > args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK; > args.command_type = IDE_DRIVE_TASK_NO_DATA; > args.handler = &task_no_data_intr; > check_disk_change(inode->i_bdev); > /* > * Ignore the return code from door_lock, > * since the open() has already succeeded, > * and the door_lock is irrelevant at this point. > */ > if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL)) > drive->doorlocking = 0; > } > return 0; > } > > > Here, if drive->usage=0 initially and two process concurrently executes > drive->usage++, then drive->usage will become 2. Both of them will > think that drive is already initialized. Something similar can happen > in case of release. > I think a semaphore need to be added in > ide_drive_t structure and method should be modified as > > static int idedisk_open(struct inode *inode, struct file *filp) > { > ide_drive_t *drive = inode->i_bdev->bd_disk->private_data; > if(down_interruptible(&drive->sem)){ > /*error handling code*/ > } > drive->usage++; > if (drive->removable && drive->usage == 1) { > ide_task_t args; > memset(&args, 0, sizeof(ide_task_t)); > args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK; > args.command_type = IDE_DRIVE_TASK_NO_DATA; > args.handler = &task_no_data_intr; > check_disk_change(inode->i_bdev); > /* > * Ignore the return code from door_lock, > * since the open() has already succeeded, > * and the door_lock is irrelevant at this point. > */ > if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL)) > drive->doorlocking = 0; > } > up(&drive->sem); > return 0; > } > Similar modifications are also required in release.
Not a problem in practice as idedisk_open() and idedisk_release() are only used in fs/block_dev.c (grep for fops->open and fops->release) and are protected against concurrent execution by bdev->bd_sem.
Bartlomiej - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |