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

[Tushar Adeshara <adesharatushar@gmail.com>]

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.