[PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

Hi,

Here is a couple of patches which fix suspicious RCU
usage warnings in kprobes.

Anders reported the first warning in kprobe smoke test
with CONFIG_PROVE_RCU_LIST=y. While fixing this issue,
I found similar issues and cleanups in kprobes.

Thank you,

---

Masami Hiramatsu (2):
      kprobes: Suppress the suspicious RCU warning on kprobes
      kprobes: Use non RCU traversal APIs on kprobe_tables if possible


 kernel/kprobes.c |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH -tip V2 1/2] kprobes: Suppress the suspicious RCU warning on kprobes

[Masami Hiramatsu]

Anders reported that the lockdep warns that suspicious
RCU list usage in register_kprobe() (detected by
CONFIG_PROVE_RCU_LIST.) This is because get_kprobe()
access kprobe_table[] by hlist_for_each_entry_rcu()
without rcu_read_lock.

If we call get_kprobe() from the breakpoint handler context,
it is run with preempt disabled, so this is not a problem.
But in other cases, instead of rcu_read_lock(), we locks
kprobe_mutex so that the kprobe_table[] is not updated.
So, current code is safe, but still not good from the view
point of RCU.

Joel suggested that we can silent that warning by passing
lockdep_is_held() to the last argument of
hlist_for_each_entry_rcu().

Add lockdep_is_held(&kprobe_mutex) at the end of the
hlist_for_each_entry_rcu() to suppress the warning.

Reported-by: Anders Roxell <anders.roxell@linaro.org>
Suggested-by: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/kprobes.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 53534aa258a6..f9ecb6d532fb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -326,7 +326,8 @@ struct kprobe *get_kprobe(void *addr)
 	struct kprobe *p;
 
 	head = &kprobe_table[hash_ptr(addr, KPROBE_HASH_BITS)];
-	hlist_for_each_entry_rcu(p, head, hlist) {
+	hlist_for_each_entry_rcu(p, head, hlist,
+				 lockdep_is_held(&kprobe_mutex)) {
 		if (p->addr == addr)
 			return p;
 	}

[PATCH -tip V2 2/2] kprobes: Use non RCU traversal APIs on kprobe_tables if possible

[Masami Hiramatsu]

Current kprobes uses RCU traversal APIs on kprobe_tables
even if it is safe because kprobe_mutex is locked.

Make those traversals to non-RCU APIs where the kprobe_mutex
is locked.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f9ecb6d532fb..4caab01ace30 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -46,6 +46,11 @@
 
 
 static int kprobes_initialized;
+/* kprobe_table can be accessed by
+ * - Normal hlist traversal and RCU add/del under kprobe_mutex is held.
+ * Or
+ * - RCU hlist traversal under disabling preempt (breakpoint handlers)
+ */
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
@@ -829,7 +834,7 @@ static void optimize_all_kprobes(void)
 	kprobes_allow_optimization = true;
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
-		hlist_for_each_entry_rcu(p, head, hlist)
+		hlist_for_each_entry(p, head, hlist)
 			if (!kprobe_disabled(p))
 				optimize_kprobe(p);
 	}
