On 9/2/05, Tushar Adeshara <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |