2001-10-23 10:45:30

by Alexander Viro

[permalink] [raw]
Subject: [CFT][PATCH] cdrom-related rmmod races

Folks, patch below should fix the rmmod/open() races for
cdrom drivers (including pcd and sr). The problem with the current
code is that they use cdrom_open() as ->open() and bump module
refcount only in cdrom_device_info->open() callback. cdrom_open()
blocks before calling it, with obvious results.

Patch is pretty straightforward - it gives these guys ->open()
of their own that does MOD_INC_USE_COUNT and calls cdrom_open() - see
below for examples. Please, give it a try - if there's no complaints it
goes to Linus.

diff -urN S13-pre6/drivers/block/paride/pcd.c S13-pre6-current/drivers/block/paride/pcd.c
--- S13-pre6/drivers/block/paride/pcd.c Sun Oct 21 15:17:06 2001
+++ S13-pre6-current/drivers/block/paride/pcd.c Mon Oct 22 22:52:22 2001
@@ -204,6 +204,8 @@
int pcd_init(void);
void cleanup_module( void );

+static int pcd_dev_open(struct inode *inode, struct file *file);
+static void pcd_dev_release(struct inode *inode, struct file *file);
static int pcd_open(struct cdrom_device_info *cdi, int purpose);
static void pcd_release(struct cdrom_device_info *cdi);
static int pcd_drive_status(struct cdrom_device_info *cdi, int slot_nr);
@@ -265,6 +267,13 @@

/* kernel glue structures */

+struct block_device_operations pcd_bdops = {
+ open: pcd_dev_open,
+ release: pcd_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+}
+
static struct cdrom_device_ops pcd_dops = {
pcd_open,
pcd_release,
@@ -335,13 +344,17 @@
/* get the atapi capabilities page */
pcd_probe_capabilities();

- if (register_blkdev(MAJOR_NR,name,&cdrom_fops)) {
+ if (register_blkdev(MAJOR_NR,name,&pcd_bdops)) {
printk("pcd: unable to get major number %d\n",MAJOR_NR);
return -1;
}

- for (unit=0;unit<PCD_UNITS;unit++)
- if (PCD.present) register_cdrom(&PCD.info);
+ for (unit=0;unit<PCD_UNITS;unit++) {
+ if (PCD.present) {
+ register_cdrom(&PCD.info);
+ devfs_plain_cdrom(&PCD.info, &pcd_bdops);
+ }
+ }

blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), DEVICE_REQUEST);
read_ahead[MAJOR_NR] = 8; /* 8 sector (4kB) read ahead */
@@ -352,20 +365,36 @@
return 0;
}

+static int pcd_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int pcd_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
static int pcd_open(struct cdrom_device_info *cdi, int purpose)

{ int unit = DEVICE_NR(cdi->dev);

if ((unit >= PCD_UNITS) || (!PCD.present)) return -ENODEV;

- MOD_INC_USE_COUNT;
-
return 0;
}

static void pcd_release(struct cdrom_device_info *cdi)

-{ MOD_DEC_USE_COUNT;
+{
}

#ifdef MODULE
diff -urN S13-pre6/drivers/cdrom/cdrom.c S13-pre6-current/drivers/cdrom/cdrom.c
--- S13-pre6/drivers/cdrom/cdrom.c Sun Oct 21 15:17:06 2001
+++ S13-pre6-current/drivers/cdrom/cdrom.c Mon Oct 22 23:58:14 2001
@@ -310,11 +310,6 @@
#define CHECKAUDIO if ((ret=check_for_audio_disc(cdi, cdo))) return ret

/* Not-exported routines. */
-static int cdrom_open(struct inode *ip, struct file *fp);
-static int cdrom_release(struct inode *ip, struct file *fp);
-static int cdrom_ioctl(struct inode *ip, struct file *fp,
- unsigned int cmd, unsigned long arg);
-static int cdrom_media_changed(kdev_t dev);
static int open_for_data(struct cdrom_device_info * cdi);
static int check_for_audio_disc(struct cdrom_device_info * cdi,
struct cdrom_device_ops * cdo);
@@ -333,14 +328,6 @@
static devfs_handle_t devfs_handle;
static struct unique_numspace cdrom_numspace = UNIQUE_NUMBERSPACE_INITIALISER;

-struct block_device_operations cdrom_fops =
-{
- open: cdrom_open,
- release: cdrom_release,
- ioctl: cdrom_ioctl,
- check_media_change: cdrom_media_changed,
-};
-
/* This macro makes sure we don't have to check on cdrom_device_ops
* existence in the run-time routines below. Change_capability is a
* hack to have the capability flags defined const, while we can still
@@ -354,7 +341,6 @@
int major = MAJOR(cdi->dev);
struct cdrom_device_ops *cdo = cdi->ops;
int *change_capability = (int *)&cdo->capability; /* hack */
- char vname[16];

cdinfo(CD_OPEN, "entering register_cdrom\n");

@@ -396,7 +382,6 @@
if (!devfs_handle)
devfs_handle = devfs_mk_dir (NULL, "cdroms", NULL);
cdi->number = devfs_alloc_unique_number (&cdrom_numspace);
- sprintf (vname, "cdrom%d", cdi->number);
if (cdi->de) {
int pos;
devfs_handle_t slave;
@@ -405,6 +390,8 @@
pos = devfs_generate_path (cdi->de, rname + 3,
sizeof rname - 3);
if (pos >= 0) {
+ char vname[16];
+ sprintf (vname, "cdrom%d", cdi->number);
strncpy (rname + pos, "../", 3);
devfs_mk_symlink (devfs_handle, vname,
DEVFS_FL_DEFAULT,
@@ -412,13 +399,6 @@
devfs_auto_unregister (cdi->de, slave);
}
}
- else {
- cdi->de =
- devfs_register (devfs_handle, vname, DEVFS_FL_DEFAULT,
- MAJOR (cdi->dev), MINOR (cdi->dev),
- S_IFBLK | S_IRUGO | S_IWUGO,
- &cdrom_fops, NULL);
- }
cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
cdi->next = topCdromPtr;
topCdromPtr = cdi;
@@ -475,7 +455,6 @@
* is in their own interest: device control becomes a lot easier
* this way.
*/
-static
int cdrom_open(struct inode *ip, struct file *fp)
{
struct cdrom_device_info *cdi;
@@ -669,7 +648,6 @@


/* Admittedly, the logic below could be performed in a nicer way. */
-static
int cdrom_release(struct inode *ip, struct file *fp)
{
kdev_t dev = ip->i_rdev;
@@ -867,7 +845,7 @@
return ret;
}

-static int cdrom_media_changed(kdev_t dev)
+int cdrom_media_changed(kdev_t dev)
{
struct cdrom_device_info *cdi = cdrom_find_device(dev);
/* This talks to the VFS, which doesn't like errors - just 1 or 0.
@@ -1480,7 +1458,7 @@
* these days. ATAPI / SCSI specific code now mainly resides in
* mmc_ioct().
*/
-static int cdrom_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
+int cdrom_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
unsigned long arg)
{
kdev_t dev = ip->i_rdev;
@@ -2391,7 +2369,10 @@
EXPORT_SYMBOL(cdrom_count_tracks);
EXPORT_SYMBOL(register_cdrom);
EXPORT_SYMBOL(unregister_cdrom);
-EXPORT_SYMBOL(cdrom_fops);
+EXPORT_SYMBOL(cdrom_open);
+EXPORT_SYMBOL(cdrom_release);
+EXPORT_SYMBOL(cdrom_ioctl);
+EXPORT_SYMBOL(cdrom_media_changed);
EXPORT_SYMBOL(cdrom_number_of_slots);
EXPORT_SYMBOL(cdrom_select_disc);
EXPORT_SYMBOL(cdrom_mode_select);
diff -urN S13-pre6/drivers/cdrom/cdu31a.c S13-pre6-current/drivers/cdrom/cdu31a.c
--- S13-pre6/drivers/cdrom/cdu31a.c Sun Sep 23 16:11:59 2001
+++ S13-pre6-current/drivers/cdrom/cdu31a.c Mon Oct 22 23:40:16 2001
@@ -3102,6 +3102,23 @@
return 0;
}

+static int scd_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int scd_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
/*
* Open the drive for operations. Spin the drive up and read the table of
* contents if these have not already been done.
@@ -3112,17 +3129,13 @@
unsigned int res_size;
unsigned char params[2];

- MOD_INC_USE_COUNT;
if (sony_usage == 0) {
- if (scd_spinup() != 0) {
- MOD_DEC_USE_COUNT;
+ if (scd_spinup() != 0)
return -EIO;
- }
sony_get_toc();
if (!sony_toc_read) {
do_sony_cd_cmd(SONY_SPIN_DOWN_CMD, NULL, 0,
res_reg, &res_size);
- MOD_DEC_USE_COUNT;
return -EIO;
}

@@ -3183,9 +3196,16 @@
sony_spun_up = 0;
}
sony_usage--;
- MOD_DEC_USE_COUNT;
}

+struct block_device_operations scd_bdops =
+{
+ open: scd_dev_open,
+ release: scd_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
+
static struct cdrom_device_ops scd_dops = {
open:scd_open,
release:scd_release,
@@ -3383,7 +3403,7 @@

request_region(cdu31a_port, 4, "cdu31a");

- if (devfs_register_blkdev(MAJOR_NR, "cdu31a", &cdrom_fops)) {
+ if (devfs_register_blkdev(MAJOR_NR, "cdu31a", &scd_bdops)) {
printk("Unable to get major %d for CDU-31a\n",
MAJOR_NR);
goto errout2;
@@ -3465,6 +3485,7 @@
if (register_cdrom(&scd_info)) {
goto errout0;
}
+ devfs_plain_cdrom(&scd_info, &scd_bdops);
}


diff -urN S13-pre6/drivers/cdrom/cm206.c S13-pre6-current/drivers/cdrom/cm206.c
--- S13-pre6/drivers/cdrom/cm206.c Sun Sep 23 16:11:59 2001
+++ S13-pre6-current/drivers/cdrom/cm206.c Mon Oct 22 23:44:34 2001
@@ -765,11 +765,35 @@
}
}

+static int cm206_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int cm206_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+struct block_device_operations cm206_bdops =
+{
+ open: cm206_dev_open,
+ release: cm206_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
+
/* The new open. The real opening strategy is defined in cdrom.c. */

static int cm206_open(struct cdrom_device_info *cdi, int purpose)
{
- MOD_INC_USE_COUNT;
if (!cd->openfiles) { /* reset only first time */
cd->background = 0;
reset_cm260();
@@ -792,7 +816,6 @@
FIRST_TRACK = 0; /* No valid disc status */
}
--cd->openfiles;
- MOD_DEC_USE_COUNT;
}

/* Empty buffer empties $sectors$ sectors of the adapter card buffer,
@@ -1478,7 +1501,7 @@
return -EIO;
}
printk(".\n");
- if (devfs_register_blkdev(MAJOR_NR, "cm206", &cdrom_fops) != 0) {
+ if (devfs_register_blkdev(MAJOR_NR, "cm206", &cm206_bdops) != 0) {
printk(KERN_INFO "Cannot register for major %d!\n",
MAJOR_NR);
cleanup(3);
@@ -1491,6 +1514,7 @@
cleanup(3);
return -EIO;
}
+ devfs_plain_cdrom(&cm206_info, &cm206_bdops);
blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), DEVICE_REQUEST);
blksize_size[MAJOR_NR] = cm206_blocksizes;
read_ahead[MAJOR_NR] = 16; /* reads ahead what? */
diff -urN S13-pre6/drivers/cdrom/mcd.c S13-pre6-current/drivers/cdrom/mcd.c
--- S13-pre6/drivers/cdrom/mcd.c Sun Sep 23 16:11:59 2001
+++ S13-pre6-current/drivers/cdrom/mcd.c Mon Oct 22 23:47:28 2001
@@ -189,6 +189,31 @@
void *arg);
int mcd_drive_status(struct cdrom_device_info *cdi, int slot_nr);

+static int mcd_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int mcd_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+struct block_device_operations mcd_bdops =
+{
+ open: mcd_dev_open,
+ release: mcd_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
+
static struct timer_list mcd_timer;

static struct cdrom_device_ops mcd_dops = {
@@ -978,8 +1003,6 @@
if (mcdPresent == 0)
return -ENXIO; /* no hardware */

- MOD_INC_USE_COUNT;
-
if (mcd_open_count || mcd_state != MCD_S_IDLE)
goto bump_count;

@@ -1002,7 +1025,6 @@
return 0;

err_out:
- MOD_DEC_USE_COUNT;
return -EIO;
}

@@ -1015,7 +1037,6 @@
if (!--mcd_open_count) {
mcd_invalidate_buffers();
}
- MOD_DEC_USE_COUNT;
}


@@ -1060,7 +1081,7 @@
return -EIO;
}

- if (devfs_register_blkdev(MAJOR_NR, "mcd", &cdrom_fops) != 0) {
+ if (devfs_register_blkdev(MAJOR_NR, "mcd", &mcd_bdops) != 0) {
printk(KERN_ERR "mcd: Unable to get major %d for Mitsumi CD-ROM\n", MAJOR_NR);
return -EIO;
}
@@ -1152,6 +1173,7 @@
cleanup(3);
return -EIO;
}
+ devfs_plain_cdrom(&mcd_info, &mcd_bdops);
printk(msg);

return 0;
diff -urN S13-pre6/drivers/cdrom/mcdx.c S13-pre6-current/drivers/cdrom/mcdx.c
--- S13-pre6/drivers/cdrom/mcdx.c Sun Sep 23 16:11:59 2001
+++ S13-pre6-current/drivers/cdrom/mcdx.c Tue Oct 23 05:31:44 2001
@@ -219,6 +219,31 @@
int mcdx_init(void);
void do_mcdx_request(request_queue_t * q);

+static int mcdx_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int mcdx_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+struct block_device_operations mcdx_bdops =
+{
+ open: mcdx_dev_open,
+ release: mcdx_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
+

/* Indirect exported functions. These functions are exported by their
addresses, such as mcdx_open and mcdx_close in the
@@ -640,13 +665,10 @@
/* Make the modules looking used ... (thanx bjorn).
* But we shouldn't forget to decrement the module counter
* on error return */
- MOD_INC_USE_COUNT;

/* this is only done to test if the drive talks with us */
- if (-1 == mcdx_getstatus(stuffp, 1)) {
- MOD_DEC_USE_COUNT;
+ if (-1 == mcdx_getstatus(stuffp, 1))
return -EIO;
- }

if (stuffp->xxx) {

@@ -705,10 +727,8 @@
}

xtrace(OPENCLOSE, "open() init irq generation\n");
- if (-1 == mcdx_config(stuffp, 1)) {
- MOD_DEC_USE_COUNT;
+ if (-1 == mcdx_config(stuffp, 1))
return -EIO;
- }
#if FALLBACK
/* Set the read speed */
xwarn("AAA %x AAA\n", stuffp->readcmd);
@@ -745,7 +765,7 @@
MODE2 :
MODE1,
1))) {
- /* MOD_DEC_USE_COUNT, return -EIO; */
+ /* return -EIO; */
stuffp->xa = 0;
break;
}
@@ -765,10 +785,8 @@
/* xa disks will be read in raw mode, others not */
if (-1 == mcdx_setdrivemode(stuffp,
stuffp->xa ? RAW : COOKED,
- 1)) {
- MOD_DEC_USE_COUNT;
+ 1))
return -EIO;
- }
if (stuffp->audio) {
xinfo("open() audio disk found\n");
} else if (stuffp->lastsector >= 0) {
@@ -792,8 +810,6 @@
stuffp = mcdx_stuffp[MINOR(cdi->dev)];

--stuffp->users;
-
- MOD_DEC_USE_COUNT;
}

