percpu_ref, RCU: Audit RCU usages in percpu_ref users

rvt_mregion uses percpu_ref for reference counting and RCU to protect accesses from lkey_table. When a rvt_mregion needs to be freed, it first gets unregistered from lkey_table and then rvt_check_refs() is called to wait for in-flight usages before the rvt_mregion is freed.

rvt_check_refs() seems to have a couple issues.

* It has a fast exit path which tests percpu_ref_is_zero(). However, a percpu_ref reading zero doesn't mean that the object can be released. In fact, the ->release() callback might not even have started executing yet. Proceeding with freeing can lead to use-after-free.

* lkey_table is RCU protected but there is no RCU grace period in the free path. percpu_ref uses RCU internally but it's sched-RCU whose grace periods are different from regular RCU. Also, it generally isn't a good idea to depend on internal behaviors like this.

To address the above issues, this patch removes the the fast exit and

adds an explicit synchronize_rcu().

Signed-off-by Tejun Heo <[email protected]>

Cc Dennis Dalessandro <[email protected]>

Cc Mike Marciniszyn <[email protected]>

Cc [email protected]

Cc Linus Torvalds <[email protected]>

---
Hello, Dennis, Mike.

I don't know RDMA at all and this patch is only compile tested. Can you please take a careful look?

hmm_devmem_find() requires rcu_read_lock_held() but there's nothing which actually uses the RCU protection. The only caller is hmm_devmem_pages_create() which already grabs the mutex and does superflous rcu_read_lock/unlock() around the function.

This doesn't add anything and just adds to confusion. Remove the RCU protection and open-code the radix tree lookup. If this needs to become more sophisticated in the future, let's add them back when necessary.

Signed-off-by Tejun Heo <[email protected]>

Cc [email protected]

Cc Linus Torvalds <[email protected]>

---
Hello, Jérôme.

This came up while auditing percpu_ref users for missing explicit RCU grace periods. HMM doesn't seem to depend on RCU protection at all, so I thought it'd be better to remove it for now. It's only compile tested.

Thanks.

 mm/hmm.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/hmm.c +2 -10

index 320545b98..d4627c5 100644
@@ -845,13 +845,6 @@ static void hmm_devmem_release(struct device *dev, void *data)
 	hmm_devmem_radix_release(resource);
 }
 
