lkml.org 
[lkml]   [2004]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[PATCH] Fix open/close races in pktcdvd
    From
    Date
    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@telia.com>

    ---

    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@telia.com
    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@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-03-22 14:04    [W:0.057 / U:1.800 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site