@@ -856,7 +861,7 @@ static void unoptimize_all_kprobes(void)
 	kprobes_allow_optimization = false;
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
-		hlist_for_each_entry_rcu(p, head, hlist) {
+		hlist_for_each_entry(p, head, hlist) {
 			if (!kprobe_disabled(p))
 				unoptimize_kprobe(p, false);
 		}
@@ -1479,12 +1484,14 @@ static struct kprobe *__get_valid_kprobe(struct kprobe *p)
 {
 	struct kprobe *ap, *list_p;
 
+	lockdep_assert_held(&kprobe_mutex);
+
 	ap = get_kprobe(p->addr);
 	if (unlikely(!ap))
 		return NULL;
 
 	if (p != ap) {
-		list_for_each_entry_rcu(list_p, &ap->list, list)
+		list_for_each_entry(list_p, &ap->list, list)
 			if (list_p == p)
 			/* kprobe p is a valid probe */
 				goto valid;
@@ -1649,7 +1656,9 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 {
 	struct kprobe *kp;
 
-	list_for_each_entry_rcu(kp, &ap->list, list)
+	lockdep_assert_held(&kprobe_mutex);
+
+	list_for_each_entry(kp, &ap->list, list)
 		if (!kprobe_disabled(kp))
 			/*
 			 * There is an active probe on the list.
@@ -1728,7 +1737,7 @@ static int __unregister_kprobe_top(struct kprobe *p)
 	else {
 		/* If disabling probe has special handlers, update aggrprobe */
 		if (p->post_handler && !kprobe_gone(p)) {
-			list_for_each_entry_rcu(list_p, &ap->list, list) {
+			list_for_each_entry(list_p, &ap->list, list) {
 				if ((list_p != p) && (list_p->post_handler))
 					goto noclean;
 			}
@@ -2042,13 +2051,15 @@ static void kill_kprobe(struct kprobe *p)
 {
 	struct kprobe *kp;
 
+	lockdep_assert_held(&kprobe_mutex);
+
 	p->flags |= KPROBE_FLAG_GONE;
 	if (kprobe_aggrprobe(p)) {
 		/*
 		 * If this is an aggr_kprobe, we have to list all the
 		 * chained probes and mark them GONE.
 		 */
-		list_for_each_entry_rcu(kp, &p->list, list)
+		list_for_each_entry(kp, &p->list, list)
 			kp->flags |= KPROBE_FLAG_GONE;
 		p->post_handler = NULL;
 		kill_optimized_kprobe(p);
@@ -2217,7 +2228,7 @@ static int kprobes_module_callback(struct notifier_block *nb,
 	mutex_lock(&kprobe_mutex);
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
-		hlist_for_each_entry_rcu(p, head, hlist)
+		hlist_for_each_entry(p, head, hlist)
 			if (within_module_init((unsigned long)p->addr, mod) ||
 			    (checkcore &&
 			     within_module_core((unsigned long)p->addr, mod))) {
@@ -2468,7 +2479,7 @@ static int arm_all_kprobes(void)
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
 		/* Arm all kprobes on a best-effort basis */
-		hlist_for_each_entry_rcu(p, head, hlist) {
+		hlist_for_each_entry(p, head, hlist) {
 			if (!kprobe_disabled(p)) {
 				err = arm_kprobe(p);
 				if (err)  {
@@ -2511,7 +2522,7 @@ static int disarm_all_kprobes(void)
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
 		/* Disarm all kprobes on a best-effort basis */
-		hlist_for_each_entry_rcu(p, head, hlist) {
+		hlist_for_each_entry(p, head, hlist) {
 			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
 				err = disarm_kprobe(p, false);
 				if (err) {

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

Hi Ingo,

Could you pick this series to fix the false-positive RCU-list warnings?

Thank you,

On Tue,  3 Dec 2019 15:06:06 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> Here is a couple of patches which fix suspicious RCU
> usage warnings in kprobes.
> 
> Anders reported the first warning in kprobe smoke test
> with CONFIG_PROVE_RCU_LIST=y. While fixing this issue,
> I found similar issues and cleanups in kprobes.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (2):
>       kprobes: Suppress the suspicious RCU warning on kprobes
>       kprobes: Use non RCU traversal APIs on kprobe_tables if possible
> 
> 
>  kernel/kprobes.c |   32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

Hello,

Anyone have any comment on this series?
Without this series, I still see the suspicious RCU warning for kprobe on -tip tree.

Thank you,

On Sat, 21 Dec 2019 03:55:41 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Ingo,
> 
> Could you pick this series to fix the false-positive RCU-list warnings?
> 
> Thank you,
> 
> On Tue,  3 Dec 2019 15:06:06 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi,
> > 
> > Here is a couple of patches which fix suspicious RCU
> > usage warnings in kprobes.
> > 
> > Anders reported the first warning in kprobe smoke test
> > with CONFIG_PROVE_RCU_LIST=y. While fixing this issue,
> > I found similar issues and cleanups in kprobes.
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (2):
> >       kprobes: Suppress the suspicious RCU warning on kprobes
> >       kprobes: Use non RCU traversal APIs on kprobe_tables if possible
> > 
> > 
> >  kernel/kprobes.c |   32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> > 
> > --
> > Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Joel Fernandes]

On Tue, Jan 07, 2020 at 09:15:35PM +0900, Masami Hiramatsu wrote:
> Hello,
> 
> Anyone have any comment on this series?
> Without this series, I still see the suspicious RCU warning for kprobe on -tip tree.

+Paul since RCU.

Hi Masami,

I believe I had commented before that I don't agree with this patch:
https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/

The rationale you used is to replace RCU-api with non-RCU api just to avoid
warnings. I think a better approach is to use RCU api and pass the optional
expression to silence the false-positive warnings by informing the RCU API
about the fact that locks are held (similar to what we do for
rcu_dereference_protected()). The RCU API will do additional checking
(such as making sure preemption is disabled for safe RCU usage etc) as well.

Could you repost the latest versions?

thanks,

 - Joel


> 
> Thank you,
> 
> On Sat, 21 Dec 2019 03:55:41 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi Ingo,
> > 
> > Could you pick this series to fix the false-positive RCU-list warnings?
> > 
> > Thank you,
> > 
> > On Tue,  3 Dec 2019 15:06:06 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Hi,
> > > 
> > > Here is a couple of patches which fix suspicious RCU
> > > usage warnings in kprobes.
> > > 
> > > Anders reported the first warning in kprobe smoke test
> > > with CONFIG_PROVE_RCU_LIST=y. While fixing this issue,
> > > I found similar issues and cleanups in kprobes.
> > > 
> > > Thank you,
> > > 
> > > ---
> > > 
> > > Masami Hiramatsu (2):
> > >       kprobes: Suppress the suspicious RCU warning on kprobes
> > >       kprobes: Use non RCU traversal APIs on kprobe_tables if possible
> > > 
> > > 
> > >  kernel/kprobes.c |   32 ++++++++++++++++++++++----------
> > >  1 file changed, 22 insertions(+), 10 deletions(-)
> > > 
> > > --
> > > Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

Hi Joel and Paul,

On Fri, 10 Jan 2020 16:14:38 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Tue, Jan 07, 2020 at 09:15:35PM +0900, Masami Hiramatsu wrote:
> > Hello,
> > 
> > Anyone have any comment on this series?
> > Without this series, I still see the suspicious RCU warning for kprobe on -tip tree.
> 
> +Paul since RCU.
> 
> Hi Masami,
> 
> I believe I had commented before that I don't agree with this patch:
> https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> 
> The rationale you used is to replace RCU-api with non-RCU api just to avoid
> warnings. I think a better approach is to use RCU api and pass the optional
> expression to silence the false-positive warnings by informing the RCU API
> about the fact that locks are held (similar to what we do for
> rcu_dereference_protected()). The RCU API will do additional checking
> (such as making sure preemption is disabled for safe RCU usage etc) as well.

Yes, that is what I did in [1/2] for get_kprobe().
Let me clarify the RCU list usage in [2/2].

With the careful check, other list traversals never be done in non-sleepable
context, those are always runs with kprobe_mutex held.
If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
non-RCU api for those cases, or do I miss something?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Joel Fernandes]

On Sat, Jan 11, 2020 at 08:35:07AM +0900, Masami Hiramatsu wrote:
> Hi Joel and Paul,
> 
> On Fri, 10 Jan 2020 16:14:38 -0500
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > On Tue, Jan 07, 2020 at 09:15:35PM +0900, Masami Hiramatsu wrote:
> > > Hello,
> > > 
> > > Anyone have any comment on this series?
> > > Without this series, I still see the suspicious RCU warning for kprobe on -tip tree.
> > 
> > +Paul since RCU.
> > 
> > Hi Masami,
> > 
> > I believe I had commented before that I don't agree with this patch:
> > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > 
> > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > warnings. I think a better approach is to use RCU api and pass the optional
> > expression to silence the false-positive warnings by informing the RCU API
> > about the fact that locks are held (similar to what we do for
> > rcu_dereference_protected()). The RCU API will do additional checking
> > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> 
> Yes, that is what I did in [1/2] for get_kprobe().
> Let me clarify the RCU list usage in [2/2].
> 
> With the careful check, other list traversals never be done in non-sleepable
> context, those are always runs with kprobe_mutex held.
> If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> non-RCU api for those cases, or do I miss something?

Yes, that is fine. However personally I prefer not to mix usage of
list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
(kprobe_table). I think it is more confusing and error prone. Just use
list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
expression, instead of calling lockdep_assert_held() independently. Is this
not doable?

thanks,

 - Joel

> Thank you,
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

On Sat, 11 Jan 2020 21:05:37 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Sat, Jan 11, 2020 at 08:35:07AM +0900, Masami Hiramatsu wrote:
> > Hi Joel and Paul,
> > 
> > On Fri, 10 Jan 2020 16:14:38 -0500
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> > 
> > > On Tue, Jan 07, 2020 at 09:15:35PM +0900, Masami Hiramatsu wrote:
> > > > Hello,
> > > > 
> > > > Anyone have any comment on this series?
> > > > Without this series, I still see the suspicious RCU warning for kprobe on -tip tree.
> > > 
> > > +Paul since RCU.
> > > 
> > > Hi Masami,
> > > 
> > > I believe I had commented before that I don't agree with this patch:
> > > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > > 
> > > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > > warnings. I think a better approach is to use RCU api and pass the optional
> > > expression to silence the false-positive warnings by informing the RCU API
> > > about the fact that locks are held (similar to what we do for
> > > rcu_dereference_protected()). The RCU API will do additional checking
> > > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> > 
> > Yes, that is what I did in [1/2] for get_kprobe().
> > Let me clarify the RCU list usage in [2/2].
> > 
> > With the careful check, other list traversals never be done in non-sleepable
> > context, those are always runs with kprobe_mutex held.
> > If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> > non-RCU api for those cases, or do I miss something?
> 
> Yes, that is fine. However personally I prefer not to mix usage of
> list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
> (kprobe_table). I think it is more confusing and error prone. Just use
> list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
> expression, instead of calling lockdep_assert_held() independently. Is this
> not doable?

Hmm, but isn't it more confusing that user just take a mutex but
no rcu_read_lock() with list_for_each_entry_rcu()? In that case,
sometimes it might sleep inside list_for_each_entry_rcu(), I thought
that might be more confusing mind model for users...

Anyway, if so, please update Documentation/RCU/listRCU.rst too.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

Hi Joel,

On Mon, 13 Jan 2020 12:16:40 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > 
> > > > Hi Masami,
> > > > 
> > > > I believe I had commented before that I don't agree with this patch:
> > > > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > > > 
> > > > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > > > warnings. I think a better approach is to use RCU api and pass the optional
> > > > expression to silence the false-positive warnings by informing the RCU API
> > > > about the fact that locks are held (similar to what we do for
> > > > rcu_dereference_protected()). The RCU API will do additional checking
> > > > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> > > 
> > > Yes, that is what I did in [1/2] for get_kprobe().
> > > Let me clarify the RCU list usage in [2/2].
> > > 
> > > With the careful check, other list traversals never be done in non-sleepable
> > > context, those are always runs with kprobe_mutex held.
> > > If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> > > non-RCU api for those cases, or do I miss something?
> > 
> > Yes, that is fine. However personally I prefer not to mix usage of
> > list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
> > (kprobe_table). I think it is more confusing and error prone. Just use
> > list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
> > expression, instead of calling lockdep_assert_held() independently. Is this
> > not doable?
> 
> Hmm, but isn't it more confusing that user just take a mutex but
> no rcu_read_lock() with list_for_each_entry_rcu()? In that case,
> sometimes it might sleep inside list_for_each_entry_rcu(), I thought
> that might be more confusing mind model for users...

I meant, do we always need to do something like below?

{
	mutex_lock(&lock);
	list_for_each_entry_rcu(list, ..., lockdep_is_held(&lock)) {
		...
	}
	mutex_unlock(&lock);
}

BTW, I found another problem on this policy, since we don't have
list_for_each_*_safe() equivalents for RCU, we can not do a safe
loop on it. Should we call a find function for each time?

Thank you, 

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Paul E. McKenney]

On Mon, Jan 13, 2020 at 10:09:53PM +0900, Masami Hiramatsu wrote:
> Hi Joel,
> 
> On Mon, 13 Jan 2020 12:16:40 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > > 
> > > > > Hi Masami,
> > > > > 
> > > > > I believe I had commented before that I don't agree with this patch:
> > > > > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > > > > 
> > > > > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > > > > warnings. I think a better approach is to use RCU api and pass the optional
> > > > > expression to silence the false-positive warnings by informing the RCU API
> > > > > about the fact that locks are held (similar to what we do for
> > > > > rcu_dereference_protected()). The RCU API will do additional checking
> > > > > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> > > > 
> > > > Yes, that is what I did in [1/2] for get_kprobe().
> > > > Let me clarify the RCU list usage in [2/2].
> > > > 
> > > > With the careful check, other list traversals never be done in non-sleepable
> > > > context, those are always runs with kprobe_mutex held.
> > > > If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> > > > non-RCU api for those cases, or do I miss something?
> > > 
> > > Yes, that is fine. However personally I prefer not to mix usage of
> > > list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
> > > (kprobe_table). I think it is more confusing and error prone. Just use
> > > list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
> > > expression, instead of calling lockdep_assert_held() independently. Is this
> > > not doable?
> > 
> > Hmm, but isn't it more confusing that user just take a mutex but
> > no rcu_read_lock() with list_for_each_entry_rcu()? In that case,
> > sometimes it might sleep inside list_for_each_entry_rcu(), I thought
> > that might be more confusing mind model for users...

The correct answer will be different in different situations.
For example, code that might be called either with the mutex held or
within an RCU read-side critical section will definitely need the _rcu()
and the lockdep_is_held().  Code that looks OK to call from within
RCU readers, but must not be (e.g., because it sleeps), will just as
definitely need to avoid _rcu().  (If the lack of _rcu() proves confusing,
maybe list_for_each_entry() needs to grow an optional lockdep expression?)

I am therefore personally OK with either approach, though in confusing
cases a comment might help.

> I meant, do we always need to do something like below?
> 
> {
> 	mutex_lock(&lock);
> 	list_for_each_entry_rcu(list, ..., lockdep_is_held(&lock)) {
> 		...
> 	}
> 	mutex_unlock(&lock);
> }
> 
> BTW, I found another problem on this policy, since we don't have
> list_for_each_*_safe() equivalents for RCU, we can not do a safe
> loop on it. Should we call a find function for each time?

Good point.

RCU readers don't need _safe() because RCU grace periods provide this
for free within RCU read-side critical sections.

So agreed, if you need _safe() on the update side, you would need to
call list_for_each_entry_safe().  If this proves confusing due to RCU
readers, maybe it should grow a lockdep expression?  In the meantime,
lockdep_assert_held() could be used if needed to let people know that
this should not be used in an RCU reader.

Does that work, or am I missing part of the problem?

							Thanx, Paul

Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Masami Hiramatsu]

On Mon, 13 Jan 2020 11:23:31 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Mon, Jan 13, 2020 at 10:09:53PM +0900, Masami Hiramatsu wrote:
> > Hi Joel,
> > 
> > On Mon, 13 Jan 2020 12:16:40 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > > > 
> > > > > > Hi Masami,
> > > > > > 
> > > > > > I believe I had commented before that I don't agree with this patch:
> > > > > > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > > > > > 
> > > > > > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > > > > > warnings. I think a better approach is to use RCU api and pass the optional
> > > > > > expression to silence the false-positive warnings by informing the RCU API
> > > > > > about the fact that locks are held (similar to what we do for
> > > > > > rcu_dereference_protected()). The RCU API will do additional checking
> > > > > > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> > > > > 
> > > > > Yes, that is what I did in [1/2] for get_kprobe().
> > > > > Let me clarify the RCU list usage in [2/2].
> > > > > 
> > > > > With the careful check, other list traversals never be done in non-sleepable
> > > > > context, those are always runs with kprobe_mutex held.
> > > > > If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> > > > > non-RCU api for those cases, or do I miss something?
> > > > 
> > > > Yes, that is fine. However personally I prefer not to mix usage of
> > > > list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
> > > > (kprobe_table). I think it is more confusing and error prone. Just use
> > > > list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
> > > > expression, instead of calling lockdep_assert_held() independently. Is this
> > > > not doable?
> > > 
> > > Hmm, but isn't it more confusing that user just take a mutex but
> > > no rcu_read_lock() with list_for_each_entry_rcu()? In that case,
> > > sometimes it might sleep inside list_for_each_entry_rcu(), I thought
> > > that might be more confusing mind model for users...
> 
> The correct answer will be different in different situations.
> For example, code that might be called either with the mutex held or
> within an RCU read-side critical section will definitely need the _rcu()
> and the lockdep_is_held().  Code that looks OK to call from within
> RCU readers, but must not be (e.g., because it sleeps), will just as
> definitely need to avoid _rcu().

I see. So the patch [2/2] is just removing useless rcu_read_lock()
and use non RCU api for kprobe_table, because those code never be
called from rcu read-side critical section. (It makes a critical section
only for using RCU list operation)

>  (If the lack of _rcu() proves confusing,
> maybe list_for_each_entry() needs to grow an optional lockdep expression?)

That is OK for me, anyway the [2/2] also introduces some lockdep_assert_held()
instead of rcu_read_lock() so that lockdep can check sanity.

> 
> I am therefore personally OK with either approach, though in confusing
> cases a comment might help.
> 
> > I meant, do we always need to do something like below?
> > 
> > {
> > 	mutex_lock(&lock);
> > 	list_for_each_entry_rcu(list, ..., lockdep_is_held(&lock)) {
> > 		...
> > 	}
> > 	mutex_unlock(&lock);
> > }
> > 
> > BTW, I found another problem on this policy, since we don't have
> > list_for_each_*_safe() equivalents for RCU, we can not do a safe
> > loop on it. Should we call a find function for each time?
> 
> Good point.
> 
> RCU readers don't need _safe() because RCU grace periods provide this
> for free within RCU read-side critical sections.
> 
> So agreed, if you need _safe() on the update side, you would need to
> call list_for_each_entry_safe().  If this proves confusing due to RCU
> readers, maybe it should grow a lockdep expression?  In the meantime,
> lockdep_assert_held() could be used if needed to let people know that
> this should not be used in an RCU reader.

I think lockdep_assert_held() is enough.

> 
> Does that work, or am I missing part of the problem?
> 
> 							Thanx, Paul

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip V2 2/2] kprobes: Use non RCU traversal APIs on kprobe_tables if possible

[Joel Fernandes]

On Tue, Dec 03, 2019 at 03:06:28PM +0900, Masami Hiramatsu wrote:
> Current kprobes uses RCU traversal APIs on kprobe_tables
> even if it is safe because kprobe_mutex is locked.
> 
> Make those traversals to non-RCU APIs where the kprobe_mutex
> is locked.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

May be resend both patch with appropriate tags since it has been some time
since originally posted?

thanks,

 - Joel


> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c |   29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index f9ecb6d532fb..4caab01ace30 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -46,6 +46,11 @@
>  
>  
>  static int kprobes_initialized;
> +/* kprobe_table can be accessed by
> + * - Normal hlist traversal and RCU add/del under kprobe_mutex is held.
> + * Or
> + * - RCU hlist traversal under disabling preempt (breakpoint handlers)
> + */
>  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
>  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
>  
> @@ -829,7 +834,7 @@ static void optimize_all_kprobes(void)
>  	kprobes_allow_optimization = true;
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
> -		hlist_for_each_entry_rcu(p, head, hlist)
> +		hlist_for_each_entry(p, head, hlist)
>  			if (!kprobe_disabled(p))
>  				optimize_kprobe(p);
>  	}
> @@ -856,7 +861,7 @@ static void unoptimize_all_kprobes(void)
>  	kprobes_allow_optimization = false;
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
> -		hlist_for_each_entry_rcu(p, head, hlist) {
> +		hlist_for_each_entry(p, head, hlist) {
>  			if (!kprobe_disabled(p))
>  				unoptimize_kprobe(p, false);
>  		}
> @@ -1479,12 +1484,14 @@ static struct kprobe *__get_valid_kprobe(struct kprobe *p)
>  {
>  	struct kprobe *ap, *list_p;
>  
> +	lockdep_assert_held(&kprobe_mutex);
> +
>  	ap = get_kprobe(p->addr);
>  	if (unlikely(!ap))
>  		return NULL;
>  
>  	if (p != ap) {
> -		list_for_each_entry_rcu(list_p, &ap->list, list)
> +		list_for_each_entry(list_p, &ap->list, list)
>  			if (list_p == p)
>  			/* kprobe p is a valid probe */
>  				goto valid;
> @@ -1649,7 +1656,9 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
>  {
>  	struct kprobe *kp;
>  
> -	list_for_each_entry_rcu(kp, &ap->list, list)
> +	lockdep_assert_held(&kprobe_mutex);
> +
> +	list_for_each_entry(kp, &ap->list, list)
>  		if (!kprobe_disabled(kp))
>  			/*
>  			 * There is an active probe on the list.
> @@ -1728,7 +1737,7 @@ static int __unregister_kprobe_top(struct kprobe *p)
>  	else {
>  		/* If disabling probe has special handlers, update aggrprobe */
>  		if (p->post_handler && !kprobe_gone(p)) {
> -			list_for_each_entry_rcu(list_p, &ap->list, list) {
> +			list_for_each_entry(list_p, &ap->list, list) {
>  				if ((list_p != p) && (list_p->post_handler))
>  					goto noclean;
>  			}
> @@ -2042,13 +2051,15 @@ static void kill_kprobe(struct kprobe *p)
>  {
>  	struct kprobe *kp;
>  
> +	lockdep_assert_held(&kprobe_mutex);
> +
>  	p->flags |= KPROBE_FLAG_GONE;
>  	if (kprobe_aggrprobe(p)) {
>  		/*
>  		 * If this is an aggr_kprobe, we have to list all the
>  		 * chained probes and mark them GONE.
>  		 */
> -		list_for_each_entry_rcu(kp, &p->list, list)
> +		list_for_each_entry(kp, &p->list, list)
>  			kp->flags |= KPROBE_FLAG_GONE;
>  		p->post_handler = NULL;
>  		kill_optimized_kprobe(p);
> @@ -2217,7 +2228,7 @@ static int kprobes_module_callback(struct notifier_block *nb,
>  	mutex_lock(&kprobe_mutex);
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
> -		hlist_for_each_entry_rcu(p, head, hlist)
> +		hlist_for_each_entry(p, head, hlist)
>  			if (within_module_init((unsigned long)p->addr, mod) ||
>  			    (checkcore &&
>  			     within_module_core((unsigned long)p->addr, mod))) {
> @@ -2468,7 +2479,7 @@ static int arm_all_kprobes(void)
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
>  		/* Arm all kprobes on a best-effort basis */
> -		hlist_for_each_entry_rcu(p, head, hlist) {
> +		hlist_for_each_entry(p, head, hlist) {
>  			if (!kprobe_disabled(p)) {
>  				err = arm_kprobe(p);
>  				if (err)  {
> @@ -2511,7 +2522,7 @@ static int disarm_all_kprobes(void)
>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>  		head = &kprobe_table[i];
>  		/* Disarm all kprobes on a best-effort basis */
> -		hlist_for_each_entry_rcu(p, head, hlist) {
> +		hlist_for_each_entry(p, head, hlist) {
>  			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
>  				err = disarm_kprobe(p, false);
>  				if (err) {
> 

Re: [PATCH -tip V2 2/2] kprobes: Use non RCU traversal APIs on kprobe_tables if possible

[Masami Hiramatsu]

On Tue, 14 Jan 2020 08:56:21 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Tue, Dec 03, 2019 at 03:06:28PM +0900, Masami Hiramatsu wrote:
> > Current kprobes uses RCU traversal APIs on kprobe_tables
> > even if it is safe because kprobe_mutex is locked.
> > 
> > Make those traversals to non-RCU APIs where the kprobe_mutex
> > is locked.
> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks Joel!
> 
> May be resend both patch with appropriate tags since it has been some time
> since originally posted?

OK, I'll update it on the latest -tip with your reviewed-by.

Thank you,

> 
> thanks,
> 
>  - Joel
> 
> 
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/kprobes.c |   29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index f9ecb6d532fb..4caab01ace30 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -46,6 +46,11 @@
> >  
> >  
> >  static int kprobes_initialized;
> > +/* kprobe_table can be accessed by
> > + * - Normal hlist traversal and RCU add/del under kprobe_mutex is held.
> > + * Or
> > + * - RCU hlist traversal under disabling preempt (breakpoint handlers)
> > + */
> >  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> >  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> >  
> > @@ -829,7 +834,7 @@ static void optimize_all_kprobes(void)
> >  	kprobes_allow_optimization = true;
> >  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >  		head = &kprobe_table[i];
> > -		hlist_for_each_entry_rcu(p, head, hlist)
> > +		hlist_for_each_entry(p, head, hlist)
> >  			if (!kprobe_disabled(p))
> >  				optimize_kprobe(p);
> >  	}
> > @@ -856,7 +861,7 @@ static void unoptimize_all_kprobes(void)
> >  	kprobes_allow_optimization = false;
> >  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >  		head = &kprobe_table[i];
> > -		hlist_for_each_entry_rcu(p, head, hlist) {
> > +		hlist_for_each_entry(p, head, hlist) {
> >  			if (!kprobe_disabled(p))
> >  				unoptimize_kprobe(p, false);
> >  		}
> > @@ -1479,12 +1484,14 @@ static struct kprobe *__get_valid_kprobe(struct kprobe *p)
> >  {
> >  	struct kprobe *ap, *list_p;
> >  
> > +	lockdep_assert_held(&kprobe_mutex);
> > +
> >  	ap = get_kprobe(p->addr);
> >  	if (unlikely(!ap))
> >  		return NULL;
> >  
> >  	if (p != ap) {
> > -		list_for_each_entry_rcu(list_p, &ap->list, list)
> > +		list_for_each_entry(list_p, &ap->list, list)
> >  			if (list_p == p)
> >  			/* kprobe p is a valid probe */
> >  				goto valid;
> > @@ -1649,7 +1656,9 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
> >  {
> >  	struct kprobe *kp;
> >  
> > -	list_for_each_entry_rcu(kp, &ap->list, list)
> > +	lockdep_assert_held(&kprobe_mutex);
> > +
> > +	list_for_each_entry(kp, &ap->list, list)
> >  		if (!kprobe_disabled(kp))
> >  			/*
> >  			 * There is an active probe on the list.
> > @@ -1728,7 +1737,7 @@ static int __unregister_kprobe_top(struct kprobe *p)
> >  	else {
> >  		/* If disabling probe has special handlers, update aggrprobe */
> >  		if (p->post_handler && !kprobe_gone(p)) {
> > -			list_for_each_entry_rcu(list_p, &ap->list, list) {
> > +			list_for_each_entry(list_p, &ap->list, list) {
> >  				if ((list_p != p) && (list_p->post_handler))
> >  					goto noclean;
> >  			}
> > @@ -2042,13 +2051,15 @@ static void kill_kprobe(struct kprobe *p)
> >  {
> >  	struct kprobe *kp;
> >  
> > +	lockdep_assert_held(&kprobe_mutex);
> > +
> >  	p->flags |= KPROBE_FLAG_GONE;
> >  	if (kprobe_aggrprobe(p)) {
> >  		/*
> >  		 * If this is an aggr_kprobe, we have to list all the
> >  		 * chained probes and mark them GONE.
> >  		 */
> > -		list_for_each_entry_rcu(kp, &p->list, list)
> > +		list_for_each_entry(kp, &p->list, list)
> >  			kp->flags |= KPROBE_FLAG_GONE;
> >  		p->post_handler = NULL;
> >  		kill_optimized_kprobe(p);
> > @@ -2217,7 +2228,7 @@ static int kprobes_module_callback(struct notifier_block *nb,
> >  	mutex_lock(&kprobe_mutex);
> >  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >  		head = &kprobe_table[i];
> > -		hlist_for_each_entry_rcu(p, head, hlist)
> > +		hlist_for_each_entry(p, head, hlist)
> >  			if (within_module_init((unsigned long)p->addr, mod) ||
> >  			    (checkcore &&
> >  			     within_module_core((unsigned long)p->addr, mod))) {
> > @@ -2468,7 +2479,7 @@ static int arm_all_kprobes(void)
> >  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >  		head = &kprobe_table[i];
> >  		/* Arm all kprobes on a best-effort basis */
> > -		hlist_for_each_entry_rcu(p, head, hlist) {
> > +		hlist_for_each_entry(p, head, hlist) {
> >  			if (!kprobe_disabled(p)) {
> >  				err = arm_kprobe(p);
> >  				if (err)  {
> > @@ -2511,7 +2522,7 @@ static int disarm_all_kprobes(void)
> >  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >  		head = &kprobe_table[i];
> >  		/* Disarm all kprobes on a best-effort basis */
> > -		hlist_for_each_entry_rcu(p, head, hlist) {
> > +		hlist_for_each_entry(p, head, hlist) {
> >  			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
> >  				err = disarm_kprobe(p, false);
> >  				if (err) {
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>