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] [day] [month] [year] [list]
Message-ID: <CAADnVQ+4YghLdZ7X-PSUyUvGc2RreFHgTLS-J7hBZm3WTEMEjg@mail.gmail.com>
Date: Tue, 25 Jun 2024 17:09:52 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Vlastimil Babka <vbabka@...e.cz>, Jiri Olsa <jolsa@...nel.org>, 
	Andrii Nakryiko <andrii@...nel.org>
Cc: Akinobu Mita <akinobu.mita@...il.com>, Christoph Lameter <cl@...ux.com>, 
	David Rientjes <rientjes@...gle.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>, 
	Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>, "David S. Miller" <davem@...emloft.net>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Steven Rostedt <rostedt@...dmis.org>, 
	Mark Rutland <mark.rutland@....com>, Roman Gushchin <roman.gushchin@...ux.dev>, 
	Hyeonggon Yoo <42.hyeyoo@...il.com>, LKML <linux-kernel@...r.kernel.org>, 
	linux-mm <linux-mm@...ck.org>, bpf <bpf@...r.kernel.org>, 
	linux-trace-kernel <linux-trace-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/7] bpf: support error injection static keys for
 multi_link attached progs

On Wed, Jun 19, 2024 at 3:49 PM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> Functions marked for error injection can have an associated static key
> that guards the callsite(s) to avoid overhead of calling an empty
> function when no error injection is in progress.
>
> Outside of the error injection framework itself, bpf programs can be
> atteched to kprobes and override results of error-injectable functions.
> To make sure these functions are actually called, attaching such bpf
> programs should control the static key accordingly.
>
> Therefore, add an array of static keys to struct bpf_kprobe_multi_link
> and fill it in addrs_check_error_injection_list() for programs with
> kprobe_override enabled, using get_injection_key() instead of
> within_error_injection_list(). Introduce bpf_kprobe_ei_keys_control() to
> control the static keys and call the control function when doing
> multi_link_attach and release.
>
> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
> ---
>  kernel/trace/bpf_trace.c | 59 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 944de1c41209..ef0fadb76bfa 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2613,6 +2613,7 @@ struct bpf_kprobe_multi_link {
>         struct bpf_link link;
>         struct fprobe fp;
>         unsigned long *addrs;
> +       struct static_key **ei_keys;
>         u64 *cookies;
>         u32 cnt;
>         u32 mods_cnt;
> @@ -2687,11 +2688,30 @@ static void free_user_syms(struct user_syms *us)
>         kvfree(us->buf);
>  }
>
> +static void bpf_kprobe_ei_keys_control(struct bpf_kprobe_multi_link *link, bool enable)
> +{
> +       u32 i;
> +
> +       for (i = 0; i < link->cnt; i++) {
> +               if (!link->ei_keys[i])
> +                       break;
> +
> +               if (enable)
> +                       static_key_slow_inc(link->ei_keys[i]);
> +               else
> +                       static_key_slow_dec(link->ei_keys[i]);
> +       }
> +}
> +
>  static void bpf_kprobe_multi_link_release(struct bpf_link *link)
>  {
>         struct bpf_kprobe_multi_link *kmulti_link;
>
>         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> +
> +       if (kmulti_link->ei_keys)
> +               bpf_kprobe_ei_keys_control(kmulti_link, false);
> +
>         unregister_fprobe(&kmulti_link->fp);
>         kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
>  }
> @@ -2703,6 +2723,7 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
>         kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
>         kvfree(kmulti_link->addrs);
>         kvfree(kmulti_link->cookies);
> +       kvfree(kmulti_link->ei_keys);
>         kfree(kmulti_link->mods);
>         kfree(kmulti_link);
>  }
> @@ -2985,13 +3006,19 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
>         return arr.mods_cnt;
>  }
>
> -static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> +static int addrs_check_error_injection_list(unsigned long *addrs, struct static_key **ei_keys,
> +                                           u32 cnt)
>  {
> -       u32 i;
> +       struct static_key *ei_key;
> +       u32 i, j = 0;
>
>         for (i = 0; i < cnt; i++) {
> -               if (!within_error_injection_list(addrs[i]))
> +               ei_key = get_injection_key(addrs[i]);
> +               if (IS_ERR(ei_key))
>                         return -EINVAL;
> +
> +               if (ei_key)
> +                       ei_keys[j++] = ei_key;
>         }
>         return 0;
>  }
> @@ -3000,6 +3027,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  {
>         struct bpf_kprobe_multi_link *link = NULL;
>         struct bpf_link_primer link_primer;
> +       struct static_key **ei_keys = NULL;
>         void __user *ucookies;
>         unsigned long *addrs;
>         u32 flags, cnt, size;
> @@ -3075,9 +3103,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                         goto error;
>         }
>
> -       if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
> -               err = -EINVAL;
> -               goto error;
> +       if (prog->kprobe_override) {
> +               ei_keys = kvcalloc(cnt, sizeof(*ei_keys), GFP_KERNEL);

cnt can be huge. Like tens of thousands.
while number of keys is tiny. two so far?
So most of the array will be wasted.

Jiri, Andrii,

please take a look as well.

> +               if (!ei_keys) {
> +                       err = -ENOMEM;
> +                       goto error;
> +               }
> +
> +               if (addrs_check_error_injection_list(addrs, ei_keys, cnt)) {
> +                       err = -EINVAL;
> +                       goto error;
> +               }
> +
> +               if (ei_keys[0]) {
> +                       link->ei_keys = ei_keys;
> +               } else {
> +                       kvfree(ei_keys);
> +                       ei_keys = NULL;
> +               }
>         }
>
>         link = kzalloc(sizeof(*link), GFP_KERNEL);
> @@ -3132,10 +3175,14 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                 return err;
>         }
>
> +       if (link->ei_keys)
> +               bpf_kprobe_ei_keys_control(link, true);
> +
>         return bpf_link_settle(&link_primer);
>
>  error:
>         kfree(link);
> +       kvfree(ei_keys);
>         kvfree(addrs);
>         kvfree(cookies);
>         return err;
>
> --
> 2.45.2
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