diff mbox series

[v2] blk-mq, elevator: pair up elevator_get and elevator_put to avoid refcnt problems

Message ID 20221019132053.3597239-1-nickyc975@zju.edu.cn
State New
Headers
Series [v2] blk-mq, elevator: pair up elevator_get and elevator_put to avoid refcnt problems |

Commit Message

Jinlong Chen Oct. 19, 2022, 1:20 p.m. UTC 
  The current reference management logic of io scheduler modules contains
refcnt problems. For example, blk_mq_init_sched may fail before or after
the calling of e->ops.init_sched. If it fails before the calling, it does
nothing to the reference to the io scheduler module. But if it fails after
the calling, it releases the reference by calling kobject_put(&eq->kobj).

As the callers of blk_mq_init_sched can't know exactly where the failure
happens, they can't handle the reference to the io scheduler module
properly: releasing the reference on failure results in double-release if
blk_mq_init_sched has released it, and not releasing the reference results
in ghost reference if blk_mq_init_sched did not release it either.

The same problem also exists in io schedulers' init_sched implementations.

We can address the problem by adding releasing statements to the error
handling procedures of blk_mq_init_sched and init_sched implementations.
But that is counterintuitive and requires modifications to existing io
schedulers.

Instead, We make elevator_alloc get the io scheduler module references
that will be released by elevator_release. And then, we match each
elevator_get with an elevator_put. Therefore, each reference to an io
scheduler module explicitly has its own getter and releaser, and we no
longer need to worry about the refcnt problems.

The bugs and the patch can be validated with tools here:
https://github.com/nickyc975/linux_elv_refcnt_bug.git

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
Changes in v2:
 - reword the commit message for clarity

 block/blk-mq.c   |  6 ++++++
 block/elevator.c | 29 ++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

Comments

Christoph Hellwig Oct. 19, 2022, 3:04 p.m. UTC | #1 
I think this looks reasonable, but really needs to be split up into
a few self-contained patches.  I've started looking into that, give
me a bit of time to send it together with some extra cleanups.  I'll
make sure to preserve your attribution.
22021009@zju.edu.cn Oct. 19, 2022, 4:27 p.m. UTC | #2 
>I think this looks reasonable, but really needs to be split up into
>a few self-contained patches.  I've started looking into that, give
>me a bit of time to send it together with some extra cleanups.  I'll
>make sure to preserve your attribution.

Thanks, Christoph!

I am new to kernel development. There is definitely a bunch of things I need to learn. :)

Jinlong Chen
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8070b6c10e8d..2adfd52786dc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4595,6 +4595,12 @@  static void blk_mq_elv_switch_back(struct list_head *head,
 
 	mutex_lock(&q->sysfs_lock);
 	elevator_switch(q, t);
+	/**
+	 * We got a reference of the io scheduler module in blk_mq_elv_switch_none
+	 * to prevent the module from being removed. We need to put that reference
+	 * back once we are done.
+	 */
+	module_put(t->elevator_owner);
 	mutex_unlock(&q->sysfs_lock);
 }
 
diff --git a/block/elevator.c b/block/elevator.c
index bd71f0fc4e4b..aaafd415f922 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -166,9 +166,12 @@  struct elevator_queue *elevator_alloc(struct request_queue *q,
 {
 	struct elevator_queue *eq;
 
+	if (!try_module_get(e->elevator_owner))
+		goto err_out;
+
 	eq = kzalloc_node(sizeof(*eq), GFP_KERNEL, q->node);
 	if (unlikely(!eq))
-		return NULL;
+		goto err_put_elevator;
 
 	eq->type = e;
 	kobject_init(&eq->kobj, &elv_ktype);
@@ -176,6 +179,11 @@  struct elevator_queue *elevator_alloc(struct request_queue *q,
 	hash_init(eq->hash);
 
 	return eq;
+
+err_put_elevator:
+	elevator_put(e);
+err_out:
+	return NULL;
 }
 EXPORT_SYMBOL(elevator_alloc);
 
@@ -713,8 +721,9 @@  void elevator_init_mq(struct request_queue *q)
 	if (err) {
 		pr_warn("\"%s\" elevator initialization failed, "
 			"falling back to \"none\"\n", e->elevator_name);
-		elevator_put(e);
 	}
+
+	elevator_put(e);
 }
 
 /*
@@ -747,6 +756,7 @@  static int __elevator_change(struct request_queue *q, const char *name)
 {
 	char elevator_name[ELV_NAME_MAX];
 	struct elevator_type *e;
+	int ret = 0;
 
 	/* Make sure queue is not in the middle of being removed */
 	if (!blk_queue_registered(q))
@@ -762,17 +772,18 @@  static int __elevator_change(struct request_queue *q, const char *name)
 	}
 
 	strlcpy(elevator_name, name, sizeof(elevator_name));
-	e = elevator_get(q, strstrip(elevator_name), true);
-	if (!e)
-		return -EINVAL;
-
 	if (q->elevator &&
-	    elevator_match(q->elevator->type, elevator_name, 0)) {
-		elevator_put(e);
+	    elevator_match(q->elevator->type, strstrip(elevator_name), 0)) {
 		return 0;
 	}
 
-	return elevator_switch(q, e);
+	e = elevator_get(q, strstrip(elevator_name), true);
+	if (!e)
+		return -EINVAL;
+
+	ret = elevator_switch(q, e);
+	elevator_put(e);
+	return ret;
 }
 
 ssize_t elv_iosched_store(struct request_queue *q, const char *name,