Messages in this thread | | | Date | Tue, 27 Sep 2005 20:37:23 +0530 | From | Tushar Adeshara <> | Subject | Re: Potential concurrency bug in ide-disk.c ? |
| |
On 9/27/05, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > 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.roblem in practice as idedisk_open() and idedisk_release() are only used in fs/block_dev.c (grep for fops->open and fops > > 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. > > */roblem in practice as idedisk_open() and idedisk_release() are only used in fs/block_dev.c (grep for fops->open and fops > > 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 Its ok. Thanks. >
-- Regards, Tushar -------------------- It's not a problem, it's an opportunity for improvement. Lets improve. - 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/
| |