[PATCH] Fix open/close races in pktcdvd
From: Peter Osterlund
Date: Sun Jul 04 2004 - 07:43:13 EST
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.
This patch applies cleanly on top of the "Fix race in pktcdvd kernel
thread handling" patch.
Signed-off-by: Peter Osterlund <petero2@xxxxxxxxx>
---
linux-petero/drivers/block/pktcdvd.c | 88 +++++++++++++++++++++--------------
linux-petero/include/linux/pktcdvd.h | 3 -
2 files changed, 55 insertions(+), 36 deletions(-)
diff -puN drivers/block/pktcdvd.c~packet-open-race-fix drivers/block/pktcdvd.c
--- linux/drivers/block/pktcdvd.c~packet-open-race-fix 2004-07-04 14:09:40.840062728 +0200
+++ linux-petero/drivers/block/pktcdvd.c 2004-07-04 14:09:40.850061208 +0200
@@ -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~packet-open-race-fix include/linux/pktcdvd.h
--- linux/include/linux/pktcdvd.h~packet-open-race-fix 2004-07-04 14:09:40.843062272 +0200
+++ linux-petero/include/linux/pktcdvd.h 2004-07-04 14:09:40.850061208 +0200
@@ -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 */
_
--
Peter Osterlund - petero2@xxxxxxxxx
http://w1.894.telia.com/~u89404340
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/