| 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
| ||
|
Message-ID: <7ab56bb6-0ddb-2c3c-d116-fc01feddba5e@isovalent.com>
Date: Fri, 21 Feb 2020 11:28:05 +0000
From: Quentin Monnet <quentin@...valent.com>
To: Michal Rostecki <mrostecki@...nsuse.org>, bpf@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Andrii Nakryiko <andriin@...com>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Shuah Khan <shuah@...nel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/5] bpftool: Make probes which emit dmesg
warnings optional
2020-02-21 04:16 UTC+0100 ~ Michal Rostecki <mrostecki@...nsuse.org>
> Probes related to bpf_probe_write_user and bpf_trace_printk helpers emit
> dmesg warnings which might be confusing for people running bpftool on
> production environments. This change filters them out by default and
> introduces the new positional argument "full" which enables all
> available probes.
>
> Signed-off-by: Michal Rostecki <mrostecki@...nsuse.org>
> ---
> tools/bpf/bpftool/feature.c | 80 +++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 345e4a2b4f53..0731804b8160 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -3,6 +3,7 @@
>
> #include <ctype.h>
> #include <errno.h>
> +#include <regex.h>
> #include <string.h>
> #include <unistd.h>
> #include <net/if.h>
> @@ -22,6 +23,9 @@
> # define PROC_SUPER_MAGIC 0x9fa0
> #endif
>
> +/* Regex pattern for filtering out probes which emit dmesg warnings */
> +#define FILTER_OUT_PATTERN "(trace|write_user)"
"trace" sounds too generic. If filters are applied again to prog and map
types in the future (as you had in v1), this would catch tracepoint and
raw_tracepoint program types and stack_trace map type. Or if new helpers
with "trace" in their name are added, we skip them too. Can we use
something more specific, probably "trace_printk"?
> +
> enum probe_component {
> COMPONENT_UNSPEC,
> COMPONENT_KERNEL,
> @@ -57,6 +61,35 @@ static void uppercase(char *str, size_t len)
> str[i] = toupper(str[i]);
> }
>
> +/* Filtering utility functions */
> +
> +static bool
> +check_filters(const char *name, regex_t *filter_out)
> +{
> + char err_buf[100];
> + int ret;
> +
> + /* Do not probe if filter_out was defined and string matches against the
> + * pattern.
> + */
> + if (filter_out) {
> + ret = regexec(filter_out, name, 0, NULL, 0);
> + switch (ret) {
> + case 0:
> + return false;
> + case REG_NOMATCH:
> + break;
> + default:
> + regerror(ret, filter_out, err_buf, ARRAY_SIZE(err_buf));
> + p_err("could not match regex: %s", err_buf);
> + free(filter_out);
> + exit(1);
> + }
> + }
> +
> + return true;
> +}
> +
> /* Printing utility functions */
>
> static void
> @@ -515,7 +548,8 @@ probe_map_type(enum bpf_map_type map_type, const char *define_prefix,
>
> static void
> probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
> - const char *define_prefix, __u32 ifindex)
> + const char *define_prefix, regex_t *filter_out,
> + __u32 ifindex)
> {
> const char *ptype_name = prog_type_name[prog_type];
> char feat_name[128];
> @@ -542,6 +576,9 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
> }
>
> for (id = 1; id < ARRAY_SIZE(helper_name); id++) {
> + if (!check_filters(helper_name[id], filter_out))
> + continue;
> +
> if (!supported_type)
> res = false;
> else
> @@ -634,7 +671,8 @@ section_program_types(bool *supported_types, const char *define_prefix,
> define_prefix);
>
> for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
> - probe_prog_type(i, supported_types, define_prefix, ifindex);
> + probe_prog_type(i, supported_types, define_prefix,
> + ifindex);
Splitting the line here is not desirable, probably some leftover after
rolling back on changes?
>
> print_end_section();
> }
> @@ -655,7 +693,8 @@ static void section_map_types(const char *define_prefix, __u32 ifindex)
> }
>
> static void
> -section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
> +section_helpers(bool *supported_types, const char *define_prefix,
> + regex_t *filter_out, __u32 ifindex)
> {
> unsigned int i;
>
> @@ -681,7 +720,7 @@ section_helpers(bool *supported_types, const char *define_prefix, __u32 ifindex)
> define_prefix);
> for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
> probe_helpers_for_progtype(i, supported_types[i],
> - define_prefix, ifindex);
> + define_prefix, filter_out, ifindex);
>
> print_end_section();
> }
> @@ -701,8 +740,13 @@ static int do_probe(int argc, char **argv)
> enum probe_component target = COMPONENT_UNSPEC;
> const char *define_prefix = NULL;
> bool supported_types[128] = {};
> + regex_t *filter_out = NULL;
> + bool full_mode = false;
> + char regerror_buf[100];
> __u32 ifindex = 0;
> char *ifname;
> + int reg_ret;
> + int ret = 0;
>
> /* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN).
> * Let's approximate, and restrict usage to root user only.
> @@ -740,6 +784,9 @@ static int do_probe(int argc, char **argv)
> strerror(errno));
> return -1;
> }
> + } else if (is_prefix(*argv, "full")) {
> + full_mode = true;
> + NEXT_ARG();
> } else if (is_prefix(*argv, "macros") && !define_prefix) {
> define_prefix = "";
> NEXT_ARG();
> @@ -764,6 +811,22 @@ static int do_probe(int argc, char **argv)
> }
> }
>
> + /* If full mode is not acivated, filter out probes which emit dmesg
Typo: acivated
> + * messages.
> + */
> + if (!full_mode) {
> + filter_out = malloc(sizeof(regex_t));
filter_out is not free()-d on the different error paths in the function.
You would probably have to `goto cleanup` from several other locations.
> + reg_ret = regcomp(filter_out, FILTER_OUT_PATTERN, REG_EXTENDED);
> + if (reg_ret) {
> + regerror(reg_ret, filter_out, regerror_buf,
> + ARRAY_SIZE(regerror_buf));
> + p_err("could not compile regex: %s",
> + regerror_buf);
> + ret = -1;
> + goto cleanup;
> + }
> + }
> +
> if (json_output) {
> define_prefix = NULL;
> jsonw_start_object(json_wtr);
> @@ -775,7 +838,7 @@ static int do_probe(int argc, char **argv)
> goto exit_close_json;
> section_program_types(supported_types, define_prefix, ifindex);
> section_map_types(define_prefix, ifindex);
> - section_helpers(supported_types, define_prefix, ifindex);
> + section_helpers(supported_types, define_prefix, filter_out, ifindex);
> section_misc(define_prefix, ifindex);
>
> exit_close_json:
> @@ -783,7 +846,10 @@ static int do_probe(int argc, char **argv)
> /* End root object */
> jsonw_end_object(json_wtr);
>
> - return 0;
> +cleanup:
> + free(filter_out);
> +
> + return ret;
> }
>
> static int do_help(int argc, char **argv)
> @@ -794,7 +860,7 @@ static int do_help(int argc, char **argv)
> }
>
> fprintf(stderr,
> - "Usage: %s %s probe [COMPONENT] [macros [prefix PREFIX]]\n"
> + "Usage: %s %s probe [COMPONENT] [full] [macros [prefix PREFIX]]\n"
> " %s %s help\n"
> "\n"
> " COMPONENT := { kernel | dev NAME }\n"
>
Thanks for the patch! While I understand you want to keep the changes
you have done to use regex, I do not really think they bring much in
this version of the patch. As we only want to filter out two specific
helpers, it seems to me that it would be much simpler to just compare
helper names instead of introducing regular expressions that are not
used otherwise. What do you think?
Quentin
Powered by blists - more mailing lists