FAILED: patch "[PATCH] kprobes: consistent rcu api usage for kretprobe holder" failed to apply to 6.6-stable tree

FAILED: patch "[PATCH] kprobes: consistent rcu api usage for kretprobe holder" failed to apply to 6.6-stable tree

[gregkh]

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y
git checkout FETCH_HEAD
git cherry-pick -x d839a656d0f3caca9f96e9bf912fd394ac6a11bc
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2023120316-seduce-vehicular-9e78@gregkh' --subject-prefix 'PATCH 6.6.y' HEAD^..

Possible dependencies:

d839a656d0f3 ("kprobes: consistent rcu api usage for kretprobe holder")
4bbd93455659 ("kprobes: kretprobe scalability improvement")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From d839a656d0f3caca9f96e9bf912fd394ac6a11bc Mon Sep 17 00:00:00 2001
From: JP Kobryn <inwardvessel@gmail.com>
Date: Fri, 1 Dec 2023 14:53:55 +0900
Subject: [PATCH] kprobes: consistent rcu api usage for kretprobe holder

It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
The thought behind this patch is to make use of the RCU API where possible
when accessing this pointer so that the needed barriers are always in place
and to self-document the code.

The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
done to the "rp" pointer are changed to make use of the RCU macro for
assignment. For the single read, the implementation of get_kretprobe()
is simplified by making use of an RCU macro which accomplishes the same,
but note that the log warning text will be more generic.

I did find that there is a difference in assembly generated between the
usage of the RCU macros vs without. For example, on arm64, when using
rcu_assign_pointer(), the corresponding store instruction is a
store-release (STLR) which has an implicit barrier. When normal assignment
is done, a regular store (STR) is found. In the macro case, this seems to
be a result of rcu_assign_pointer() using smp_store_release() when the
value to write is not NULL.

Link: https://lore.kernel.org/all/20231122132058.3359-1-inwardvessel@gmail.com/

Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
Cc: stable@vger.kernel.org
Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index ab1da3142b06..64672bace560 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -139,7 +139,7 @@ static inline bool kprobe_ftrace(struct kprobe *p)
  *
  */
 struct kretprobe_holder {
-	struct kretprobe	*rp;
+	struct kretprobe __rcu *rp;
 	struct objpool_head	pool;
 };
 
@@ -245,10 +245,7 @@ unsigned long kretprobe_trampoline_handler(struct pt_regs *regs,
 
 static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
 {
-	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
-		"Kretprobe is accessed from instance under preemptive context");
-
-	return READ_ONCE(ri->rph->rp);
+	return rcu_dereference_check(ri->rph->rp, rcu_read_lock_any_held());
 }
 
 static nokprobe_inline unsigned long get_kretprobe_retaddr(struct kretprobe_instance *ri)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 075a632e6c7c..d5a0ee40bf66 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2252,7 +2252,7 @@ int register_kretprobe(struct kretprobe *rp)
 		rp->rph = NULL;
 		return -ENOMEM;
 	}
-	rp->rph->rp = rp;
+	rcu_assign_pointer(rp->rph->rp, rp);
 	rp->nmissed = 0;
 	/* Establish function entry probe point */
 	ret = register_kprobe(&rp->kp);
@@ -2300,7 +2300,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
 		rethook_free(rps[i]->rh);
 #else
-		rps[i]->rph->rp = NULL;
+		rcu_assign_pointer(rps[i]->rph->rp, NULL);
 #endif
 	}
 	mutex_unlock(&kprobe_mutex);

[PATCH 6.6.y] kprobes: consistent rcu api usage for kretprobe holder

[mhiramat]

From: JP Kobryn <inwardvessel@gmail.com>

It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
The thought behind this patch is to make use of the RCU API where possible
when accessing this pointer so that the needed barriers are always in place
and to self-document the code.

The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
done to the "rp" pointer are changed to make use of the RCU macro for
assignment. For the single read, the implementation of get_kretprobe()
is simplified by making use of an RCU macro which accomplishes the same,
but note that the log warning text will be more generic.

I did find that there is a difference in assembly generated between the
usage of the RCU macros vs without. For example, on arm64, when using
rcu_assign_pointer(), the corresponding store instruction is a
store-release (STLR) which has an implicit barrier. When normal assignment
is done, a regular store (STR) is found. In the macro case, this seems to
be a result of rcu_assign_pointer() using smp_store_release() when the
value to write is not NULL.

