[RFC 0/4] use rcu_read_lock() during module list walk

[RFC 0/4] use rcu_read_lock() during module list walk

[Sebastian Andrzej Siewior]

Hi Peter,

this series was made before I noticed that you introduced a RB tree for
lookup but the old way still remains under !CONFIG_MODULES_TREE_LOOKUP.
In the old way the caller had preempt_disable() while invoking
list_for_each_safe_rcu() which is (according to the RCU checklist) not a
substitute for rcu_readlock().
With your CONFIG_MODULES_TREE_LOOKUP I fail to understand what blocks
free_module() until all mod_find() callers have dropped their refrence to
the obtained struct mod. We had synchronize_sched() in RCU case.

Sebastian

[RFC 1/4] module: use rcu_read_lock() while walking over a RCU protected list

[Sebastian Andrzej Siewior]

The `modules' list uses list_for_each_entry_rcu() itarator.
rcu_readlock() protects here against against module removal invoked from
another CPU while preempt_disable() does not.

I don't understand what syncs in CONFIG_MODULES_TREE_LOOKUP against
module removal. In the other (RCU) case there is synchronize_sched().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 4d2b82e610e2..a62d2d48cd63 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3979,9 +3979,9 @@ bool is_module_address(unsigned long addr)
 {
 	bool ret;
 
-	preempt_disable();
+	rcu_read_lock();
 	ret = __module_address(addr) != NULL;
-	preempt_enable();
+	rcu_read_unlock();
 
 	return ret;
 }
@@ -3990,7 +3990,7 @@ bool is_module_address(unsigned long addr)
  * __module_address - get the module which contains an address.
  * @addr: the address.
  *
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called with RCU read or module mutex held so that
  * module doesn't get freed during this.
  */
 struct module *__module_address(unsigned long addr)
@@ -4024,9 +4024,9 @@ bool is_module_text_address(unsigned long addr)
 {
 	bool ret;
 
-	preempt_disable();
+	rcu_read_lock();
 	ret = __module_text_address(addr) != NULL;
-	preempt_enable();
+	rcu_read_unlock();
 
 	return ret;
 }
@@ -4035,7 +4035,7 @@ bool is_module_text_address(unsigned long addr)
  * __module_text_address - get the module whose code contains an address.
  * @addr: the address.
  *
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called with RCU read lock or module mutex held so that
  * module doesn't get freed during this.
  */
 struct module *__module_text_address(unsigned long addr)
-- 
2.4.6

Re: [RFC 1/4] module: use rcu_read_lock() while walking over a RCU protected list

[Peter Zijlstra]

On Fri, Jul 31, 2015 at 07:08:05PM +0200, Sebastian Andrzej Siewior wrote:
> The `modules' list uses list_for_each_entry_rcu() itarator.
> rcu_readlock() protects here against against module removal invoked from
> another CPU while preempt_disable() does not.

Not so, it does in fact.

> I don't understand what syncs in CONFIG_MODULES_TREE_LOOKUP against
> module removal. In the other (RCU) case there is synchronize_sched().

Its using rcu-sched aka preempt_disable.

if you want to use rcu primitives, use rcu_read_lock_sched(), but that
is in fact identical to preempt_disable().

Your patch breaks things.

[RFC 2/4] jump_label: use rcu_read_lock() while accessing __module_*()

[Sebastian Andrzej Siewior]

__jump_label_mod_text_reserved() should most likely use rcu_read_lock()
while invoking __module_text_address(). I'm not sure there is anything
to protect the module pointed by `start' and `end' from removal.
I *think* the protection is based on the preempt_disable() in
check_kprobe_address_safe(). That preempt_disable however does not
protect from module removal on a SMP machine.

