From: Peter Osterlund The handling of the pd->refcnt variable is racy in a number of places. For example, running: while true ; do usleep 10 ; pktsetup /dev/pktcdvd0 /dev/hdc ; done & while true ; do pktsetup -d /dev/pktcdvd0 ; done makes a pktsetup process get stuck in D state after a while. This patch fixes it by introducing a mutex to protect the refcnt variable and critical sections in the open/release/setup/tear-down functions. Signed-off-by: Peter Osterlund Signed-off-by: Andrew Morton --- 25-akpm/drivers/block/pktcdvd.c | 88 ++++++++++++++++++++++++---------------- 25-akpm/include/linux/pktcdvd.h | 3 - 2 files changed, 55 insertions(+), 36 deletions(-) diff -puN drivers/block/pktcdvd.c~fix-open-close-races-in-pktcdvd drivers/block/pktcdvd.c --- 25/drivers/block/pktcdvd.c~fix-open-close-races-in-pktcdvd 2004-07-04 13:59:56.308944440 -0700 +++ 25-akpm/drivers/block/pktcdvd.c 2004-07-04 13:59:56.316943224 -0700 @@ -1998,10 +1998,6 @@ static void pkt_release_dev(struct pktcd { struct block_device *bdev; - atomic_dec(&pd->refcnt); - if (atomic_read(&pd->refcnt) > 0) - return; - bdev = bdget(pd->pkt_dev); if (bdev) { fsync_bdev(bdev); @@ -2029,6 +2025,7 @@ static int pkt_open(struct inode *inode, struct pktcdvd_device *pd = NULL; struct block_device *pkt_bdev; int ret; + int special_open, exclusive; VPRINTK("pktcdvd: entering open\n"); @@ -2038,21 +2035,30 @@ static int pkt_open(struct inode *inode, goto out; } + special_open = 0; + if (((file->f_flags & O_ACCMODE) == O_RDONLY) && (file->f_flags & O_CREAT)) + special_open = 1; + + exclusive = 0; + if ((file->f_mode & FMODE_WRITE) || special_open) + exclusive = 1; + /* * either device is not configured, or pktsetup is old and doesn't * use O_CREAT to create device */ pd = &pkt_devs[iminor(inode)]; - if (!pd->dev && !(file->f_flags & O_CREAT)) { + if (!pd->dev && !special_open) { VPRINTK("pktcdvd: not configured and O_CREAT not set\n"); ret = -ENXIO; goto out; } - atomic_inc(&pd->refcnt); - if (atomic_read(&pd->refcnt) > 1) { - if (file->f_mode & FMODE_WRITE) { - VPRINTK("pktcdvd: busy open for write\n"); + down(&pd->ctl_mutex); + pd->refcnt++; + if (pd->refcnt > 1) { + if (exclusive) { + VPRINTK("pktcdvd: busy open\n"); ret = -EBUSY; goto out_dec; } @@ -2060,10 +2066,10 @@ static int pkt_open(struct inode *inode, /* * Not first open, everything is already set up */ - return 0; + goto done; } - if (((file->f_flags & O_ACCMODE) != O_RDONLY) || !(file->f_flags & O_CREAT)) { + if (!special_open) { if (pkt_open_dev(pd, file->f_mode & FMODE_WRITE)) { ret = -EIO; goto out_dec; @@ -2079,16 +2085,20 @@ static int pkt_open(struct inode *inode, set_blocksize(pkt_bdev, CD_FRAMESIZE); bdput(pkt_bdev); } + +done: + up(&pd->ctl_mutex); return 0; out_dec: - atomic_dec(&pd->refcnt); - if (atomic_read(&pd->refcnt) == 0) { + pd->refcnt--; + if (pd->refcnt == 0) { if (pd->bdev) { blkdev_put(pd->bdev); pd->bdev = NULL; } } + up(&pd->ctl_mutex); out: VPRINTK("pktcdvd: failed open (%d)\n", ret); return ret; @@ -2099,11 +2109,17 @@ static int pkt_close(struct inode *inode struct pktcdvd_device *pd = &pkt_devs[iminor(inode)]; int ret = 0; + down(&pd->ctl_mutex); + pd->refcnt--; + BUG_ON(pd->refcnt < 0); + if (pd->refcnt > 0) + goto out; if (pd->dev) { int flush = test_bit(PACKET_WRITABLE, &pd->flags); pkt_release_dev(pd, flush); } - +out: + up(&pd->ctl_mutex); return ret; } @@ -2332,7 +2348,7 @@ static int pkt_proc_device(struct pktcdv b += sprintf(b, "\tread:\t\t\t%lukB\n", pd->stats.secs_r >> 1); b += sprintf(b, "\nMisc:\n"); - b += sprintf(b, "\treference count:\t%d\n", atomic_read(&pd->refcnt)); + b += sprintf(b, "\treference count:\t%d\n", pd->refcnt); b += sprintf(b, "\tflags:\t\t\t0x%lx\n", pd->flags); b += sprintf(b, "\tread speed:\t\t%ukB/s\n", pd->read_speed * 150); b += sprintf(b, "\twrite speed:\t\t%ukB/s\n", pd->write_speed * 150); @@ -2371,8 +2387,6 @@ static int pkt_read_proc(char *page, cha return len; } -extern struct block_device_operations pktcdvd_ops; - static int pkt_new_dev(struct pktcdvd_device *pd, struct block_device *bdev) { dev_t dev = bdev->bd_dev; @@ -2392,10 +2406,8 @@ static int pkt_new_dev(struct pktcdvd_de /* This is safe, since we have a reference from open(). */ __module_get(THIS_MODULE); - memset(pd, 0, sizeof(struct pktcdvd_device)); + memset(&pd->stats, 0, sizeof(struct packet_stats)); - spin_lock_init(&pd->lock); - spin_lock_init(&pd->iosched.lock); if (!pkt_grow_pktlist(pd, CONFIG_CDROM_PKTCDVD_BUFFERS)) { printk("pktcdvd: not enough memory for buffers\n"); ret = -ENOMEM; @@ -2404,12 +2416,7 @@ static int pkt_new_dev(struct pktcdvd_de set_blocksize(bdev, CD_FRAMESIZE); pd->dev = dev; - pd->bdev = NULL; - pd->pkt_dev = MKDEV(PACKET_MAJOR, minor); - sprintf(pd->name, "pktcdvd%d", minor); - atomic_set(&pd->refcnt, 0); - init_waitqueue_head(&pd->wqueue); - init_MUTEX_LOCKED(&pd->cdrw.thr_sem); + BUG_ON(pd->bdev); pkt_init_queue(pd); @@ -2428,7 +2435,6 @@ static int pkt_new_dev(struct pktcdvd_de out_thread: pkt_shrink_pktlist(pd); - memset(pd, 0, sizeof(*pd)); out_mem: /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); @@ -2464,10 +2470,7 @@ static int pkt_setup_dev(struct pktcdvd_ printk("pktcdvd: Can't write to read-only dev\n"); goto out; } - if ((ret = pkt_new_dev(pd, inode->i_bdev))) - goto out; - - atomic_inc(&pd->refcnt); + ret = pkt_new_dev(pd, inode->i_bdev); out: fput(file); @@ -2502,8 +2505,8 @@ static int pkt_remove_dev(struct pktcdvd pkt_shrink_pktlist(pd); remove_proc_entry(pd->name, pkt_proc); + pd->dev = 0; DPRINTK("pktcdvd: writer %d unmapped\n", pkt_get_minor(pd)); - memset(pd, 0, sizeof(struct pktcdvd_device)); /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); return 0; @@ -2527,6 +2530,7 @@ static int pkt_media_changed(struct gend static int pkt_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg) { struct pktcdvd_device *pd = &pkt_devs[iminor(inode)]; + int ret; VPRINTK("pkt_ioctl: cmd %x, dev %d:%d\n", cmd, imajor(inode), iminor(inode)); @@ -2553,9 +2557,14 @@ static int pkt_ioctl(struct inode *inode case PACKET_TEARDOWN_DEV: if (!capable(CAP_SYS_ADMIN)) return -EPERM; - if (atomic_read(&pd->refcnt) != 1) - return -EBUSY; - return pkt_remove_dev(pd); + down(&pd->ctl_mutex); + BUG_ON(pd->refcnt < 1); + if (pd->refcnt != 1) + ret = -EBUSY; + else + ret = pkt_remove_dev(pd); + up(&pd->ctl_mutex); + return ret; case BLKROSET: if (capable(CAP_SYS_ADMIN)) @@ -2629,6 +2638,15 @@ int pkt_init(void) for (i = 0; i < MAX_WRITERS; i++) { struct pktcdvd_device *pd = &pkt_devs[i]; struct gendisk *disk = disks[i]; + + spin_lock_init(&pd->lock); + spin_lock_init(&pd->iosched.lock); + pd->pkt_dev = MKDEV(PACKET_MAJOR, i); + sprintf(pd->name, "pktcdvd%d", i); + init_waitqueue_head(&pd->wqueue); + init_MUTEX_LOCKED(&pd->cdrw.thr_sem); + init_MUTEX(&pd->ctl_mutex); + disk->major = PACKET_MAJOR; disk->first_minor = i; disk->fops = &pktcdvd_ops; diff -puN include/linux/pktcdvd.h~fix-open-close-races-in-pktcdvd include/linux/pktcdvd.h --- 25/include/linux/pktcdvd.h~fix-open-close-races-in-pktcdvd 2004-07-04 13:59:56.310944136 -0700 +++ 25-akpm/include/linux/pktcdvd.h 2004-07-04 13:59:56.317943072 -0700 @@ -236,7 +236,8 @@ struct pktcdvd_device char name[20]; struct packet_settings settings; struct packet_stats stats; - atomic_t refcnt; + struct semaphore ctl_mutex; /* Serialize access to refcnt */ + int refcnt; /* Open count */ __u8 write_speed; /* current write speed */ __u8 read_speed; /* current read speed */ unsigned long offset; /* start offset */ _