Link: https://lore.kernel.org/all/20231122132058.3359-1-inwardvessel@gmail.com/

Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
Cc: stable@vger.kernel.org
Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
(cherry picked from commit d839a656d0f3caca9f96e9bf912fd394ac6a11bc)
---
 include/linux/kprobes.h | 8 +++-----
 kernel/kprobes.c        | 4 ++--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 38a774287bde..63ad21a6b1e0 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -140,8 +140,9 @@ static inline bool kprobe_ftrace(struct kprobe *p)
  *
  */
 struct kretprobe_holder {
-	struct kretprobe	*rp;
+	struct kretprobe __rcu *rp;
 	refcount_t		ref;
+	struct objpool_head	pool;
 };
 
 struct kretprobe {
@@ -248,10 +249,7 @@ unsigned long kretprobe_trampoline_handler(struct pt_regs *regs,
 
 static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
 {
-	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
-		"Kretprobe is accessed from instance under preemptive context");
-
-	return READ_ONCE(ri->rph->rp);
+	return rcu_dereference_check(ri->rph->rp, rcu_read_lock_any_held());
 }
 
 static nokprobe_inline unsigned long get_kretprobe_retaddr(struct kretprobe_instance *ri)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 0c6185aefaef..b486504766fb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2253,7 +2253,7 @@ int register_kretprobe(struct kretprobe *rp)
 	if (!rp->rph)
 		return -ENOMEM;
 
-	rp->rph->rp = rp;
+	rcu_assign_pointer(rp->rph->rp, rp);
 	for (i = 0; i < rp->maxactive; i++) {
 		inst = kzalloc(struct_size(inst, data, rp->data_size), GFP_KERNEL);
 		if (inst == NULL) {
@@ -2313,7 +2313,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
 		rethook_free(rps[i]->rh);
 #else
-		rps[i]->rph->rp = NULL;
+		rcu_assign_pointer(rps[i]->rph->rp, NULL);
 #endif
 	}
 	mutex_unlock(&kprobe_mutex);
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH 6.6.y] kprobes: consistent rcu api usage for kretprobe holder

[Greg KH]

On Wed, Dec 06, 2023 at 10:57:11AM +0900, mhiramat@kernel.org wrote:
> From: JP Kobryn <inwardvessel@gmail.com>
> 
> It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
> RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
> The thought behind this patch is to make use of the RCU API where possible
> when accessing this pointer so that the needed barriers are always in place
> and to self-document the code.
> 
> The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
> done to the "rp" pointer are changed to make use of the RCU macro for
> assignment. For the single read, the implementation of get_kretprobe()
> is simplified by making use of an RCU macro which accomplishes the same,
> but note that the log warning text will be more generic.
> 
> I did find that there is a difference in assembly generated between the
> usage of the RCU macros vs without. For example, on arm64, when using
> rcu_assign_pointer(), the corresponding store instruction is a
> store-release (STLR) which has an implicit barrier. When normal assignment
> is done, a regular store (STR) is found. In the macro case, this seems to
> be a result of rcu_assign_pointer() using smp_store_release() when the
> value to write is not NULL.
> 
> Link: https://lore.kernel.org/all/20231122132058.3359-1-inwardvessel@gmail.com/
> 
> Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
> Cc: stable@vger.kernel.org
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> (cherry picked from commit d839a656d0f3caca9f96e9bf912fd394ac6a11bc)
> ---
>  include/linux/kprobes.h | 8 +++-----
>  kernel/kprobes.c        | 4 ++--
>  2 files changed, 5 insertions(+), 7 deletions(-)

Did you build this?  It breaks the build in 6.6.y in horrible ways:

./include/linux/kprobes.h:145:33: error: field ‘pool’ has incomplete type
  145 |         struct objpool_head     pool;
      |                                 ^~~~


I'll drop this, can you please provide a working version?

thanks,

greg k-h

Re: [PATCH 6.6.y] kprobes: consistent rcu api usage for kretprobe holder

[Masami Hiramatsu]

On Sat, 9 Dec 2023 13:20:09 +0100
Greg KH <greg@kroah.com> wrote:

> On Wed, Dec 06, 2023 at 10:57:11AM +0900, mhiramat@kernel.org wrote:
> > From: JP Kobryn <inwardvessel@gmail.com>
> > 
> > It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
> > RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
> > The thought behind this patch is to make use of the RCU API where possible
> > when accessing this pointer so that the needed barriers are always in place
> > and to self-document the code.
> > 
> > The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
> > done to the "rp" pointer are changed to make use of the RCU macro for
> > assignment. For the single read, the implementation of get_kretprobe()
> > is simplified by making use of an RCU macro which accomplishes the same,
> > but note that the log warning text will be more generic.
> > 
> > I did find that there is a difference in assembly generated between the
> > usage of the RCU macros vs without. For example, on arm64, when using
> > rcu_assign_pointer(), the corresponding store instruction is a
> > store-release (STLR) which has an implicit barrier. When normal assignment
> > is done, a regular store (STR) is found. In the macro case, this seems to
> > be a result of rcu_assign_pointer() using smp_store_release() when the
> > value to write is not NULL.
> > 
> > Link: https://lore.kernel.org/all/20231122132058.3359-1-inwardvessel@gmail.com/
> > 
> > Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > (cherry picked from commit d839a656d0f3caca9f96e9bf912fd394ac6a11bc)
> > ---
> >  include/linux/kprobes.h | 8 +++-----
> >  kernel/kprobes.c        | 4 ++--
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> Did you build this?  It breaks the build in 6.6.y in horrible ways:
> 
> ./include/linux/kprobes.h:145:33: error: field ‘pool’ has incomplete type
>   145 |         struct objpool_head     pool;
>       |                                 ^~~~
> 
> 
> I'll drop this, can you please provide a working version?

Oops, sorry. I missed to patch this version.

Let me update it.

> 
> thanks,
> 
> greg k-h


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

[PATCH 6.6.y v2] kprobes: consistent rcu api usage for kretprobe holder

[mhiramat]

From: JP Kobryn <inwardvessel@gmail.com>

It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
The thought behind this patch is to make use of the RCU API where possible
when accessing this pointer so that the needed barriers are always in place
and to self-document the code.

The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
done to the "rp" pointer are changed to make use of the RCU macro for
assignment. For the single read, the implementation of get_kretprobe()
is simplified by making use of an RCU macro which accomplishes the same,
but note that the log warning text will be more generic.

I did find that there is a difference in assembly generated between the
usage of the RCU macros vs without. For example, on arm64, when using
rcu_assign_pointer(), the corresponding store instruction is a
store-release (STLR) which has an implicit barrier. When normal assignment
is done, a regular store (STR) is found. In the macro case, this seems to
be a result of rcu_assign_pointer() using smp_store_release() when the
value to write is not NULL.

Link: https://lore.kernel.org/all/20231122132058.3359-1-inwardvessel@gmail.com/

Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
Cc: stable@vger.kernel.org
Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
(cherry picked from commit d839a656d0f3caca9f96e9bf912fd394ac6a11bc)
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/kprobes.h | 7 ++-----
 kernel/kprobes.c        | 4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 85a64cb95d75..0fce4951a554 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -140,7 +140,7 @@ static inline bool kprobe_ftrace(struct kprobe *p)
  *
  */
 struct kretprobe_holder {
-	struct kretprobe	*rp;
+	struct kretprobe __rcu *rp;
 	refcount_t		ref;
 };
 
@@ -250,10 +250,7 @@ unsigned long kretprobe_trampoline_handler(struct pt_regs *regs,
 
 static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
 {
-	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
-		"Kretprobe is accessed from instance under preemptive context");
-
-	return READ_ONCE(ri->rph->rp);
+	return rcu_dereference_check(ri->rph->rp, rcu_read_lock_any_held());
 }
 
 static nokprobe_inline unsigned long get_kretprobe_retaddr(struct kretprobe_instance *ri)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 0c6185aefaef..b486504766fb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2253,7 +2253,7 @@ int register_kretprobe(struct kretprobe *rp)
 	if (!rp->rph)
 		return -ENOMEM;
 
-	rp->rph->rp = rp;
+	rcu_assign_pointer(rp->rph->rp, rp);
 	for (i = 0; i < rp->maxactive; i++) {
 		inst = kzalloc(struct_size(inst, data, rp->data_size), GFP_KERNEL);
 		if (inst == NULL) {
@@ -2313,7 +2313,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
 		rethook_free(rps[i]->rh);
 #else
-		rps[i]->rph->rp = NULL;
+		rcu_assign_pointer(rps[i]->rph->rp, NULL);
 #endif
 	}
 	mutex_unlock(&kprobe_mutex);
-- 
2.34.1