* Potential concurrency bug in ide-disk.c ?
@ 2005-09-02 11:38 Tushar Adeshara
2005-09-27 13:59 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 3+ messages in thread
From: Tushar Adeshara @ 2005-09-02 11:38 UTC (permalink / raw)
To: linux-kernel
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.
Please let me know if there is anything wrong in above code. Also let
me know to whom I should offer patches for this.
--
Regards,
Tushar
--------------------
It's not a problem, it's an opportunity for improvement. Lets improve.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Potential concurrency bug in ide-disk.c ?
2005-09-02 11:38 Potential concurrency bug in ide-disk.c ? Tushar Adeshara
@ 2005-09-27 13:59 ` Bartlomiej Zolnierkiewicz
2005-09-27 15:07 ` Tushar Adeshara
0 siblings, 1 reply; 3+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-09-27 13:59 UTC (permalink / raw)
To: Tushar Adeshara; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Potential concurrency bug in ide-disk.c ?
2005-09-27 13:59 ` Bartlomiej Zolnierkiewicz
@ 2005-09-27 15:07 ` Tushar Adeshara
0 siblings, 0 replies; 3+ messages in thread
From: Tushar Adeshara @ 2005-09-27 15:07 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2005-09-27 15:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-09-02 11:38 Potential concurrency bug in ide-disk.c ? Tushar Adeshara
2005-09-27 13:59 ` Bartlomiej Zolnierkiewicz
2005-09-27 15:07 ` Tushar Adeshara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox