Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

mtd: allow to unload the mtdtrans module if its block devices aren't open

Now it once again possible to remove mtdtrans module.
You still need to ensure that block devices of that module aren't mounted.
This is due to the fact that as long as a block device is open, it still exists,
therefore if we were to allow module removal, this block device might became used again.

This time in addition to code review, I also made the code
pass some torture tests like module reload in a loop + read in a loop +
card insert/removal all at same time.

The blktrans_open/blktrans_release don't take the mtd table lock because:

While device is added (that includes execution of add_mtd_blktrans_dev)
the lock is already taken

Now suppose the device will never be removed. In this case even if we have changes
in mtd table, the entry that we need will stay exactly the same. (Note that we don't
look at table at all, just following private pointer of block device).

Now suppose that someone tries to remove the mtd device.
This will be propagated to trans driver which _ought_ to call del_mtd_blktrans_dev
which will take the per device lock, release the mtd device and set trans->mtd = NULL.
>From this point on, following opens won't even be able to know anything about that mtd device
(which at that point is likely not to exist)
Also the same care is taken not to trip over NULL mtd pointer in blktrans_dev_release.

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
Tested-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

authored by

Maxim Levitsky and committed by
David Woodhouse
008c751e ebd71e3a

+24 -28
+24 -28
drivers/mtd/mtd_blkdevs.c
··· 176 176 static int blktrans_open(struct block_device *bdev, fmode_t mode) 177 177 { 178 178 struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk); 179 - int ret; 179 + int ret = 0; 180 180 181 181 if (!dev) 182 182 return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/ ··· 184 184 lock_kernel(); 185 185 mutex_lock(&dev->lock); 186 186 187 - if (!dev->mtd) { 188 - ret = -ENXIO; 187 + if (dev->open++) 189 188 goto unlock; 189 + 190 + kref_get(&dev->ref); 191 + __module_get(dev->tr->owner); 192 + 193 + if (dev->mtd) { 194 + ret = dev->tr->open ? dev->tr->open(dev) : 0; 195 + __get_mtd_device(dev->mtd); 190 196 } 191 197 192 - ret = !dev->open++ && dev->tr->open ? dev->tr->open(dev) : 0; 193 - 194 - /* Take another reference on the device so it won't go away till 195 - last release */ 196 - if (!ret) 197 - kref_get(&dev->ref); 198 198 unlock: 199 199 mutex_unlock(&dev->lock); 200 200 blktrans_dev_put(dev); ··· 205 205 static int blktrans_release(struct gendisk *disk, fmode_t mode) 206 206 { 207 207 struct mtd_blktrans_dev *dev = blktrans_dev_get(disk); 208 - int ret = -ENXIO; 208 + int ret = 0; 209 209 210 210 if (!dev) 211 211 return ret; ··· 213 213 lock_kernel(); 214 214 mutex_lock(&dev->lock); 215 215 216 - /* Release one reference, we sure its not the last one here*/ 217 - kref_put(&dev->ref, blktrans_dev_release); 218 - 219 - if (!dev->mtd) 216 + if (--dev->open) 220 217 goto unlock; 221 218 222 - ret = !--dev->open && dev->tr->release ? dev->tr->release(dev) : 0; 219 + kref_put(&dev->ref, blktrans_dev_release); 220 + module_put(dev->tr->owner); 221 + 222 + if (dev->mtd) { 223 + ret = dev->tr->release ? dev->tr->release(dev) : 0; 224 + __put_mtd_device(dev->mtd); 225 + } 223 226 unlock: 224 227 mutex_unlock(&dev->lock); 225 228 blktrans_dev_put(dev); ··· 388 385 389 386 gd->queue = new->rq; 390 387 391 - __get_mtd_device(new->mtd); 392 - __module_get(tr->owner); 393 - 394 388 /* Create processing thread */ 395 389 /* TODO: workqueue ? */ 396 390 new->thread = kthread_run(mtd_blktrans_thread, new, ··· 410 410 } 411 411 return 0; 412 412 error4: 413 - module_put(tr->owner); 414 - __put_mtd_device(new->mtd); 415 413 blk_cleanup_queue(new->rq); 416 414 error3: 417 415 put_disk(new->disk); ··· 446 448 blk_start_queue(old->rq); 447 449 spin_unlock_irqrestore(&old->queue_lock, flags); 448 450 449 - /* Ask trans driver for release to the mtd device */ 451 + /* If the device is currently open, tell trans driver to close it, 452 + then put mtd device, and don't touch it again */ 450 453 mutex_lock(&old->lock); 451 - if (old->open && old->tr->release) { 452 - old->tr->release(old); 453 - old->open = 0; 454 + if (old->open) { 455 + if (old->tr->release) 456 + old->tr->release(old); 457 + __put_mtd_device(old->mtd); 454 458 } 455 459 456 - __put_mtd_device(old->mtd); 457 - module_put(old->tr->owner); 458 - 459 - /* At that point, we don't touch the mtd anymore */ 460 460 old->mtd = NULL; 461 461 462 462 mutex_unlock(&old->lock);