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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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