Re: [PATCH] Bsg referencing parent device
The idea looks pretty reasonable, but once that is done we can get rid of
the ->release callback entirely and just handle it in the callers.
Something like the untested patch below:
diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index fc2e5ff2c4b9..9419def8c017 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -303,11 +303,9 @@ static void bsg_exit_rq(struct request_queue *q, struct
request *req)
* @name: device to give bsg device
* @job_fn: bsg job handler
* @dd_job_size: size of LLD data needed for each job
- * @release: @dev release function
*/
struct request_queue *bsg_setup_queue(struct device *dev, const char *name,
- bsg_job_fn *job_fn, int dd_job_size,
- void (*release)(struct device *))
+ bsg_job_fn *job_fn, int dd_job_size)
{
struct request_queue *q;
int ret;
@@ -331,7 +329,7 @@ struct request_queue *bsg_setup_queue(struct device *dev,
const char *name,
blk_queue_softirq_done(q, bsg_softirq_done);
blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
- ret = bsg_register_queue(q, dev, name, _transport_ops, release);
+ ret = bsg_register_queue(q, dev, name, _transport_ops);
if (ret) {
printk(KERN_ERR "%s: bsg interface failed to "
"initialize - register queue\n", dev->kobj.name);
diff --git a/block/bsg.c b/block/bsg.c
index defa06c11858..fe1e5632e5d1 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(>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(_mutex);
bcd = idr_find(_minor_idr, iminor(inode));
- if (bcd)
- kref_get(>ref);
mutex_unlock(_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(>ref, bsg_kref_release_function);
return bd;
}
@@ -913,25 +896,17 @@ void bsg_unregister_queue(struct request_queue *q)
sysfs_remove_link(>kobj, "bsg");
device_unregister(bcd->class_dev);
bcd->class_dev = NULL;
- kref_put(>ref, bsg_kref_release_function);
mutex_unlock(_mutex);
}
EXPORT_SYMBOL_GPL(bsg_unregister_queue);
int bsg_register_queue(struct request_queue *q, struct device *parent,
- const char *name, const struct bsg_ops *ops,
- void (*release)(struct device *))
+ const char *name, const struct bsg_ops *ops)
{
struct bsg_class_device *bcd;
dev_t dev;
int ret;
struct device *class_dev = NULL;
- const char *devname;
-
- if (name)
- devname = name;
- else
- devname = dev_name(parent);
/*
* we need a proper transport to send commands, not a stacked device
@@ -955,15 +930,12 @@ int bsg_register_queue(struct request_queue *q, struct
device *parent,
bcd->minor = ret;
bcd->queue = q;
- bcd->parent = get_device(parent);
- bcd->release = release;
bcd->ops = ops;
- kref_init(>ref);
dev = MKDEV(bsg_major, bcd->minor);
- class_dev = device_create(bsg_class, parent, dev, NULL, "%s", devname);
+ class_dev = device_create(bsg_class, parent, dev, NULL, "%s", name);
if (IS_ERR(class_dev)) {
ret = PTR_ERR(class_dev);
- goto put_dev;
+ goto idr_remove;
}
bcd->class_dev = class_dev;
@@ -978,8 +950,7 @@ int bsg_register_queue(struct request_queue *q, struct
device *parent,
unregister_class_dev:
device_unregister(class_dev);
-put_dev:
- put_device(parent);
+idr_remove:
idr_remove(_minor_idr, bcd->minor);
unlock:
mutex_unlock(_mutex);
@@ -993,7 +964,7 @@ int bsg_scsi_register_queue(struct request_queue *q, struct
device *parent)
return -EINVAL;
}
- return bsg_register_queue(q, parent, NULL, _scsi_ops, NULL);
+ return bsg_register_queue(q, parent, dev_name(parent), _scsi_ops);
}
EXPORT_SYMBOL_GPL(bsg_scsi_register_queue);
diff --git
[PATCH] Bsg referencing parent device
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] [] scsi_host_dev_release+0x3f/0x130
[16834.647740,07] [] device_release+0x32/0xa0
[16834.653423,07] [] kobject_cleanup+0x77/0x190
[16834.659002,07] [] kobject_put+0x25/0x50
[16834.664430,07] [] put_device+0x17/0x20
[16834.669740,07] [] bsg_kref_release_function+0x24/0x30
[16834.675007,07] [] bsg_release+0x166/0x1d0
[16834.680148,07] [] __fput+0xcb/0x1d0
[16834.685156,07] [] fput+0xe/0x10
[16834.690017,07] [] task_work_run+0x86/0xb0
[16834.694781,07] [] exit_to_usermode_loop+0x6b/0x9a
[16834.699466,07] [] syscall_return_slowpath+0x55/0x60
[16834.704110,07] [] 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(>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(_mutex);
bcd = idr_find(_minor_idr, iminor(inode));
- if (bcd)
- kref_get(>ref);
mutex_unlock(_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(>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 = >bsg_dev;
if (!bcd->class_dev)
@@ -913,8 +898,21 @@ void bsg_unregister_queue(struct request_queue *q)
sysfs_remove_link(>kobj, "bsg");
device_unregister(bcd->class_dev);
bcd->class_dev = NULL;
- kref_put(>ref, bsg_kref_release_function);
+ parent = bcd->parent;
+ release = bcd->release;
+ bcd->parent = NULL;
+ bcd->release = NULL;
mutex_unlock(_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
