@@ -650,18 +650,6 @@ static struct bsg_device *bsg_alloc_device(void)
return bd;
}
-static void bsg_kref_release_function(struct kref *kref)
-{
- struct bsg_class_device *bcd =
- container_of(kref, struct bsg_class_device, ref);
- struct device *parent = bcd->parent;
-
- if (bcd->release)
- bcd->release(bcd->parent);
-
- put_device(parent);
-}
-
static int bsg_put_device(struct bsg_device *bd)
{
int ret = 0, do_free;
@@ -694,7 +682,6 @@ static int bsg_put_device(struct bsg_device *bd)
kfree(bd);
out:
- kref_put(&q->bsg_dev.ref, bsg_kref_release_function);
if (do_free)
blk_put_queue(q);
return ret;
@@ -760,8 +747,6 @@ static struct bsg_device *bsg_get_device(struct inode *inode, struct file *file)
*/
mutex_lock(&bsg_mutex);
bcd = idr_find(&bsg_minor_idr, iminor(inode));
- if (bcd)
- kref_get(&bcd->ref);
mutex_unlock(&bsg_mutex);
if (!bcd)
@@ -772,8 +757,6 @@ static struct bsg_device *bsg_get_device(struct inode *inode, struct file *file)
return bd;
bd = bsg_add_device(inode, bcd->queue, file);
- if (IS_ERR(bd))
- kref_put(&bcd->ref, bsg_kref_release_function);
return bd;
}
@@ -902,6 +885,8 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
void bsg_unregister_queue(struct request_queue *q)
{
+ struct device *parent;
+ void (*release)(struct device *dev);
struct bsg_class_device *bcd = &q->bsg_dev;
if (!bcd->class_dev)
@@ -913,8 +898,21 @@ void bsg_unregister_queue(struct request_queue *q)
sysfs_remove_link(&q->kobj, "bsg");
device_unregister(bcd->class_dev);
bcd->class_dev = NULL;
- kref_put(&bcd->ref, bsg_kref_release_function);
+ parent = bcd->parent;
+ release = bcd->release;
+ bcd->parent = NULL;
+ bcd->release = NULL;
mutex_unlock(&bsg_mutex);
+
+ /*
+ * The caller of bsg_[un]register_queue must hold a reference
+ * to the parent device between ..register.. and ..unregister..
+ * so we do not maintain a refcount to parent here.
+ * Note: the caller is expected to blk_cleanup_queue(q)
+ * before releasing the reference to the parent device.
+ */
+ if (release)
+ release(parent);
}
EXPORT_SYMBOL_GPL(bsg_unregister_queue);
@@ -955,10 +953,9 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
bcd->minor = ret;
bcd->queue = q;
- bcd->parent = get_device(parent);
+ bcd->parent = parent;
bcd->release = release;
bcd->ops = ops;
- kref_init(&bcd->ref);
dev = MKDEV(bsg_major, bcd->minor);
class_dev = device_create(bsg_class, parent, dev, NULL, "%s", devname);
if (IS_ERR(class_dev)) {
A follow-up on earlier discussions: [PATCH] bsg referencing bus driver module https://www.spinics.net/lists/linux-scsi/msg119631.html [PATCH] Waiting for scsi_host_template release https://www.spinics.net/lists/linux-scsi/msg119432.html All these discussions are attempts to fix a crash after SCSI transport driver unload if a user-mode process holds a handle in BSG layer towards the unloaded driver via SCSI mid-layer: [16834.636216,07] Call Trace: ... scsi_proc_hostdir_rm [16834.641944,07] [<ffffffff8141723f>] scsi_host_dev_release+0x3f/0x130 [16834.647740,07] [<ffffffff813e4f82>] device_release+0x32/0xa0 [16834.653423,07] [<ffffffff812dc6c7>] kobject_cleanup+0x77/0x190 [16834.659002,07] [<ffffffff812dc585>] kobject_put+0x25/0x50 [16834.664430,07] [<ffffffff813e5277>] put_device+0x17/0x20 [16834.669740,07] [<ffffffff812d0334>] bsg_kref_release_function+0x24/0x30 [16834.675007,07] [<ffffffff812d14a6>] bsg_release+0x166/0x1d0 [16834.680148,07] [<ffffffff8119ba2b>] __fput+0xcb/0x1d0 [16834.685156,07] [<ffffffff8119bb6e>] ____fput+0xe/0x10 [16834.690017,07] [<ffffffff81077476>] task_work_run+0x86/0xb0 [16834.694781,07] [<ffffffff81057043>] exit_to_usermode_loop+0x6b/0x9a [16834.699466,07] [<ffffffff81002875>] syscall_return_slowpath+0x55/0x60 [16834.704110,07] [<ffffffff8172d615>] int_ret_from_sys_call+0x25/0x9f The latest input from earlier discussions was to cut off access to the unloaded driver at bsg_unregister_queue time by calling blk_cleanup_queue. If we do that we still have to release the reference to the parent device (otherwise we crash with the same stack). The next logical step is, rather than maintaining a "part-time" reference to be dropped early, we discard referencing completely. Discarding the reference turns out to be the only thing needed to fix the problem: all transport drivers already do blk_cleanup_queue before releasing their reference to the parent device. From 7eaa9b43f0b99766b1d197044eb4d2e549d11a24 Mon Sep 17 00:00:00 2001 From: Anatoliy Glagolev <glagolig@gmail.com> Date: Wed, 16 May 2018 17:03:09 -0600 Subject: [PATCH] Bsg referencing parent device Signed-off-by: Anatoliy Glagolev <glagolig@gmail.com> Bsg holding reference to a parent device may result in a crash if user closes a bsg file handle after the parent device driver has unloaded. Holding a reference is not really needed: parent device must exist between bsg_register_queue and bsg_unregister_queue. Before the device goes away the caller does blk_cleanup_queue so that all in-flight requests to the device are gone and all new requests cannot pass beyond the queue. The queue itself is a refcounted object and it will stay alive with a bsg file. --- block/bsg.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)