static int mcdx_media_changed(struct cdrom_device_info *cdi, int disc_nr)
@@ -1180,7 +1196,7 @@
}

xtrace(INIT, "init() register blkdev\n");
- if (devfs_register_blkdev(MAJOR_NR, "mcdx", &cdrom_fops) != 0) {
+ if (devfs_register_blkdev(MAJOR_NR, "mcdx", &mcdx_bdops) != 0) {
xwarn("%s=0x%3p,%d: Init failed. Can't get major %d.\n",
MCDX, stuffp->wreg_data, stuffp->irq, MAJOR_NR);
kfree(stuffp);
@@ -1240,6 +1256,7 @@
blk_cleanup_queue(BLK_DEFAULT_QUEUE(MAJOR_NR));
return 2;
}
+ devfs_plain_cdrom(&mcdx_info, &mcdx_bdops);
printk(msg);
return 0;
}
diff -urN S13-pre6/drivers/cdrom/sbpcd.c S13-pre6-current/drivers/cdrom/sbpcd.c
--- S13-pre6/drivers/cdrom/sbpcd.c Sun Sep 23 16:11:59 2001
+++ S13-pre6-current/drivers/cdrom/sbpcd.c Tue Oct 23 05:34:45 2001
@@ -5418,6 +5418,31 @@
return (1);
}
/*==========================================================================*/
+
+static int sbpcd_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int sbpcd_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static struct block_device_operations sbpcd_bdops =
+{
+ open: sbpcd_dev_open,
+ release: sbpcd_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
/*==========================================================================*/
/*
* Open the device special file. Check that a disk is in. Read TOC.
@@ -5428,7 +5453,6 @@

i = MINOR(cdi->dev);

- MOD_INC_USE_COUNT;
down(&ioctl_read_sem);
switch_drive(i);

@@ -5495,7 +5519,6 @@
}
}
up(&ioctl_read_sem);
- MOD_DEC_USE_COUNT;
return ;
}
/*==========================================================================*/
@@ -5848,7 +5871,7 @@
OUT(MIXER_data,0xCC); /* one nibble per channel, max. value: 0xFF */
#endif /* SOUND_BASE */

- if (devfs_register_blkdev(MAJOR_NR, major_name, &cdrom_fops) != 0)
+ if (devfs_register_blkdev(MAJOR_NR, major_name, &sbpcd_bdops) != 0)
{
msg(DBG_INF, "Can't get MAJOR %d for Matsushita CDROM\n", MAJOR_NR);
#ifdef MODULE
@@ -5923,7 +5946,7 @@
sbpcd_infop->de =
devfs_register (devfs_handle, nbuff, DEVFS_FL_DEFAULT,
MAJOR_NR, j, S_IFBLK | S_IRUGO | S_IWUGO,
- &cdrom_fops, NULL);
+ &sbpcd_bdops, NULL);
if (register_cdrom(sbpcd_infop))
{
printk(" sbpcd: Unable to register with Uniform CD-ROm driver\n");
diff -urN S13-pre6/drivers/ide/ide-cd.c S13-pre6-current/drivers/ide/ide-cd.c
--- S13-pre6/drivers/ide/ide-cd.c Sun Oct 21 15:17:06 2001
+++ S13-pre6-current/drivers/ide/ide-cd.c Mon Oct 22 22:53:44 2001
@@ -2863,7 +2863,7 @@
struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg)
{
- return cdrom_fops.ioctl (inode, file, cmd, arg);
+ return cdrom_ioctl (inode, file, cmd, arg);
}

static
@@ -2875,7 +2875,7 @@
MOD_INC_USE_COUNT;
if (info->buffer == NULL)
info->buffer = (char *) kmalloc(SECTOR_BUFFER_SIZE, GFP_KERNEL);
- if ((info->buffer == NULL) || (rc = cdrom_fops.open(ip, fp))) {
+ if ((info->buffer == NULL) || (rc = cdrom_open(ip, fp))) {
drive->usage--;
MOD_DEC_USE_COUNT;
}
@@ -2886,14 +2886,14 @@
void ide_cdrom_release (struct inode *inode, struct file *file,
ide_drive_t *drive)
{
- cdrom_fops.release (inode, file);
+ cdrom_release (inode, file);
MOD_DEC_USE_COUNT;
}

static
int ide_cdrom_check_media_change (ide_drive_t *drive)
{
- return cdrom_fops.check_media_change(MKDEV (HWIF (drive)->major,
+ return cdrom_media_change(MKDEV (HWIF (drive)->major,
(drive->select.b.unit) << PARTN_BITS));
}

diff -urN S13-pre6/drivers/scsi/sr.c S13-pre6-current/drivers/scsi/sr.c
--- S13-pre6/drivers/scsi/sr.c Sun Oct 21 15:17:14 2001
+++ S13-pre6-current/drivers/scsi/sr.c Mon Oct 22 23:56:54 2001
@@ -484,6 +484,31 @@
return 1;
}

+static int sr_dev_open(struct inode *inode, struct file *file)
+{
+ int err;
+ MOD_INC_USE_COUNT;
+ err = cdrom_open(inode, file);
+ if (err)
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+static int sr_dev_release(struct inode *inode, struct file *file)
+{
+ int err = cdrom_release(inode, file);
+ MOD_DEC_USE_COUNT;
+ return err;
+}
+
+struct block_device_operations sr_bdops =
+{
+ open: sr_dev_open,
+ release: sr_dev_release,
+ ioctl: cdrom_ioctl,
+ check_media_change: cdrom_media_changed,
+};
+
static int sr_open(struct cdrom_device_info *cdi, int purpose)
{
check_disk_change(cdi->dev);
@@ -778,7 +803,7 @@
return 0;

if (!sr_registered) {
- if (devfs_register_blkdev(MAJOR_NR, "sr", &cdrom_fops)) {
+ if (devfs_register_blkdev(MAJOR_NR, "sr", &sr_bdops)) {
printk("Unable to get major %d for SCSI-CD\n", MAJOR_NR);
return 1;
}
@@ -875,7 +900,7 @@
devfs_register (scsi_CDs[i].device->de, "cd",
DEVFS_FL_DEFAULT, MAJOR_NR, i,
S_IFBLK | S_IRUGO | S_IWUGO,
- &cdrom_fops, NULL);
+ &sr_bdops, NULL);
register_cdrom(&scsi_CDs[i].cdi);
}

diff -urN S13-pre6/include/linux/cdrom.h S13-pre6-current/include/linux/cdrom.h
--- S13-pre6/include/linux/cdrom.h Fri Oct 19 23:06:18 2001
+++ S13-pre6-current/include/linux/cdrom.h Tue Oct 23 05:26:08 2001
@@ -777,10 +777,25 @@
};

/* the general block_device operations structure: */
-extern struct block_device_operations cdrom_fops;
+extern int cdrom_open(struct inode *, struct file *);
+extern int cdrom_release(struct inode *, struct file *);
+extern int cdrom_ioctl(struct inode *, struct file *, unsigned, unsigned long);
+extern int cdrom_media_changed(kdev_t);

extern int register_cdrom(struct cdrom_device_info *cdi);
extern int unregister_cdrom(struct cdrom_device_info *cdi);
+
+static inline void devfs_plain_cdrom(struct cdrom_device_info *cdi,
+ struct block_device_operations *ops)
+{
+ char vname[23];
+
+ sprintf (vname, "cdroms/cdrom%d", cdi->number);
+ cdi->de = devfs_register (NULL, vname, DEVFS_FL_DEFAULT,
+ MAJOR (cdi->dev), MINOR (cdi->dev),
+ S_IFBLK | S_IRUGO | S_IWUGO,
+ ops, NULL);
+}

typedef struct {
int data;


2001-10-23 10:57:40

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [CFT][PATCH] cdrom-related rmmod races

In article <[email protected]> you wrote:
> Folks, patch below should fix the rmmod/open() races for
> cdrom drivers (including pcd and sr). The problem with the current
> code is that they use cdrom_open() as ->open() and bump module
> refcount only in cdrom_device_info->open() callback. cdrom_open()
> blocks before calling it, with obvious results.

> Patch is pretty straightforward - it gives these guys ->open()
> of their own that does MOD_INC_USE_COUNT and calls cdrom_open() - see
> below for examples. Please, give it a try - if there's no complaints it
> goes to Linus.

Wouldn't it be easier to add a 'struct module *owner' to struct
block_device_operations?

Christoph

--
Of course it doesn't work. We've performed a software upgrade.

2001-10-23 11:14:12

by Alexander Viro

[permalink] [raw]
Subject: Re: [CFT][PATCH] cdrom-related rmmod races



On Tue, 23 Oct 2001, Christoph Hellwig wrote:

> Wouldn't it be easier to add a 'struct module *owner' to struct
> block_device_operations?

All of them share (in the current tree) block_device_operations -
cdrom_fops from drivers/cdrom/cdrom.c. What will you put as
->owner there?

2001-10-23 13:42:45

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [CFT][PATCH] cdrom-related rmmod races

On Tue, Oct 23, 2001 at 07:14:27AM -0400, Alexander Viro wrote:
>
>
> On Tue, 23 Oct 2001, Christoph Hellwig wrote:
>
> > Wouldn't it be easier to add a 'struct module *owner' to struct
> > block_device_operations?
>
> All of them share (in the current tree) block_device_operations -
> cdrom_fops from drivers/cdrom/cdrom.c. What will you put as
> ->owner there?

Of course they would have to have their own block_device_operations as
well - but they could continue to use the common open method.

Also this would allow to get rid of the MOD_{INC,DEC}_USE_COUNT in the
blockdrivers and thus make it easier for driver writers to create races.

Christoph

--
Of course it doesn't work. We've performed a software upgrade.