-static struct hmm_devmem *hmm_devmem_find(resource_size_t phys)
-{
-	WARN_ON_ONCE(!rcu_read_lock_held());
-
-	return radix_tree_lookup(&hmm_devmem_radix, phys >> PA_SECTION_SHIFT);
-}
-
 static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 {
 	resource_size_t key, align_start, align_size, align_end;

@@ -892,9 +885,8 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 	for (key = align_start; key <= align_end; key += PA_SECTION_SIZE) {
 		struct hmm_devmem *dup;
 
-		rcu_read_lock();
-		dup = hmm_devmem_find(key);
-		rcu_read_unlock();
+		dup = radix_tree_lookup(&hmm_devmem_radix,
+					key >> PA_SECTION_SHIFT);
 		if (dup) {
 			dev_err(device, "%s: collides with mapping for %s\n",
 				__func__, dev_name(dup->device));

Signature · 1 line

2.9.5

3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work reliably") added rcu_read_[un]lock_sched() to blk_queue_enter() along with other changes but it doesn't seem to be doing anything.

blk_queue_enter() is called with @q - the pointer to the target request_queue, so the caller obviously has to guarantee that @q can be dereferenced, and inside the RCU-sched protected area, there's nothing which needs further protection.

Let's remove the superflous rcu_read_[un]lock_sched().

[ ... ]

This came up while auditing percpu_ref users for problematic RCU usages. I couldn't understand what the RCU protection was doing. It'd be great if you can take a look and tell me whether I missed something.

I think the rcu_read_lock() and rcu_read_unlock() are really necessary in this code. In the LWN article "The RCU-barrier menagerie" it is explained that RCU can be used to enforce write ordering globally if the code that reads the writes

that are ordered with an RCU read lock (https://lwn.net/Articles/573497/). See also the following comment in scsi_device_quiesce():

/*
* Ensure that the effect of blk_set_preempt_only() will be visible
* for percpu_ref_tryget() callers that occur after the queue
* unfreeze even if the queue was already frozen before this function
* was called. See also https://lwn.net/Articles/573497/.
*/

Since this patch introduces a subtle and hard to debug race condition, please drop this patch.

Quoted 10 lines

that are ordered with an RCU read lock (https://lwn.net/Articles/573497/). See also the following comment in scsi_device_quiesce():

/*
* Ensure that the effect of blk_set_preempt_only() will be visible
* for percpu_ref_tryget() callers that occur after the queue
* unfreeze even if the queue was already frozen before this function
* was called. See also https://lwn.net/Articles/573497/.
*/

Quoted 2 lines

Since this patch introduces a subtle and hard to debug race condition, please drop this patch.

Hah, the pairing is between scsi_device_quiesce() and blk_queue_enter()? But that doesn't make sense either because scsi_device_quiesce() is doing regular synchronize_rcu() and blk_queue_enter() is doing rcu_read_lock_sched(). They don't interlock with each other in any way.

So, the right thing to do here would be somehow moving the RCU synchronization into blk_set_preempt_only() and switching to regular RCU protection in blk_queue_enter(). The code as-is isn't really doing anything.

Can you clarify this further? From <linux/rcupdate.h>:

static inline void synchronize_rcu(void)

{
	synchronize_sched();
}

Quoted 6 lines · Bart Van Assche

Can you clarify this further? From <linux/rcupdate.h>:

static inline void synchronize_rcu(void)

{
	synchronize_sched();
}

You'll find that is for !CONFIG_PREEMPT_RCU.

This patch introduces rcu_work, a workqueue work variant which gets executed after a RCU grace period, and converts the open coded bouncing in fs/aio and kernel/cgroup.

- can we split this patch up, so that if somebody bisects a problem
to it, we'll see if it's cgroup or aio that triggers it?

So I'd like to either just make the thing always just use WORK_CPU_UNBOUND, or hear some kind of (handwaving ok) explanation for why something else would ever make sense. If the action is fundamentally delayed by RCU, why would it make a difference which CPU it runs on?

+/**
+ * queue_rcu_work_on - queue work on specific CPU after a RCU grace period
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @rwork: work to queue

+ *
+ * Return: %false if @work was already on a queue, %true otherwise.
+ */

kernel/workqueue.c +16 -0

+bool queue_rcu_work_on(int cpu, struct workqueue_struct *wq,
+                      struct rcu_work *rwork)
+{
+       struct work_struct *work = &rwork->work;
+
+       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+               rwork->wq = wq;
+               rwork->cpu = cpu;
+               call_rcu(&rwork->rcu, rcu_work_rcufn);
+               return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(queue_rcu_work_on);
+

Quoted 5 lines · Tejun Heo

+/**
+ * queue_rcu_work_on - queue work on specific CPU after a RCU grace period
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @rwork: work to queue

For many people, "RCU grace period" is clear enough, but not ALL.

So please make it a little more clear that it just queues work after a *Normal* RCU grace period. it supports only one RCU variant.

Quoted 3 lines

+ *
+ * Return: %false if @work was already on a queue, %true otherwise.
+ */

I'm afraid this will be a hard-using API.

The user can't find a plan B when it returns false, especially when the user expects the work must be called at least once again after an RCU grace period.

And the error-prone part of it is that, like other queue_work() functions, the return value of it is often ignored and makes the problem worse.

So, since workqueue.c provides this API, it should handle this problem. For example, by calling call_rcu() again in this case, but everything will be much more complex: a synchronization is needed for "calling call_rcu() again" and allowing the work item called twice after the last queue_rcu_work() is not workqueue style.

Some would argue that the delayed_work has the same problem when the user expects the work must be called at least once again after a period of time. But time interval is easy to detect, the user can check the time and call the queue_delayed_work() again when needed which is also a frequent design pattern. And for rcu, it is hard to use this design pattern since it is hard to detect (new) rcu grace period without using call_rcu().

I would not provide this API. it is not a NACK. I'm just trying expressing my thinking about the API. I'd rather RCU be changed and RCU callbacks are changed to be sleepable. But a complete overhaul cleanup on the whole source tree for compatibility is needed at first, an even more complex job.

Quoted 16 lines

kernel/workqueue.c +16 -0

+bool queue_rcu_work_on(int cpu, struct workqueue_struct *wq,
+                      struct rcu_work *rwork)
+{
+       struct work_struct *work = &rwork->work;
+
+       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+               rwork->wq = wq;
+               rwork->cpu = cpu;
+               call_rcu(&rwork->rcu, rcu_work_rcufn);
+               return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(queue_rcu_work_on);
+

One downside of allowing RCU callback functions to sleep is that one poorly written callback can block a bunch of other ones.

Quoted 2 lines · Paul E. McKenney

One downside of allowing RCU callback functions to sleep is that one poorly written callback can block a bunch of other ones.

Not to mention that we really want the RCU callbacks to be simple and short. Needing fancy things in the callback really should be the exception not the rule.

Quoted 24 lines · Lai Jiangshan

Quoted 5 lines · Tejun Heo

+/**
+ * queue_rcu_work_on - queue work on specific CPU after a RCU grace period
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @rwork: work to queue

For many people, "RCU grace period" is clear enough, but not ALL.

So please make it a little more clear that it just queues work after a *Normal* RCU grace period. it supports only one RCU variant.

Quoted 3 lines

+ *
+ * Return: %false if @work was already on a queue, %true otherwise.
+ */

I'm afraid this will be a hard-using API.

The user can't find a plan B when it returns false, especially when the user expects the work must be called at least once again after an RCU grace period.

Quoted 8 lines

And the error-prone part of it is that, like other queue_work() functions, the return value of it is often ignored and makes the problem worse.

So, since workqueue.c provides this API, it should handle this problem. For example, by calling call_rcu() again in this case, but everything will be much more complex: a synchronization is needed for "calling call_rcu() again" and allowing the work item called twice after the last queue_rcu_work() is not workqueue style.

I confess that I had not thought of this aspect, but doesn't the rcu_barrier() in v2 of this patchset guarantee that it has passed the RCU portion of the overall wait time? Given that, what I am missing is now this differs from flush_work() on a normal work item.

So could you please show me the sequence of events that leads to this problem? (Again, against v2 of this patch, which replaces the synchronize_rcu() with rcu_barrier().)

Quoted 9 lines

Some would argue that the delayed_work has the same problem when the user expects the work must be called at least once again after a period of time. But time interval is easy to detect, the user can check the time and call the queue_delayed_work() again when needed which is also a frequent design pattern. And for rcu, it is hard to use this design pattern since it is hard to detect (new) rcu grace period without using call_rcu().

Quoted 5 lines

I would not provide this API. it is not a NACK. I'm just trying expressing my thinking about the API. I'd rather RCU be changed and RCU callbacks are changed to be sleepable. But a complete overhaul cleanup on the whole source tree for compatibility is needed at first, an even more complex job.

One downside of allowing RCU callback functions to sleep is that one poorly written callback can block a bunch of other ones. One advantage of Tejun's approach is that such delays only affect the workqueues, which are already designed to handle such delays.

Thanx, Paul

Quoted 17 lines

Quoted 16 lines

kernel/workqueue.c +16 -0

+bool queue_rcu_work_on(int cpu, struct workqueue_struct *wq,
+                      struct rcu_work *rwork)
+{
+       struct work_struct *work = &rwork->work;
+
+       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+               rwork->wq = wq;
+               rwork->cpu = cpu;
+               call_rcu(&rwork->rcu, rcu_work_rcufn);
+               return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(queue_rcu_work_on);
+

Quoted 34 lines · Paul E. McKenney

Quoted 24 lines · Lai Jiangshan

Quoted 5 lines · Tejun Heo

+/**
+ * queue_rcu_work_on - queue work on specific CPU after a RCU grace period
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @rwork: work to queue

For many people, "RCU grace period" is clear enough, but not ALL.

So please make it a little more clear that it just queues work after a *Normal* RCU grace period. it supports only one RCU variant.

Quoted 3 lines

+ *
+ * Return: %false if @work was already on a queue, %true otherwise.
+ */

I'm afraid this will be a hard-using API.

The user can't find a plan B when it returns false, especially when the user expects the work must be called at least once again after an RCU grace period.

Quoted 8 lines

And the error-prone part of it is that, like other queue_work() functions, the return value of it is often ignored and makes the problem worse.

So, since workqueue.c provides this API, it should handle this problem. For example, by calling call_rcu() again in this case, but everything will be much more complex: a synchronization is needed for "calling call_rcu() again" and allowing the work item called twice after the last queue_rcu_work() is not workqueue style.

Quoted 8 lines

I confess that I had not thought of this aspect, but doesn't the rcu_barrier() in v2 of this patchset guarantee that it has passed the RCU portion of the overall wait time? Given that, what I am missing is now this differs from flush_work() on a normal work item.

So could you please show me the sequence of events that leads to this problem? (Again, against v2 of this patch, which replaces the synchronize_rcu() with rcu_barrier().)

I mentioned a subtle use case that user would think it is supported since the comment doesn't disallow it.

It is clear that the user expects
the work must be called at least once after the API returns

the work must be called after an RCU grace period

But in the case when the user expects the work must be called at least once again after "queue_rcu_work() + an RCU grace period", the API is not competent to it if the work is queued. Although the user can detect it by the return value of queue_rcu_work(), the user hardly makes his expectation happen by adding appropriate code.

Quoted 39 lines

Quoted 8 lines

Some would argue that the delayed_work has the same problem when the user expects the work must be called at least once again after a period of time. But time interval is easy to detect, the user can check the time and call the queue_delayed_work() again when needed which is also a frequent design pattern. And for rcu, it is hard to use this design pattern since it is hard to detect (new) rcu grace period without using call_rcu().

Quoted 5 lines

I would not provide this API. it is not a NACK. I'm just trying expressing my thinking about the API. I'd rather RCU be changed and RCU callbacks are changed to be sleepable. But a complete overhaul cleanup on the whole source tree for compatibility is needed at first, an even more complex job.

One downside of allowing RCU callback functions to sleep is that one poorly written callback can block a bunch of other ones. One advantage of Tejun's approach is that such delays only affect the workqueues, which are already designed to handle such delays.

Thanx, Paul

Quoted 17 lines

Quoted 16 lines

kernel/workqueue.c +16 -0

+bool queue_rcu_work_on(int cpu, struct workqueue_struct *wq,
+                      struct rcu_work *rwork)
+{
+       struct work_struct *work = &rwork->work;
+
+       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+               rwork->wq = wq;
+               rwork->cpu = cpu;
+               call_rcu(&rwork->rcu, rcu_work_rcufn);
+               return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(queue_rcu_work_on);
+

I mentioned a subtle use case that user would think it is supported since the comment doesn't disallow it.

It is clear that the user expects
the work must be called at least once after the API returns the work must be called after an RCU grace period

But in the case when the user expects the work must be called at least once again after "queue_rcu_work() + an RCU grace period", the API is not competent to it if the work is queued. Although the user can detect it by the return value of queue_rcu_work(), the user hardly makes his expectation happen by adding appropriate code.