diff mbox series

kprobes: Fix check for probe enabled in kill_kprobe()

Message ID 20221126114316.201857-1-lihuafei1@huawei.com
State New
Headers
Series kprobes: Fix check for probe enabled in kill_kprobe() |

Commit Message

Li Huafei Nov. 26, 2022, 11:43 a.m. UTC 
  In kill_kprobe(), the check whether disarm_kprobe_ftrace() needs to be
called always fails. This is because before that we set the
KPROBE_FLAG_GONE flag for kprobe so that "!kprobe_disabled(p)" is always
false.

The disarm_kprobe_ftrace() call introduced by commit:

  0cb2f1372baa ("kprobes: Fix NULL pointer dereference at kprobe_ftrace_handler")

to fix the NULL pointer reference problem. When the probe is enabled, if
we do not disarm it, this problem still exists.

Fix it by putting the probe enabled check before setting the
KPROBE_FLAG_GONE flag.

Fixes: 3031313eb3d54 ("kprobes: Fix to check probe enabled before disarm_kprobe_ftrace()")
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 kernel/kprobes.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Masami Hiramatsu (Google) Nov. 28, 2022, 8:48 a.m. UTC | #1 
On Sat, 26 Nov 2022 19:43:16 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> In kill_kprobe(), the check whether disarm_kprobe_ftrace() needs to be
> called always fails. This is because before that we set the
> KPROBE_FLAG_GONE flag for kprobe so that "!kprobe_disabled(p)" is always
> false.

Good catch!

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

Thank you!

> 
> The disarm_kprobe_ftrace() call introduced by commit:
> 
>   0cb2f1372baa ("kprobes: Fix NULL pointer dereference at kprobe_ftrace_handler")
> 
> to fix the NULL pointer reference problem. When the probe is enabled, if
> we do not disarm it, this problem still exists.
> 
> Fix it by putting the probe enabled check before setting the
> KPROBE_FLAG_GONE flag.
> 
> Fixes: 3031313eb3d54 ("kprobes: Fix to check probe enabled before disarm_kprobe_ftrace()")
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> ---
>  kernel/kprobes.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 3050631e528d..a35074f0daa1 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2364,6 +2364,14 @@ static void kill_kprobe(struct kprobe *p)
>  
>  	lockdep_assert_held(&kprobe_mutex);
>  
> +	/*
> +	 * The module is going away. We should disarm the kprobe which
> +	 * is using ftrace, because ftrace framework is still available at
> +	 * 'MODULE_STATE_GOING' notification.
> +	 */
> +	if (kprobe_ftrace(p) && !kprobe_disabled(p) && !kprobes_all_disarmed)
> +		disarm_kprobe_ftrace(p);
> +
>  	p->flags |= KPROBE_FLAG_GONE;
>  	if (kprobe_aggrprobe(p)) {
>  		/*
> @@ -2380,14 +2388,6 @@ static void kill_kprobe(struct kprobe *p)
>  	 * the original probed function (which will be freed soon) any more.
>  	 */
>  	arch_remove_kprobe(p);
> -
> -	/*
> -	 * The module is going away. We should disarm the kprobe which
> -	 * is using ftrace, because ftrace framework is still available at
> -	 * 'MODULE_STATE_GOING' notification.
> -	 */
> -	if (kprobe_ftrace(p) && !kprobe_disabled(p) && !kprobes_all_disarmed)
> -		disarm_kprobe_ftrace(p);
>  }
>  
>  /* Disable one kprobe */
> -- 
> 2.17.1
>
Steven Rostedt Nov. 28, 2022, 7:21 p.m. UTC | #2 
On Mon, 28 Nov 2022 17:48:43 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Sat, 26 Nov 2022 19:43:16 +0800
> Li Huafei <lihuafei1@huawei.com> wrote:
> 
> > In kill_kprobe(), the check whether disarm_kprobe_ftrace() needs to be
> > called always fails. This is because before that we set the
> > KPROBE_FLAG_GONE flag for kprobe so that "!kprobe_disabled(p)" is always
> > false.  
> 
> Good catch!
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thank you!

Looks good.

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve
diff mbox series

Patch

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3050631e528d..a35074f0daa1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2364,6 +2364,14 @@  static void kill_kprobe(struct kprobe *p)
 
 	lockdep_assert_held(&kprobe_mutex);
 
+	/*
+	 * The module is going away. We should disarm the kprobe which
+	 * is using ftrace, because ftrace framework is still available at
+	 * 'MODULE_STATE_GOING' notification.
+	 */
+	if (kprobe_ftrace(p) && !kprobe_disabled(p) && !kprobes_all_disarmed)
+		disarm_kprobe_ftrace(p);
+
 	p->flags |= KPROBE_FLAG_GONE;
 	if (kprobe_aggrprobe(p)) {
 		/*
@@ -2380,14 +2388,6 @@  static void kill_kprobe(struct kprobe *p)
 	 * the original probed function (which will be freed soon) any more.
 	 */
 	arch_remove_kprobe(p);
-
-	/*
-	 * The module is going away. We should disarm the kprobe which
-	 * is using ftrace, because ftrace framework is still available at
-	 * 'MODULE_STATE_GOING' notification.
-	 */
-	if (kprobe_ftrace(p) && !kprobe_disabled(p) && !kprobes_all_disarmed)
-		disarm_kprobe_ftrace(p);
 }
 
 /* Disable one kprobe */