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 <glago...@gmail.com>
Date: Wed, 16 May 2018 17:03:09 -0600
Subject: [PATCH] Bsg referencing parent device
Signed-off-by: Anatoliy Glagolev <glago...@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(-)
diff --git a/block/bsg.c b/block/bsg.c
index defa06c..f9e4b91 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -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)) {
--
1.9.1
