Re: Potential concurrency bug in ide-disk.c ?

Bartlomiej Zolnierkiewicz
2005-09-27T13:59:16Z

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