lkml.org 
[lkml]   [2005]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Potential concurrency bug in ide-disk.c ?
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
-
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/

\
 
 \ /
  Last update: 2005-09-27 16:01    [W:0.052 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site