linux-kernel - Re: [PATCH 2/2] trace/kprobe: Remove limit on kretprobe maxactive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20210615183527.9068ef2f70fdd2a45fea78f0@kernel.org>
Date:   Tue, 15 Jun 2021 18:35:27 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     <linux-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Anton Blanchard <anton@...abs.org>
Subject: Re: [PATCH 2/2] trace/kprobe: Remove limit on kretprobe maxactive

On Mon, 14 Jun 2021 23:33:29 +0530
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:

> We currently limit maxactive for a kretprobe to 4096 when registering
> the same through tracefs. The comment indicates that this is done so as
> to keep list traversal reasonable. However, we don't ever iterate over
> all kretprobe_instance structures. The core kprobes infrastructure also
> imposes no such limitation.
> 
> Remove the limit from the tracefs interface. This limit is easy to hit
> on large cpu machines when tracing functions that can sleep.
> 
> Reported-by: Anton Blanchard <anton@...abs.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>

OK, but I don't like to just remove the limit (since it can cause
memory shortage easily.)
Can't we make it configurable? I don't mean Kconfig, but 
tracefs/options/kretprobe_maxactive, or kprobes's debugfs knob.

Hmm, maybe debugfs/kprobes/kretprobe_maxactive will be better since
it can limit both trace_kprobe and kprobes itself.

Let me fix that.

Thank you,

> ---
>  kernel/trace/trace_kprobe.c                               | 8 --------
>  kernel/trace/trace_probe.h                                | 1 -
>  .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc          | 1 -
>  .../selftests/ftrace/test.d/kprobe/kretprobe_maxactive.tc | 3 ---
>  4 files changed, 13 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 0475e2a6d0825e..b3e214980eed3d 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -21,7 +21,6 @@
>  #include "trace_probe_tmpl.h"
>  
>  #define KPROBE_EVENT_SYSTEM "kprobes"
> -#define KRETPROBE_MAXACTIVE_MAX 4096
>  
>  /* Kprobe early definition from command line */
>  static char kprobe_boot_events_buf[COMMAND_LINE_SIZE] __initdata;
> @@ -786,13 +785,6 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  			trace_probe_log_err(1, BAD_MAXACT);
>  			goto parse_error;
>  		}
> -		/* kretprobes instances are iterated over via a list. The
> -		 * maximum should stay reasonable.
> -		 */
> -		if (maxactive > KRETPROBE_MAXACTIVE_MAX) {
> -			trace_probe_log_err(1, MAXACT_TOO_BIG);
> -			goto parse_error;
> -		}
>  	}
>  
>  	/* try to parse an address. if that fails, try to read the
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 227d518e5ba521..e331017dc086ed 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -389,7 +389,6 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
>  	C(BAD_UPROBE_OFFS,	"Invalid uprobe offset"),		\
>  	C(MAXACT_NO_KPROBE,	"Maxactive is not for kprobe"),		\
>  	C(BAD_MAXACT,		"Invalid maxactive number"),		\
> -	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
>  	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
>  	C(BAD_RETPROBE,		"Retprobe address must be an function entry"), \
>  	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> index fa928b431555ca..be3360a258bae8 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> @@ -10,7 +10,6 @@ check_error() { # command-with-error-pos-by-^
>  if grep -q 'r\[maxactive\]' README; then
>  check_error 'p^100 vfs_read'		# MAXACT_NO_KPROBE
>  check_error 'r^1a111 vfs_read'		# BAD_MAXACT
> -check_error 'r^100000 vfs_read'		# MAXACT_TOO_BIG
>  fi
>  
>  check_error 'p ^non_exist_func'		# BAD_PROBE_ADDR (enoent)
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_maxactive.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_maxactive.tc
> index 4f0b268c12332a..f57c95bfc5ed5a 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_maxactive.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_maxactive.tc
> @@ -6,9 +6,6 @@
>  # Test if we successfully reject unknown messages
>  if echo 'a:myprobeaccept inet_csk_accept' > kprobe_events; then false; else true; fi
>  
> -# Test if we successfully reject too big maxactive
> -if echo 'r1000000:myprobeaccept inet_csk_accept' > kprobe_events; then false; else true; fi
> -
>  # Test if we successfully reject unparsable numbers for maxactive
>  if echo 'r10fuzz:myprobeaccept inet_csk_accept' > kprobe_events; then false; else true; fi
>  
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>