Same goes for jump_label_update(). I think it is safe here to avoid the
read lock since the caller is the module so it can't go away at the same
time. However it might get a problem if the key reference here is shared
across modules for some reason with no module dependency.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/jump_label.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 52ebaca1b9fc..8aa9e0fce92e 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -231,16 +231,21 @@ struct static_key_mod {
 static int __jump_label_mod_text_reserved(void *start, void *end)
 {
 	struct module *mod;
+	int ret = 0;
 
+	rcu_read_lock();
 	mod = __module_text_address((unsigned long)start);
 	if (!mod)
-		return 0;
+		goto out;
 
 	WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
 
-	return __jump_label_text_reserved(mod->jump_entries,
+	ret = __jump_label_text_reserved(mod->jump_entries,
 				mod->jump_entries + mod->num_jump_entries,
 				start, end);
+out:
+	rcu_read_unlock();
+	return ret;
 }
 
 static void __jump_label_mod_update(struct static_key *key, int enable)
@@ -448,11 +453,11 @@ static void jump_label_update(struct static_key *key, int enable)
 
 	__jump_label_mod_update(key, enable);
 
-	preempt_disable();
+	rcu_read_lock();
 	mod = __module_address((unsigned long)key);
 	if (mod)
 		stop = mod->jump_entries + mod->num_jump_entries;
-	preempt_enable();
+	rcu_read_unlock();
 #endif
 	/* if there are no users, entry can be NULL */
 	if (entry)
-- 
2.4.6

Re: [RFC 2/4] jump_label: use rcu_read_lock() while accessing __module_*()

[Peter Zijlstra]

On Fri, Jul 31, 2015 at 07:08:06PM +0200, Sebastian Andrzej Siewior wrote:
> __jump_label_mod_text_reserved() should most likely use rcu_read_lock()
> while invoking __module_text_address(). I'm not sure there is anything
> to protect the module pointed by `start' and `end' from removal.
> I *think* the protection is based on the preempt_disable() in
> check_kprobe_address_safe(). That preempt_disable however does not
> protect from module removal on a SMP machine.

Yes it does, seeing how it uses sync_sched().

[RFC 3/4] kprobes: Add a RCU lock while invoking __module_text_address()

[Sebastian Andrzej Siewior]

delete_module() first sets the module's state to MODULE_STATE_GOING
which means further try_module_get() invocations will fail. However
without a RCU readlock there is nothing that ensures that the pointer
returned by __module_text_address() is still valid in try_module_get().
The preempt_disable() does not protect here against module removal from
another CPU.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kprobes.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417bb963..a2d4e5164d7d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1449,6 +1449,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	}
 
 	/* Check if are we probing a module */
+	rcu_read_lock();
 	*probed_mod = __module_text_address((unsigned long) p->addr);
 	if (*probed_mod) {
 		/*
@@ -1457,7 +1458,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 		 */
 		if (unlikely(!try_module_get(*probed_mod))) {
 			ret = -ENOENT;
-			goto out;
+			goto out_rcu_unlock;
 		}
 
 		/*
@@ -1471,6 +1472,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
 			ret = -ENOENT;
 		}
 	}
+out_rcu_unlock:
+	rcu_read_unlock();
 out:
 	preempt_enable();
 	jump_label_unlock();
-- 
2.4.6

[RFC 4/4] kprobe: remove preempt_disable() from check_kprobe_address_safe()

[Sebastian Andrzej Siewior]

It should be safe to remove preempt_disable() here. It was introduced in
commit a189d0350f387 ("kprobes: disable preempt for module_text_address()
and kernel_text_address()") to protect against module removal. This is
however wrong (now) because the RCU list which is used requires a RCU
readlock. The comment in __module_text_address() has been updated and the
caller, too.
I *think* this preempt_disable() may go now.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kprobes.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a2d4e5164d7d..92d50ea83564 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1438,7 +1438,6 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	if (ret)
 		return ret;
 	jump_label_lock();
-	preempt_disable();
 
 	/* Ensure it is not in reserved area nor out of text */
 	if (!kernel_text_address((unsigned long) p->addr) ||
@@ -1475,7 +1474,6 @@ static int check_kprobe_address_safe(struct kprobe *p,
 out_rcu_unlock:
 	rcu_read_unlock();
 out:
-	preempt_enable();
 	jump_label_unlock();
 
 	return ret;
-- 
2.4.6

Re: [RFC 0/4] use rcu_read_lock() during module list walk

[Peter Zijlstra]

On Fri, Jul 31, 2015 at 07:08:04PM +0200, Sebastian Andrzej Siewior wrote:
> Hi Peter,
> 
> this series was made before I noticed that you introduced a RB tree for
> lookup but the old way still remains under !CONFIG_MODULES_TREE_LOOKUP.

> In the old way the caller had preempt_disable() while invoking
> list_for_each_safe_rcu() which is (according to the RCU checklist) not a
> substitute for rcu_readlock().

This is true. preempt_disable() != rcu_read_lock().

> With your CONFIG_MODULES_TREE_LOOKUP I fail to understand what blocks
> free_module() until all mod_find() callers have dropped their refrence to
> the obtained struct mod. We had synchronize_sched() in RCU case.

We still have synchronize_sched(), but that is not RCU, that is
RCU-sched, and rcu_read_lock_sched() == preempt_disable() (+- some
debugging bits).

The code used to be broken in this regard, see 0be964be0d45 ("module:
Sanitize RCU usage and locking"), that fixed things to be consistent.