* [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 1:52 [PATCH bpf-next 0/5] Add SEC("ksyscall") support Andrii Nakryiko
@ 2022-07-13 1:53 ` Andrii Nakryiko
2022-07-13 15:18 ` Alan Maguire
2022-07-13 16:24 ` sdf
2022-07-13 1:53 ` [PATCH bpf-next 2/5] selftests/bpf: add test of __weak unknown virtual __kconfig extern Andrii Nakryiko
` (3 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 1:53 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
Libbpf supports single virtual __kconfig extern currently: LINUX_KERNEL_VERSION.
LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
customly filled out by libbpf.
This patch generalizes this approach to support more such virtual
__kconfig externs. One such extern added in this patch is
LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
usdt.bpf.h instead of using CO-RE-based enum detection approach for
detecting bpf_get_attach_cookie() BPF helper. This allows to remove
otherwise not needed CO-RE dependency and keeps user-space and BPF-side
parts of libbpf's USDT support strictly in sync in terms of their
feature detection.
We'll use similar approach for syscall wrapper detection for
BPF_KSYSCALL() BPF-side macro in follow up patch.
Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
and LINUX_ for virtual libbpf-backed externs. In the future we might
extend the set of prefixes that are supported. This can be done without
any breaking changes, as currently any __kconfig extern with
unrecognized name is rejected.
For LINUX_xxx externs we support the normal "weak rule": if libbpf
doesn't recognize given LINUX_xxx extern but such extern is marked as
__weak, it is not rejected and defaults to zero. This follows
CONFIG_xxx handling logic and will allow BPF applications to
opportunistically use newer libbpf virtual externs without breaking on
older libbpf versions unnecessarily.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
tools/lib/bpf/usdt.bpf.h | 16 ++--------
2 files changed, 52 insertions(+), 33 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index cb49408eb298..4bae67767f82 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct extern_desc *ext, __u64 v)
static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
__u64 value)
{
- if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
- pr_warn("extern (kcfg) %s=%llu should be integer\n",
+ if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
+ ext->kcfg.type != KCFG_BOOL) {
+ pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
ext->name, (unsigned long long)value);
return -EINVAL;
}
+ if (ext->kcfg.type == KCFG_BOOL && value > 1) {
+ pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
+ ext->name, (unsigned long long)value);
+ return -EINVAL;
+
+ }
if (!is_kcfg_value_in_range(ext, value)) {
pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
ext->name, (unsigned long long)value, ext->kcfg.sz);
@@ -1870,10 +1877,13 @@ static int bpf_object__process_kconfig_line(struct bpf_object *obj,
/* assume integer */
err = parse_u64(value, &num);
if (err) {
- pr_warn("extern (kcfg) %s=%s should be integer\n",
- ext->name, value);
+ pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
return err;
}
+ if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
+ pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
+ return -EINVAL;
+ }
err = set_kcfg_value_num(ext, ext_val, num);
break;
}
@@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
for (i = 0; i < obj->nr_extern; i++) {
ext = &obj->externs[i];
- if (ext->type == EXT_KCFG &&
- strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
- void *ext_val = kcfg_data + ext->kcfg.data_off;
- __u32 kver = get_kernel_version();
+ if (ext->type == EXT_KSYM) {
+ if (ext->ksym.type_id)
+ need_vmlinux_btf = true;
+ else
+ need_kallsyms = true;
+ continue;
+ } else if (ext->type == EXT_KCFG) {
+ void *ext_ptr = kcfg_data + ext->kcfg.data_off;
+ __u64 value = 0;
+
+ /* Kconfig externs need actual /proc/config.gz */
+ if (str_has_pfx(ext->name, "CONFIG_")) {
+ need_config = true;
+ continue;
+ }
- if (!kver) {
- pr_warn("failed to get kernel version\n");
+ /* Virtual kcfg externs are customly handled by libbpf */
+ if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
+ value = get_kernel_version();
+ if (!value) {
+ pr_warn("extern (kcfg) '%s': failed to get kernel version\n", ext->name);
+ return -EINVAL;
+ }
+ } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
+ value = kernel_supports(obj, FEAT_BPF_COOKIE);
+ } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
+ /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
+ * __kconfig externs, where LINUX_ ones are virtual and filled out
+ * customly by libbpf (their values don't come from Kconfig).
+ * If LINUX_xxx variable is not recognized by libbpf, but is marked
+ * __weak, it defaults to zero value, just like for CONFIG_xxx
+ * externs.
+ */
+ pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n", ext->name);
return -EINVAL;
}
- err = set_kcfg_value_num(ext, ext_val, kver);
+
+ err = set_kcfg_value_num(ext, ext_ptr, value);
if (err)
return err;
- pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
- } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_")) {
- need_config = true;
- } else if (ext->type == EXT_KSYM) {
- if (ext->ksym.type_id)
- need_vmlinux_btf = true;
- else
- need_kallsyms = true;
+ pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
} else {
pr_warn("unrecognized extern '%s'\n", ext->name);
return -EINVAL;
diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
index 4181fddb3687..4f2adc0bd6ca 100644
--- a/tools/lib/bpf/usdt.bpf.h
+++ b/tools/lib/bpf/usdt.bpf.h
@@ -6,7 +6,6 @@
#include <linux/errno.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
-#include <bpf/bpf_core_read.h>
/* Below types and maps are internal implementation details of libbpf's USDT
* support and are subjects to change. Also, bpf_usdt_xxx() API helpers should
@@ -30,14 +29,6 @@
#ifndef BPF_USDT_MAX_IP_CNT
#define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
#endif
-/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This is
- * the only dependency on CO-RE, so if it's undesirable, user can override
- * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported or not.
- */
-#ifndef BPF_USDT_HAS_BPF_COOKIE
-#define BPF_USDT_HAS_BPF_COOKIE \
- bpf_core_enum_value_exists(enum bpf_func_id___usdt, BPF_FUNC_get_attach_cookie___usdt)
-#endif
enum __bpf_usdt_arg_type {
BPF_USDT_ARG_CONST,
@@ -83,15 +74,12 @@ struct {
__type(value, __u32);
} __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
-/* don't rely on user's BPF code to have latest definition of bpf_func_id */
-enum bpf_func_id___usdt {
- BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
-};
+extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
static __always_inline
int __bpf_usdt_spec_id(struct pt_regs *ctx)
{
- if (!BPF_USDT_HAS_BPF_COOKIE) {
+ if (!LINUX_HAS_BPF_COOKIE) {
long ip = PT_REGS_IP(ctx);
int *spec_id_ptr;
--
2.30.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 1:53 ` [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT Andrii Nakryiko
@ 2022-07-13 15:18 ` Alan Maguire
2022-07-13 18:02 ` Andrii Nakryiko
2022-07-13 16:24 ` sdf
1 sibling, 1 reply; 18+ messages in thread
From: Alan Maguire @ 2022-07-13 15:18 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team
On 13/07/2022 02:53, Andrii Nakryiko wrote:
> Libbpf supports single virtual __kconfig extern currently: LINUX_KERNEL_VERSION.
> LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> customly filled out by libbpf.
>
> This patch generalizes this approach to support more such virtual
> __kconfig externs. One such extern added in this patch is
> LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> usdt.bpf.h instead of using CO-RE-based enum detection approach for
> detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> parts of libbpf's USDT support strictly in sync in terms of their
> feature detection.
>
> We'll use similar approach for syscall wrapper detection for
> BPF_KSYSCALL() BPF-side macro in follow up patch.
>
> Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> and LINUX_ for virtual libbpf-backed externs. In the future we might
> extend the set of prefixes that are supported. This can be done without
> any breaking changes, as currently any __kconfig extern with
> unrecognized name is rejected.
>
> For LINUX_xxx externs we support the normal "weak rule": if libbpf
> doesn't recognize given LINUX_xxx extern but such extern is marked as
> __weak, it is not rejected and defaults to zero. This follows
> CONFIG_xxx handling logic and will allow BPF applications to
> opportunistically use newer libbpf virtual externs without breaking on
> older libbpf versions unnecessarily.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Tested the v1 patch series on arm64, all works well, so feel free to add
Tested-by: Alan Maguire <alan.maguire@oracle.com>
...for the series.
I really like the concept of extending LINUX_ kconfig values.
A few nits, but looks great!
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> tools/lib/bpf/usdt.bpf.h | 16 ++--------
> 2 files changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index cb49408eb298..4bae67767f82 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct extern_desc *ext, __u64 v)
> static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
> __u64 value)
> {
> - if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> - pr_warn("extern (kcfg) %s=%llu should be integer\n",
> + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
> + ext->kcfg.type != KCFG_BOOL) {
> + pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
> ext->name, (unsigned long long)value);
> return -EINVAL;
> }
> + if (ext->kcfg.type == KCFG_BOOL && value > 1) {
> + pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
most warnings sem to conform to the format
pr_warn("extern (kcfg) '%s'; value '%llu' isn't boolean\n",
I find that a bit clearer but subjective I realize...
> + ext->name, (unsigned long long)value);
> + return -EINVAL;
> +
> + }
> if (!is_kcfg_value_in_range(ext, value)) {
> pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> ext->name, (unsigned long long)value, ext->kcfg.sz);
> @@ -1870,10 +1877,13 @@ static int bpf_object__process_kconfig_line(struct bpf_object *obj,
> /* assume integer */
> err = parse_u64(value, &num);
> if (err) {
> - pr_warn("extern (kcfg) %s=%s should be integer\n",
> - ext->name, value);
> + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> return err;
> }
> + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
I think the logic here is meant to support a KCONFIG_CHAR value that is expressed as an
integer; if I've got this right would the error message read better as something like
pr_warn("extern (kcfg) '%s': '%s' isn't an integer value\n", ext->name, value);
> + return -EINVAL;
> + }
> err = set_kcfg_value_num(ext, ext_val, num);
> break;
> }
> @@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
> for (i = 0; i < obj->nr_extern; i++) {
> ext = &obj->externs[i];
>
> - if (ext->type == EXT_KCFG &&
> - strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> - void *ext_val = kcfg_data + ext->kcfg.data_off;
> - __u32 kver = get_kernel_version();
> + if (ext->type == EXT_KSYM) {
> + if (ext->ksym.type_id)
> + need_vmlinux_btf = true;
> + else
> + need_kallsyms = true;
> + continue;
> + } else if (ext->type == EXT_KCFG) {
> + void *ext_ptr = kcfg_data + ext->kcfg.data_off;
> + __u64 value = 0;
> +
> + /* Kconfig externs need actual /proc/config.gz */
> + if (str_has_pfx(ext->name, "CONFIG_")) {
> + need_config = true;
> + continue;
> + }
>
> - if (!kver) {
> - pr_warn("failed to get kernel version\n");
> + /* Virtual kcfg externs are customly handled by libbpf */
> + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> + value = get_kernel_version();
> + if (!value) {
> + pr_warn("extern (kcfg) '%s': failed to get kernel version\n", ext->name);
> + return -EINVAL;
> + }
> + } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
> + value = kernel_supports(obj, FEAT_BPF_COOKIE);
> + } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
> + /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
> + * __kconfig externs, where LINUX_ ones are virtual and filled out
> + * customly by libbpf (their values don't come from Kconfig).
> + * If LINUX_xxx variable is not recognized by libbpf, but is marked
> + * __weak, it defaults to zero value, just like for CONFIG_xxx
> + * externs.
> + */
> + pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n", ext->name);
> return -EINVAL;
> }
> - err = set_kcfg_value_num(ext, ext_val, kver);
> +
> + err = set_kcfg_value_num(ext, ext_ptr, value);
> if (err)
> return err;
> - pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> - } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_")) {
> - need_config = true;
> - } else if (ext->type == EXT_KSYM) {
> - if (ext->ksym.type_id)
> - need_vmlinux_btf = true;
> - else
> - need_kallsyms = true;
> + pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
nit: should we use "extern (kcfg) '%s'=" as above to be consistent with warning messages?
> } else {
> pr_warn("unrecognized extern '%s'\n", ext->name);
> return -EINVAL;
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index 4181fddb3687..4f2adc0bd6ca 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -6,7 +6,6 @@
> #include <linux/errno.h>
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> -#include <bpf/bpf_core_read.h>
>
> /* Below types and maps are internal implementation details of libbpf's USDT
> * support and are subjects to change. Also, bpf_usdt_xxx() API helpers should
> @@ -30,14 +29,6 @@
> #ifndef BPF_USDT_MAX_IP_CNT
> #define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
> #endif
> -/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This is
> - * the only dependency on CO-RE, so if it's undesirable, user can override
> - * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported or not.
> - */
> -#ifndef BPF_USDT_HAS_BPF_COOKIE
> -#define BPF_USDT_HAS_BPF_COOKIE \
> - bpf_core_enum_value_exists(enum bpf_func_id___usdt, BPF_FUNC_get_attach_cookie___usdt)
> -#endif
>
> enum __bpf_usdt_arg_type {
> BPF_USDT_ARG_CONST,
> @@ -83,15 +74,12 @@ struct {
> __type(value, __u32);
> } __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
>
> -/* don't rely on user's BPF code to have latest definition of bpf_func_id */
> -enum bpf_func_id___usdt {
> - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> -};
> +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
>
> static __always_inline
> int __bpf_usdt_spec_id(struct pt_regs *ctx)
> {
> - if (!BPF_USDT_HAS_BPF_COOKIE) {
> + if (!LINUX_HAS_BPF_COOKIE) {
> long ip = PT_REGS_IP(ctx);
> int *spec_id_ptr;
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 15:18 ` Alan Maguire
@ 2022-07-13 18:02 ` Andrii Nakryiko
0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 18:02 UTC (permalink / raw)
To: Alan Maguire
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 8:18 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 13/07/2022 02:53, Andrii Nakryiko wrote:
> > Libbpf supports single virtual __kconfig extern currently: LINUX_KERNEL_VERSION.
> > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > customly filled out by libbpf.
> >
> > This patch generalizes this approach to support more such virtual
> > __kconfig externs. One such extern added in this patch is
> > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > parts of libbpf's USDT support strictly in sync in terms of their
> > feature detection.
> >
> > We'll use similar approach for syscall wrapper detection for
> > BPF_KSYSCALL() BPF-side macro in follow up patch.
> >
> > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > extend the set of prefixes that are supported. This can be done without
> > any breaking changes, as currently any __kconfig extern with
> > unrecognized name is rejected.
> >
> > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > __weak, it is not rejected and defaults to zero. This follows
> > CONFIG_xxx handling logic and will allow BPF applications to
> > opportunistically use newer libbpf virtual externs without breaking on
> > older libbpf versions unnecessarily.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Tested the v1 patch series on arm64, all works well, so feel free to add
oh, cool, ignore my last message on your RFC reply then :)
>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
>
>
> ...for the series.
>
> I really like the concept of extending LINUX_ kconfig values.
> A few nits, but looks great!
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
> > ---
> > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > 2 files changed, 52 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index cb49408eb298..4bae67767f82 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct extern_desc *ext, __u64 v)
> > static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
> > __u64 value)
> > {
> > - if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > - pr_warn("extern (kcfg) %s=%llu should be integer\n",
> > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
> > + ext->kcfg.type != KCFG_BOOL) {
> > + pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
> > ext->name, (unsigned long long)value);
> > return -EINVAL;
> > }
> > + if (ext->kcfg.type == KCFG_BOOL && value > 1) {
> > + pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
>
> most warnings sem to conform to the format
>
> pr_warn("extern (kcfg) '%s'; value '%llu' isn't boolean\n",
>
> I find that a bit clearer but subjective I realize...
yeah, this was a bit older format, but I can update all such use cases
as well, just didn't want to distract from main changes
>
> > + ext->name, (unsigned long long)value);
> > + return -EINVAL;
> > +
> > + }
> > if (!is_kcfg_value_in_range(ext, value)) {
> > pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> > ext->name, (unsigned long long)value, ext->kcfg.sz);
> > @@ -1870,10 +1877,13 @@ static int bpf_object__process_kconfig_line(struct bpf_object *obj,
> > /* assume integer */
> > err = parse_u64(value, &num);
> > if (err) {
> > - pr_warn("extern (kcfg) %s=%s should be integer\n",
> > - ext->name, value);
> > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> > return err;
> > }
> > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
>
> I think the logic here is meant to support a KCONFIG_CHAR value that is expressed as an
> integer; if I've got this right would the error message read better as something like
>
> pr_warn("extern (kcfg) '%s': '%s' isn't an integer value\n", ext->name, value);
yeah, makes sense. and yes about KCFG_CHAR (char in general is this
weird hybrid integer/character type, but what can we do)
>
> > + return -EINVAL;
> > + }
> > err = set_kcfg_value_num(ext, ext_val, num);
> > break;
> > }
> > @@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
> > for (i = 0; i < obj->nr_extern; i++) {
> > ext = &obj->externs[i];
> >
> > - if (ext->type == EXT_KCFG &&
> > - strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > - void *ext_val = kcfg_data + ext->kcfg.data_off;
> > - __u32 kver = get_kernel_version();
> > + if (ext->type == EXT_KSYM) {
> > + if (ext->ksym.type_id)
> > + need_vmlinux_btf = true;
> > + else
> > + need_kallsyms = true;
> > + continue;
> > + } else if (ext->type == EXT_KCFG) {
> > + void *ext_ptr = kcfg_data + ext->kcfg.data_off;
> > + __u64 value = 0;
> > +
> > + /* Kconfig externs need actual /proc/config.gz */
> > + if (str_has_pfx(ext->name, "CONFIG_")) {
> > + need_config = true;
> > + continue;
> > + }
> >
> > - if (!kver) {
> > - pr_warn("failed to get kernel version\n");
> > + /* Virtual kcfg externs are customly handled by libbpf */
> > + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > + value = get_kernel_version();
> > + if (!value) {
> > + pr_warn("extern (kcfg) '%s': failed to get kernel version\n", ext->name);
> > + return -EINVAL;
> > + }
> > + } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
> > + value = kernel_supports(obj, FEAT_BPF_COOKIE);
> > + } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
> > + /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
> > + * __kconfig externs, where LINUX_ ones are virtual and filled out
> > + * customly by libbpf (their values don't come from Kconfig).
> > + * If LINUX_xxx variable is not recognized by libbpf, but is marked
> > + * __weak, it defaults to zero value, just like for CONFIG_xxx
> > + * externs.
> > + */
> > + pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n", ext->name);
> > return -EINVAL;
> > }
> > - err = set_kcfg_value_num(ext, ext_val, kver);
> > +
> > + err = set_kcfg_value_num(ext, ext_ptr, value);
> > if (err)
> > return err;
> > - pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> > - } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_")) {
> > - need_config = true;
> > - } else if (ext->type == EXT_KSYM) {
> > - if (ext->ksym.type_id)
> > - need_vmlinux_btf = true;
> > - else
> > - need_kallsyms = true;
> > + pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
>
> nit: should we use "extern (kcfg) '%s'=" as above to be consistent with warning messages?
>
will update
> > } else {
> > pr_warn("unrecognized extern '%s'\n", ext->name);
> > return -EINVAL;
> > diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> > index 4181fddb3687..4f2adc0bd6ca 100644
> > --- a/tools/lib/bpf/usdt.bpf.h
> > +++ b/tools/lib/bpf/usdt.bpf.h
> > @@ -6,7 +6,6 @@
> > #include <linux/errno.h>
> > #include <bpf/bpf_helpers.h>
> > #include <bpf/bpf_tracing.h>
> > -#include <bpf/bpf_core_read.h>
> >
> > /* Below types and maps are internal implementation details of libbpf's USDT
> > * support and are subjects to change. Also, bpf_usdt_xxx() API helpers should
> > @@ -30,14 +29,6 @@
> > #ifndef BPF_USDT_MAX_IP_CNT
> > #define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
> > #endif
> > -/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This is
> > - * the only dependency on CO-RE, so if it's undesirable, user can override
> > - * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported or not.
> > - */
> > -#ifndef BPF_USDT_HAS_BPF_COOKIE
> > -#define BPF_USDT_HAS_BPF_COOKIE \
> > - bpf_core_enum_value_exists(enum bpf_func_id___usdt, BPF_FUNC_get_attach_cookie___usdt)
> > -#endif
> >
> > enum __bpf_usdt_arg_type {
> > BPF_USDT_ARG_CONST,
> > @@ -83,15 +74,12 @@ struct {
> > __type(value, __u32);
> > } __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
> >
> > -/* don't rely on user's BPF code to have latest definition of bpf_func_id */
> > -enum bpf_func_id___usdt {
> > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > -};
> > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
> >
> > static __always_inline
> > int __bpf_usdt_spec_id(struct pt_regs *ctx)
> > {
> > - if (!BPF_USDT_HAS_BPF_COOKIE) {
> > + if (!LINUX_HAS_BPF_COOKIE) {
> > long ip = PT_REGS_IP(ctx);
> > int *spec_id_ptr;
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 1:53 ` [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT Andrii Nakryiko
2022-07-13 15:18 ` Alan Maguire
@ 2022-07-13 16:24 ` sdf
2022-07-13 18:07 ` Andrii Nakryiko
1 sibling, 1 reply; 18+ messages in thread
From: sdf @ 2022-07-13 16:24 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, kernel-team
On 07/12, Andrii Nakryiko wrote:
> Libbpf supports single virtual __kconfig extern currently:
> LINUX_KERNEL_VERSION.
> LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> customly filled out by libbpf.
> This patch generalizes this approach to support more such virtual
> __kconfig externs. One such extern added in this patch is
> LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> usdt.bpf.h instead of using CO-RE-based enum detection approach for
> detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> parts of libbpf's USDT support strictly in sync in terms of their
> feature detection.
> We'll use similar approach for syscall wrapper detection for
> BPF_KSYSCALL() BPF-side macro in follow up patch.
> Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> and LINUX_ for virtual libbpf-backed externs. In the future we might
> extend the set of prefixes that are supported. This can be done without
> any breaking changes, as currently any __kconfig extern with
> unrecognized name is rejected.
> For LINUX_xxx externs we support the normal "weak rule": if libbpf
> doesn't recognize given LINUX_xxx extern but such extern is marked as
> __weak, it is not rejected and defaults to zero. This follows
> CONFIG_xxx handling logic and will allow BPF applications to
> opportunistically use newer libbpf virtual externs without breaking on
> older libbpf versions unnecessarily.
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> tools/lib/bpf/usdt.bpf.h | 16 ++--------
> 2 files changed, 52 insertions(+), 33 deletions(-)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index cb49408eb298..4bae67767f82 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct
> extern_desc *ext, __u64 v)
> static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
> __u64 value)
> {
> - if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> - pr_warn("extern (kcfg) %s=%llu should be integer\n",
> + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
> + ext->kcfg.type != KCFG_BOOL) {
> + pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
> ext->name, (unsigned long long)value);
> return -EINVAL;
> }
> + if (ext->kcfg.type == KCFG_BOOL && value > 1) {
> + pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
> + ext->name, (unsigned long long)value);
> + return -EINVAL;
> +
> + }
> if (!is_kcfg_value_in_range(ext, value)) {
> pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> ext->name, (unsigned long long)value, ext->kcfg.sz);
> @@ -1870,10 +1877,13 @@ static int
> bpf_object__process_kconfig_line(struct bpf_object *obj,
> /* assume integer */
> err = parse_u64(value, &num);
> if (err) {
> - pr_warn("extern (kcfg) %s=%s should be integer\n",
> - ext->name, value);
> + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> return err;
> }
> + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> + return -EINVAL;
> + }
> err = set_kcfg_value_num(ext, ext_val, num);
> break;
> }
> @@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct
> bpf_object *obj,
> for (i = 0; i < obj->nr_extern; i++) {
> ext = &obj->externs[i];
> - if (ext->type == EXT_KCFG &&
> - strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> - void *ext_val = kcfg_data + ext->kcfg.data_off;
> - __u32 kver = get_kernel_version();
> + if (ext->type == EXT_KSYM) {
> + if (ext->ksym.type_id)
> + need_vmlinux_btf = true;
> + else
> + need_kallsyms = true;
> + continue;
> + } else if (ext->type == EXT_KCFG) {
> + void *ext_ptr = kcfg_data + ext->kcfg.data_off;
> + __u64 value = 0;
> +
> + /* Kconfig externs need actual /proc/config.gz */
> + if (str_has_pfx(ext->name, "CONFIG_")) {
> + need_config = true;
> + continue;
> + }
> - if (!kver) {
> - pr_warn("failed to get kernel version\n");
> + /* Virtual kcfg externs are customly handled by libbpf */
> + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> + value = get_kernel_version();
> + if (!value) {
> + pr_warn("extern (kcfg) '%s': failed to get kernel version\n",
> ext->name);
> + return -EINVAL;
> + }
> + } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
> + value = kernel_supports(obj, FEAT_BPF_COOKIE);
> + } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
> + /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
> + * __kconfig externs, where LINUX_ ones are virtual and filled out
> + * customly by libbpf (their values don't come from Kconfig).
> + * If LINUX_xxx variable is not recognized by libbpf, but is marked
> + * __weak, it defaults to zero value, just like for CONFIG_xxx
> + * externs.
> + */
> + pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n",
> ext->name);
> return -EINVAL;
> }
> - err = set_kcfg_value_num(ext, ext_val, kver);
> +
> + err = set_kcfg_value_num(ext, ext_ptr, value);
> if (err)
> return err;
> - pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> - } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_"))
> {
> - need_config = true;
> - } else if (ext->type == EXT_KSYM) {
> - if (ext->ksym.type_id)
> - need_vmlinux_btf = true;
> - else
> - need_kallsyms = true;
> + pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
> } else {
> pr_warn("unrecognized extern '%s'\n", ext->name);
> return -EINVAL;
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index 4181fddb3687..4f2adc0bd6ca 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -6,7 +6,6 @@
> #include <linux/errno.h>
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> -#include <bpf/bpf_core_read.h>
> /* Below types and maps are internal implementation details of libbpf's
> USDT
> * support and are subjects to change. Also, bpf_usdt_xxx() API helpers
> should
> @@ -30,14 +29,6 @@
> #ifndef BPF_USDT_MAX_IP_CNT
> #define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
> #endif
> -/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This
> is
> - * the only dependency on CO-RE, so if it's undesirable, user can
> override
> - * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported
> or not.
> - */
> -#ifndef BPF_USDT_HAS_BPF_COOKIE
> -#define BPF_USDT_HAS_BPF_COOKIE \
> - bpf_core_enum_value_exists(enum bpf_func_id___usdt,
> BPF_FUNC_get_attach_cookie___usdt)
> -#endif
> enum __bpf_usdt_arg_type {
> BPF_USDT_ARG_CONST,
> @@ -83,15 +74,12 @@ struct {
> __type(value, __u32);
> } __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
> -/* don't rely on user's BPF code to have latest definition of
> bpf_func_id */
> -enum bpf_func_id___usdt {
> - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> -};
> +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
Could _Bool be a problem when used by c++? I remember that we can have
sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
use 'int' instead I'm assuming we'll loose all the niceties of
KCFG_BOOL?
Or shouldn't be a problem since it's not part of C/C++ api boundary?
> static __always_inline
> int __bpf_usdt_spec_id(struct pt_regs *ctx)
> {
> - if (!BPF_USDT_HAS_BPF_COOKIE) {
> + if (!LINUX_HAS_BPF_COOKIE) {
> long ip = PT_REGS_IP(ctx);
> int *spec_id_ptr;
> --
> 2.30.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 16:24 ` sdf
@ 2022-07-13 18:07 ` Andrii Nakryiko
2022-07-13 18:57 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 18:07 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 9:24 AM <sdf@google.com> wrote:
>
> On 07/12, Andrii Nakryiko wrote:
> > Libbpf supports single virtual __kconfig extern currently:
> > LINUX_KERNEL_VERSION.
> > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > customly filled out by libbpf.
>
> > This patch generalizes this approach to support more such virtual
> > __kconfig externs. One such extern added in this patch is
> > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > parts of libbpf's USDT support strictly in sync in terms of their
> > feature detection.
>
> > We'll use similar approach for syscall wrapper detection for
> > BPF_KSYSCALL() BPF-side macro in follow up patch.
>
> > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > extend the set of prefixes that are supported. This can be done without
> > any breaking changes, as currently any __kconfig extern with
> > unrecognized name is rejected.
>
> > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > __weak, it is not rejected and defaults to zero. This follows
> > CONFIG_xxx handling logic and will allow BPF applications to
> > opportunistically use newer libbpf virtual externs without breaking on
> > older libbpf versions unnecessarily.
>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > 2 files changed, 52 insertions(+), 33 deletions(-)
>
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index cb49408eb298..4bae67767f82 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct
> > extern_desc *ext, __u64 v)
> > static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
> > __u64 value)
> > {
> > - if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > - pr_warn("extern (kcfg) %s=%llu should be integer\n",
> > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
> > + ext->kcfg.type != KCFG_BOOL) {
> > + pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
> > ext->name, (unsigned long long)value);
> > return -EINVAL;
> > }
> > + if (ext->kcfg.type == KCFG_BOOL && value > 1) {
> > + pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
> > + ext->name, (unsigned long long)value);
> > + return -EINVAL;
> > +
> > + }
> > if (!is_kcfg_value_in_range(ext, value)) {
> > pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> > ext->name, (unsigned long long)value, ext->kcfg.sz);
> > @@ -1870,10 +1877,13 @@ static int
> > bpf_object__process_kconfig_line(struct bpf_object *obj,
> > /* assume integer */
> > err = parse_u64(value, &num);
> > if (err) {
> > - pr_warn("extern (kcfg) %s=%s should be integer\n",
> > - ext->name, value);
> > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> > return err;
> > }
> > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> > + return -EINVAL;
> > + }
> > err = set_kcfg_value_num(ext, ext_val, num);
> > break;
> > }
> > @@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct
> > bpf_object *obj,
> > for (i = 0; i < obj->nr_extern; i++) {
> > ext = &obj->externs[i];
>
> > - if (ext->type == EXT_KCFG &&
> > - strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > - void *ext_val = kcfg_data + ext->kcfg.data_off;
> > - __u32 kver = get_kernel_version();
> > + if (ext->type == EXT_KSYM) {
> > + if (ext->ksym.type_id)
> > + need_vmlinux_btf = true;
> > + else
> > + need_kallsyms = true;
> > + continue;
> > + } else if (ext->type == EXT_KCFG) {
> > + void *ext_ptr = kcfg_data + ext->kcfg.data_off;
> > + __u64 value = 0;
> > +
> > + /* Kconfig externs need actual /proc/config.gz */
> > + if (str_has_pfx(ext->name, "CONFIG_")) {
> > + need_config = true;
> > + continue;
> > + }
>
> > - if (!kver) {
> > - pr_warn("failed to get kernel version\n");
> > + /* Virtual kcfg externs are customly handled by libbpf */
> > + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > + value = get_kernel_version();
> > + if (!value) {
> > + pr_warn("extern (kcfg) '%s': failed to get kernel version\n",
> > ext->name);
> > + return -EINVAL;
> > + }
> > + } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
> > + value = kernel_supports(obj, FEAT_BPF_COOKIE);
> > + } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
> > + /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
> > + * __kconfig externs, where LINUX_ ones are virtual and filled out
> > + * customly by libbpf (their values don't come from Kconfig).
> > + * If LINUX_xxx variable is not recognized by libbpf, but is marked
> > + * __weak, it defaults to zero value, just like for CONFIG_xxx
> > + * externs.
> > + */
> > + pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n",
> > ext->name);
> > return -EINVAL;
> > }
> > - err = set_kcfg_value_num(ext, ext_val, kver);
> > +
> > + err = set_kcfg_value_num(ext, ext_ptr, value);
> > if (err)
> > return err;
> > - pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> > - } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_"))
> > {
> > - need_config = true;
> > - } else if (ext->type == EXT_KSYM) {
> > - if (ext->ksym.type_id)
> > - need_vmlinux_btf = true;
> > - else
> > - need_kallsyms = true;
> > + pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
> > } else {
> > pr_warn("unrecognized extern '%s'\n", ext->name);
> > return -EINVAL;
> > diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> > index 4181fddb3687..4f2adc0bd6ca 100644
> > --- a/tools/lib/bpf/usdt.bpf.h
> > +++ b/tools/lib/bpf/usdt.bpf.h
> > @@ -6,7 +6,6 @@
> > #include <linux/errno.h>
> > #include <bpf/bpf_helpers.h>
> > #include <bpf/bpf_tracing.h>
> > -#include <bpf/bpf_core_read.h>
>
> > /* Below types and maps are internal implementation details of libbpf's
> > USDT
> > * support and are subjects to change. Also, bpf_usdt_xxx() API helpers
> > should
> > @@ -30,14 +29,6 @@
> > #ifndef BPF_USDT_MAX_IP_CNT
> > #define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
> > #endif
> > -/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This
> > is
> > - * the only dependency on CO-RE, so if it's undesirable, user can
> > override
> > - * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported
> > or not.
> > - */
> > -#ifndef BPF_USDT_HAS_BPF_COOKIE
> > -#define BPF_USDT_HAS_BPF_COOKIE \
> > - bpf_core_enum_value_exists(enum bpf_func_id___usdt,
> > BPF_FUNC_get_attach_cookie___usdt)
> > -#endif
>
> > enum __bpf_usdt_arg_type {
> > BPF_USDT_ARG_CONST,
> > @@ -83,15 +74,12 @@ struct {
> > __type(value, __u32);
> > } __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
>
> > -/* don't rely on user's BPF code to have latest definition of
> > bpf_func_id */
> > -enum bpf_func_id___usdt {
> > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > -};
> > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
>
> Could _Bool be a problem when used by c++? I remember that we can have
> sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
> use 'int' instead I'm assuming we'll loose all the niceties of
> KCFG_BOOL?
>
> Or shouldn't be a problem since it's not part of C/C++ api boundary?
I actually don't know if C++ supports "_Bool", but in C, "bool" is an
alias to _Bool. _Bool is *the type* of the boolean. The benefit of
_Bool is that it doesn't require including stdbool.h, while "bool"
itself needs extra header. So I try not to use bool in libbpf BPF API
headers just to avoid extra header dependencies.
But it shouldn't matter as this is BPF-side code, so it has to be
compiled in C mode by Clang/GCC, so _Bool should always be there.
As for the size it seems like it's not even specified by the standard
that sizeof(bool/_Bool) is 1, though it is in practice. I only
remember some very-very-very old versions of Microsoft's Visual C++
having sizeof(bool) == 4, but then they changed that anyways to
sizeof(bool) == 1 (it was many-many years ago, so it might be an
incomplete story).
But either way it doesn't matter, because libbpf will support any
size: 1, 2, 4, 8 and will just set 1 for true, 0 for false, with
correct zero extension to match variable size.
As for bool vs int, no real difference, but it is true/false
conceptually, so seems cleaner to use bool. But using int will work
just fine here as well (you still get 0 or 1 for both, effectively).
>
> > static __always_inline
> > int __bpf_usdt_spec_id(struct pt_regs *ctx)
> > {
> > - if (!BPF_USDT_HAS_BPF_COOKIE) {
> > + if (!LINUX_HAS_BPF_COOKIE) {
> > long ip = PT_REGS_IP(ctx);
> > int *spec_id_ptr;
>
> > --
> > 2.30.2
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 18:07 ` Andrii Nakryiko
@ 2022-07-13 18:57 ` Stanislav Fomichev
2022-07-13 20:26 ` Andrii Nakryiko
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-13 18:57 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 11:08 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 9:24 AM <sdf@google.com> wrote:
> >
> > On 07/12, Andrii Nakryiko wrote:
> > > Libbpf supports single virtual __kconfig extern currently:
> > > LINUX_KERNEL_VERSION.
> > > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > > customly filled out by libbpf.
> >
> > > This patch generalizes this approach to support more such virtual
> > > __kconfig externs. One such extern added in this patch is
> > > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > > parts of libbpf's USDT support strictly in sync in terms of their
> > > feature detection.
> >
> > > We'll use similar approach for syscall wrapper detection for
> > > BPF_KSYSCALL() BPF-side macro in follow up patch.
> >
> > > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > > extend the set of prefixes that are supported. This can be done without
> > > any breaking changes, as currently any __kconfig extern with
> > > unrecognized name is rejected.
> >
> > > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > > __weak, it is not rejected and defaults to zero. This follows
> > > CONFIG_xxx handling logic and will allow BPF applications to
> > > opportunistically use newer libbpf virtual externs without breaking on
> > > older libbpf versions unnecessarily.
> >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > > 2 files changed, 52 insertions(+), 33 deletions(-)
> >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index cb49408eb298..4bae67767f82 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -1800,11 +1800,18 @@ static bool is_kcfg_value_in_range(const struct
> > > extern_desc *ext, __u64 v)
> > > static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
> > > __u64 value)
> > > {
> > > - if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > > - pr_warn("extern (kcfg) %s=%llu should be integer\n",
> > > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR &&
> > > + ext->kcfg.type != KCFG_BOOL) {
> > > + pr_warn("extern (kcfg) %s=%llu should be integer, char or boolean\n",
> > > ext->name, (unsigned long long)value);
> > > return -EINVAL;
> > > }
> > > + if (ext->kcfg.type == KCFG_BOOL && value > 1) {
> > > + pr_warn("extern (kcfg) %s=%llu value isn't boolean\n",
> > > + ext->name, (unsigned long long)value);
> > > + return -EINVAL;
> > > +
> > > + }
> > > if (!is_kcfg_value_in_range(ext, value)) {
> > > pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> > > ext->name, (unsigned long long)value, ext->kcfg.sz);
> > > @@ -1870,10 +1877,13 @@ static int
> > > bpf_object__process_kconfig_line(struct bpf_object *obj,
> > > /* assume integer */
> > > err = parse_u64(value, &num);
> > > if (err) {
> > > - pr_warn("extern (kcfg) %s=%s should be integer\n",
> > > - ext->name, value);
> > > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> > > return err;
> > > }
> > > + if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
> > > + pr_warn("extern (kcfg) %s=%s should be integer\n", ext->name, value);
> > > + return -EINVAL;
> > > + }
> > > err = set_kcfg_value_num(ext, ext_val, num);
> > > break;
> > > }
> > > @@ -7493,26 +7503,47 @@ static int bpf_object__resolve_externs(struct
> > > bpf_object *obj,
> > > for (i = 0; i < obj->nr_extern; i++) {
> > > ext = &obj->externs[i];
> >
> > > - if (ext->type == EXT_KCFG &&
> > > - strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > > - void *ext_val = kcfg_data + ext->kcfg.data_off;
> > > - __u32 kver = get_kernel_version();
> > > + if (ext->type == EXT_KSYM) {
> > > + if (ext->ksym.type_id)
> > > + need_vmlinux_btf = true;
> > > + else
> > > + need_kallsyms = true;
> > > + continue;
> > > + } else if (ext->type == EXT_KCFG) {
> > > + void *ext_ptr = kcfg_data + ext->kcfg.data_off;
> > > + __u64 value = 0;
> > > +
> > > + /* Kconfig externs need actual /proc/config.gz */
> > > + if (str_has_pfx(ext->name, "CONFIG_")) {
> > > + need_config = true;
> > > + continue;
> > > + }
> >
> > > - if (!kver) {
> > > - pr_warn("failed to get kernel version\n");
> > > + /* Virtual kcfg externs are customly handled by libbpf */
> > > + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > > + value = get_kernel_version();
> > > + if (!value) {
> > > + pr_warn("extern (kcfg) '%s': failed to get kernel version\n",
> > > ext->name);
> > > + return -EINVAL;
> > > + }
> > > + } else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
> > > + value = kernel_supports(obj, FEAT_BPF_COOKIE);
> > > + } else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
> > > + /* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
> > > + * __kconfig externs, where LINUX_ ones are virtual and filled out
> > > + * customly by libbpf (their values don't come from Kconfig).
> > > + * If LINUX_xxx variable is not recognized by libbpf, but is marked
> > > + * __weak, it defaults to zero value, just like for CONFIG_xxx
> > > + * externs.
> > > + */
> > > + pr_warn("extern (kcfg) '%s': unrecognized virtual extern\n",
> > > ext->name);
> > > return -EINVAL;
> > > }
> > > - err = set_kcfg_value_num(ext, ext_val, kver);
> > > +
> > > + err = set_kcfg_value_num(ext, ext_ptr, value);
> > > if (err)
> > > return err;
> > > - pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> > > - } else if (ext->type == EXT_KCFG && str_has_pfx(ext->name, "CONFIG_"))
> > > {
> > > - need_config = true;
> > > - } else if (ext->type == EXT_KSYM) {
> > > - if (ext->ksym.type_id)
> > > - need_vmlinux_btf = true;
> > > - else
> > > - need_kallsyms = true;
> > > + pr_debug("extern (kcfg) %s=0x%llx\n", ext->name, (long long)value);
> > > } else {
> > > pr_warn("unrecognized extern '%s'\n", ext->name);
> > > return -EINVAL;
> > > diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> > > index 4181fddb3687..4f2adc0bd6ca 100644
> > > --- a/tools/lib/bpf/usdt.bpf.h
> > > +++ b/tools/lib/bpf/usdt.bpf.h
> > > @@ -6,7 +6,6 @@
> > > #include <linux/errno.h>
> > > #include <bpf/bpf_helpers.h>
> > > #include <bpf/bpf_tracing.h>
> > > -#include <bpf/bpf_core_read.h>
> >
> > > /* Below types and maps are internal implementation details of libbpf's
> > > USDT
> > > * support and are subjects to change. Also, bpf_usdt_xxx() API helpers
> > > should
> > > @@ -30,14 +29,6 @@
> > > #ifndef BPF_USDT_MAX_IP_CNT
> > > #define BPF_USDT_MAX_IP_CNT (4 * BPF_USDT_MAX_SPEC_CNT)
> > > #endif
> > > -/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This
> > > is
> > > - * the only dependency on CO-RE, so if it's undesirable, user can
> > > override
> > > - * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported
> > > or not.
> > > - */
> > > -#ifndef BPF_USDT_HAS_BPF_COOKIE
> > > -#define BPF_USDT_HAS_BPF_COOKIE \
> > > - bpf_core_enum_value_exists(enum bpf_func_id___usdt,
> > > BPF_FUNC_get_attach_cookie___usdt)
> > > -#endif
> >
> > > enum __bpf_usdt_arg_type {
> > > BPF_USDT_ARG_CONST,
> > > @@ -83,15 +74,12 @@ struct {
> > > __type(value, __u32);
> > > } __bpf_usdt_ip_to_spec_id SEC(".maps") __weak;
> >
> > > -/* don't rely on user's BPF code to have latest definition of
> > > bpf_func_id */
> > > -enum bpf_func_id___usdt {
> > > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > > -};
> > > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
> >
> > Could _Bool be a problem when used by c++? I remember that we can have
> > sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
> > use 'int' instead I'm assuming we'll loose all the niceties of
> > KCFG_BOOL?
> >
> > Or shouldn't be a problem since it's not part of C/C++ api boundary?
>
> I actually don't know if C++ supports "_Bool", but in C, "bool" is an
> alias to _Bool. _Bool is *the type* of the boolean. The benefit of
> _Bool is that it doesn't require including stdbool.h, while "bool"
> itself needs extra header. So I try not to use bool in libbpf BPF API
> headers just to avoid extra header dependencies.
>
> But it shouldn't matter as this is BPF-side code, so it has to be
> compiled in C mode by Clang/GCC, so _Bool should always be there.
The program is fine, but these _Bool's can now leak into generated
skeletons, right?
And skeletons might be included into c++ and I'm not sure what happens
with _Bool over there.
My naive attempt to use it gives me the following:
$ cat tst.cc
int main(int argc, char *argv[])
{
_Bool have_args = argc > 1;
if (have_args)
return 1;
else
return 0;
}
$ clang++ tst.cc
tst.cc:3:5: error: unknown type name '_Bool'
_Bool have_args = argc > 1;
^
1 error generated.
> As for the size it seems like it's not even specified by the standard
> that sizeof(bool/_Bool) is 1, though it is in practice. I only
> remember some very-very-very old versions of Microsoft's Visual C++
> having sizeof(bool) == 4, but then they changed that anyways to
> sizeof(bool) == 1 (it was many-many years ago, so it might be an
> incomplete story).
>
> But either way it doesn't matter, because libbpf will support any
> size: 1, 2, 4, 8 and will just set 1 for true, 0 for false, with
> correct zero extension to match variable size.
>
> As for bool vs int, no real difference, but it is true/false
> conceptually, so seems cleaner to use bool. But using int will work
> just fine here as well (you still get 0 or 1 for both, effectively).
Yeah, so my question is more like: rather than trying to figure out if
_Bool is safe, might it be easier to stick to ints?
> > > static __always_inline
> > > int __bpf_usdt_spec_id(struct pt_regs *ctx)
> > > {
> > > - if (!BPF_USDT_HAS_BPF_COOKIE) {
> > > + if (!LINUX_HAS_BPF_COOKIE) {
> > > long ip = PT_REGS_IP(ctx);
> > > int *spec_id_ptr;
> >
> > > --
> > > 2.30.2
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 18:57 ` Stanislav Fomichev
@ 2022-07-13 20:26 ` Andrii Nakryiko
2022-07-13 21:14 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 20:26 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Wed, Jul 13, 2022 at 11:08 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 9:24 AM <sdf@google.com> wrote:
> > >
> > > On 07/12, Andrii Nakryiko wrote:
> > > > Libbpf supports single virtual __kconfig extern currently:
> > > > LINUX_KERNEL_VERSION.
> > > > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > > > customly filled out by libbpf.
> > >
> > > > This patch generalizes this approach to support more such virtual
> > > > __kconfig externs. One such extern added in this patch is
> > > > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > > > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > > > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > > > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > > > parts of libbpf's USDT support strictly in sync in terms of their
> > > > feature detection.
> > >
> > > > We'll use similar approach for syscall wrapper detection for
> > > > BPF_KSYSCALL() BPF-side macro in follow up patch.
> > >
> > > > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > > > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > > > extend the set of prefixes that are supported. This can be done without
> > > > any breaking changes, as currently any __kconfig extern with
> > > > unrecognized name is rejected.
> > >
> > > > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > > > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > > > __weak, it is not rejected and defaults to zero. This follows
> > > > CONFIG_xxx handling logic and will allow BPF applications to
> > > > opportunistically use newer libbpf virtual externs without breaking on
> > > > older libbpf versions unnecessarily.
> > >
> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > > ---
> > > > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > > > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > > > 2 files changed, 52 insertions(+), 33 deletions(-)
> > >
[...]
> > >
> > > > -/* don't rely on user's BPF code to have latest definition of
> > > > bpf_func_id */
> > > > -enum bpf_func_id___usdt {
> > > > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > > > -};
> > > > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
> > >
> > > Could _Bool be a problem when used by c++? I remember that we can have
> > > sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
> > > use 'int' instead I'm assuming we'll loose all the niceties of
> > > KCFG_BOOL?
> > >
> > > Or shouldn't be a problem since it's not part of C/C++ api boundary?
> >
> > I actually don't know if C++ supports "_Bool", but in C, "bool" is an
> > alias to _Bool. _Bool is *the type* of the boolean. The benefit of
> > _Bool is that it doesn't require including stdbool.h, while "bool"
> > itself needs extra header. So I try not to use bool in libbpf BPF API
> > headers just to avoid extra header dependencies.
> >
> > But it shouldn't matter as this is BPF-side code, so it has to be
> > compiled in C mode by Clang/GCC, so _Bool should always be there.
>
> The program is fine, but these _Bool's can now leak into generated
> skeletons, right?
> And skeletons might be included into c++ and I'm not sure what happens
> with _Bool over there.
>
> My naive attempt to use it gives me the following:
>
> $ cat tst.cc
> int main(int argc, char *argv[])
> {
> _Bool have_args = argc > 1;
> if (have_args)
> return 1;
> else
> return 0;
> }
> $ clang++ tst.cc
> tst.cc:3:5: error: unknown type name '_Bool'
> _Bool have_args = argc > 1;
> ^
> 1 error generated.
Check test_cpp.cpp, it includes test_core_extern.skel.h, which has
bool externs in it. In generated code those bools are emitted as _Bool
(because that's what gets emitted to BTF by Clang). It seems to be
working fine with g++ already, and I just confirmed that clang++ also
works. So not sure what's different (perhaps stdbool.h included
somewhere "fixes" this for C++), but I don't it's the problem.
>
> > As for the size it seems like it's not even specified by the standard
> > that sizeof(bool/_Bool) is 1, though it is in practice. I only
> > remember some very-very-very old versions of Microsoft's Visual C++
> > having sizeof(bool) == 4, but then they changed that anyways to
> > sizeof(bool) == 1 (it was many-many years ago, so it might be an
> > incomplete story).
> >
> > But either way it doesn't matter, because libbpf will support any
> > size: 1, 2, 4, 8 and will just set 1 for true, 0 for false, with
> > correct zero extension to match variable size.
> >
> > As for bool vs int, no real difference, but it is true/false
> > conceptually, so seems cleaner to use bool. But using int will work
> > just fine here as well (you still get 0 or 1 for both, effectively).
>
> Yeah, so my question is more like: rather than trying to figure out if
> _Bool is safe, might it be easier to stick to ints?
>
I don't mind, but seems like it should be fine. It's not the first
bool used in skeleton, so if it didn't work, we'd be already suffering
and fixing it with some other means.
>
> > > > static __always_inline
> > > > int __bpf_usdt_spec_id(struct pt_regs *ctx)
> > > > {
> > > > - if (!BPF_USDT_HAS_BPF_COOKIE) {
> > > > + if (!LINUX_HAS_BPF_COOKIE) {
> > > > long ip = PT_REGS_IP(ctx);
> > > > int *spec_id_ptr;
> > >
> > > > --
> > > > 2.30.2
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT
2022-07-13 20:26 ` Andrii Nakryiko
@ 2022-07-13 21:14 ` Stanislav Fomichev
0 siblings, 0 replies; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-13 21:14 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 1:26 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 11:08 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 9:24 AM <sdf@google.com> wrote:
> > > >
> > > > On 07/12, Andrii Nakryiko wrote:
> > > > > Libbpf supports single virtual __kconfig extern currently:
> > > > > LINUX_KERNEL_VERSION.
> > > > > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > > > > customly filled out by libbpf.
> > > >
> > > > > This patch generalizes this approach to support more such virtual
> > > > > __kconfig externs. One such extern added in this patch is
> > > > > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > > > > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > > > > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > > > > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > > > > parts of libbpf's USDT support strictly in sync in terms of their
> > > > > feature detection.
> > > >
> > > > > We'll use similar approach for syscall wrapper detection for
> > > > > BPF_KSYSCALL() BPF-side macro in follow up patch.
> > > >
> > > > > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > > > > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > > > > extend the set of prefixes that are supported. This can be done without
> > > > > any breaking changes, as currently any __kconfig extern with
> > > > > unrecognized name is rejected.
> > > >
> > > > > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > > > > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > > > > __weak, it is not rejected and defaults to zero. This follows
> > > > > CONFIG_xxx handling logic and will allow BPF applications to
> > > > > opportunistically use newer libbpf virtual externs without breaking on
> > > > > older libbpf versions unnecessarily.
> > > >
> > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > ---
> > > > > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > > > > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > > > > 2 files changed, 52 insertions(+), 33 deletions(-)
> > > >
>
> [...]
>
> > > >
> > > > > -/* don't rely on user's BPF code to have latest definition of
> > > > > bpf_func_id */
> > > > > -enum bpf_func_id___usdt {
> > > > > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > > > > -};
> > > > > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
> > > >
> > > > Could _Bool be a problem when used by c++? I remember that we can have
> > > > sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
> > > > use 'int' instead I'm assuming we'll loose all the niceties of
> > > > KCFG_BOOL?
> > > >
> > > > Or shouldn't be a problem since it's not part of C/C++ api boundary?
> > >
> > > I actually don't know if C++ supports "_Bool", but in C, "bool" is an
> > > alias to _Bool. _Bool is *the type* of the boolean. The benefit of
> > > _Bool is that it doesn't require including stdbool.h, while "bool"
> > > itself needs extra header. So I try not to use bool in libbpf BPF API
> > > headers just to avoid extra header dependencies.
> > >
> > > But it shouldn't matter as this is BPF-side code, so it has to be
> > > compiled in C mode by Clang/GCC, so _Bool should always be there.
> >
> > The program is fine, but these _Bool's can now leak into generated
> > skeletons, right?
> > And skeletons might be included into c++ and I'm not sure what happens
> > with _Bool over there.
> >
> > My naive attempt to use it gives me the following:
> >
> > $ cat tst.cc
> > int main(int argc, char *argv[])
> > {
> > _Bool have_args = argc > 1;
> > if (have_args)
> > return 1;
> > else
> > return 0;
> > }
> > $ clang++ tst.cc
> > tst.cc:3:5: error: unknown type name '_Bool'
> > _Bool have_args = argc > 1;
> > ^
> > 1 error generated.
>
> Check test_cpp.cpp, it includes test_core_extern.skel.h, which has
> bool externs in it. In generated code those bools are emitted as _Bool
> (because that's what gets emitted to BTF by Clang). It seems to be
> working fine with g++ already, and I just confirmed that clang++ also
> works. So not sure what's different (perhaps stdbool.h included
> somewhere "fixes" this for C++), but I don't it's the problem.
Oh, didn't know we already export them. Looks like we do include
stdbool.h eventually and it does:
#define _Bool bool
In this case, yeah, ignore me, I was thinking that it's the first
_Bool being "exported".
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH bpf-next 2/5] selftests/bpf: add test of __weak unknown virtual __kconfig extern
2022-07-13 1:52 [PATCH bpf-next 0/5] Add SEC("ksyscall") support Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT Andrii Nakryiko
@ 2022-07-13 1:53 ` Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 3/5] libbpf: improve BPF_KPROBE_SYSCALL macro and rename it to BPF_KSYSCALL Andrii Nakryiko
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 1:53 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
Exercise libbpf's logic for unknown __weak virtual __kconfig externs.
USDT selftests are already excercising non-weak known virtual extern
already (LINUX_HAS_BPF_COOKIE), so no need to add explicit tests for it.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/core_extern.c | 17 +++++++----------
.../selftests/bpf/progs/test_core_extern.c | 3 +++
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/core_extern.c b/tools/testing/selftests/bpf/prog_tests/core_extern.c
index 1931a158510e..63a51e9f3630 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_extern.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_extern.c
@@ -39,6 +39,7 @@ static struct test_case {
"CONFIG_STR=\"abracad\"\n"
"CONFIG_MISSING=0",
.data = {
+ .unkn_virt_val = 0,
.bpf_syscall = false,
.tristate_val = TRI_MODULE,
.bool_val = true,
@@ -121,7 +122,7 @@ static struct test_case {
void test_core_extern(void)
{
const uint32_t kern_ver = get_kernel_version();
- int err, duration = 0, i, j;
+ int err, i, j;
struct test_core_extern *skel = NULL;
uint64_t *got, *exp;
int n = sizeof(*skel->data) / sizeof(uint64_t);
@@ -136,19 +137,17 @@ void test_core_extern(void)
continue;
skel = test_core_extern__open_opts(&opts);
- if (CHECK(!skel, "skel_open", "skeleton open failed\n"))
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
goto cleanup;
err = test_core_extern__load(skel);
if (t->fails) {
- CHECK(!err, "skel_load",
- "shouldn't succeed open/load of skeleton\n");
+ ASSERT_ERR(err, "skel_load_should_fail");
goto cleanup;
- } else if (CHECK(err, "skel_load",
- "failed to open/load skeleton\n")) {
+ } else if (!ASSERT_OK(err, "skel_load")) {
goto cleanup;
}
err = test_core_extern__attach(skel);
- if (CHECK(err, "attach_raw_tp", "failed attach: %d\n", err))
+ if (!ASSERT_OK(err, "attach_raw_tp"))
goto cleanup;
usleep(1);
@@ -158,9 +157,7 @@ void test_core_extern(void)
got = (uint64_t *)skel->data;
exp = (uint64_t *)&t->data;
for (j = 0; j < n; j++) {
- CHECK(got[j] != exp[j], "check_res",
- "result #%d: expected %llx, but got %llx\n",
- j, (__u64)exp[j], (__u64)got[j]);
+ ASSERT_EQ(got[j], exp[j], "result");
}
cleanup:
test_core_extern__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/test_core_extern.c b/tools/testing/selftests/bpf/progs/test_core_extern.c
index 3ac3603ad53d..a3c7c1042f35 100644
--- a/tools/testing/selftests/bpf/progs/test_core_extern.c
+++ b/tools/testing/selftests/bpf/progs/test_core_extern.c
@@ -11,6 +11,7 @@
static int (*bpf_missing_helper)(const void *arg1, int arg2) = (void *) 999;
extern int LINUX_KERNEL_VERSION __kconfig;
+extern int LINUX_UNKNOWN_VIRTUAL_EXTERN __kconfig __weak;
extern bool CONFIG_BPF_SYSCALL __kconfig; /* strong */
extern enum libbpf_tristate CONFIG_TRISTATE __kconfig __weak;
extern bool CONFIG_BOOL __kconfig __weak;
@@ -22,6 +23,7 @@ extern const char CONFIG_STR[8] __kconfig __weak;
extern uint64_t CONFIG_MISSING __kconfig __weak;
uint64_t kern_ver = -1;
+uint64_t unkn_virt_val = -1;
uint64_t bpf_syscall = -1;
uint64_t tristate_val = -1;
uint64_t bool_val = -1;
@@ -38,6 +40,7 @@ int handle_sys_enter(struct pt_regs *ctx)
int i;
kern_ver = LINUX_KERNEL_VERSION;
+ unkn_virt_val = LINUX_UNKNOWN_VIRTUAL_EXTERN;
bpf_syscall = CONFIG_BPF_SYSCALL;
tristate_val = CONFIG_TRISTATE;
bool_val = CONFIG_BOOL;
--
2.30.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH bpf-next 3/5] libbpf: improve BPF_KPROBE_SYSCALL macro and rename it to BPF_KSYSCALL
2022-07-13 1:52 [PATCH bpf-next 0/5] Add SEC("ksyscall") support Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 1/5] libbpf: generalize virtual __kconfig externs and use it for USDT Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 2/5] selftests/bpf: add test of __weak unknown virtual __kconfig extern Andrii Nakryiko
@ 2022-07-13 1:53 ` Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 4/5] libbpf: add ksyscall/kretsyscall sections support for syscall kprobes Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests Andrii Nakryiko
4 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 1:53 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
Improve BPF_KPROBE_SYSCALL (and rename it to shorter BPF_KSYSCALL to
match libbpf's SEC("ksyscall") section name, added in next patch) to use
__kconfig variable to determine how to properly fetch syscall arguments.
Instead of relying on hard-coded knowledge of whether kernel's
architecture uses syscall wrapper or not (which only reflects the latest
kernel versions, but is not necessarily true for older kernels and won't
necessarily hold for later kernel versions on some particular host
architecture), determine this at runtime by attempting to create
perf_event (with fallback to kprobe event creation through tracefs on
legacy kernels, just like kprobe attachment code is doing) for kernel
function that would correspond to bpf() syscall on a system that has
CONFIG_ARCH_HAS_SYSCALL_WRAPPER set (e.g., for x86-64 it would try
'__x64_sys_bpf').
If host kernel uses syscall wrapper, syscall kernel function's first
argument is a pointer to struct pt_regs that then contains syscall
arguments. In such case we need to use bpf_probe_read_kernel() to fetch
actual arguments (which we do through BPF_CORE_READ() macro) from inner
pt_regs.
But if the kernel doesn't use syscall wrapper approach, input
arguments can be read from struct pt_regs directly with no probe reading.
All this feature detection is done without requiring /proc/config.gz
existence and parsing, and BPF-side helper code uses newly added
LINUX_HAS_SYSCALL_WRAPPER virtual __kconfig extern to keep in sync with
user-side feature detection of libbpf.
BPF_KSYSCALL() macro can be used both with SEC("kprobe") programs that
define syscall function explicitly (e.g., SEC("kprobe/__x64_sys_bpf"))
and SEC("ksyscall") program added in the next patch (which are the same
kprobe program with added benefit of libbpf determining correct kernel
function name automatically).
Kretprobe and kretsyscall (added in next patch) programs don't need
BPF_KSYSCALL as they don't provide access to input arguments. Normal
BPF_KRETPROBE is completely sufficient and is recommended.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_tracing.h | 51 +++++++++++++++++++++++++++----------
tools/lib/bpf/libbpf.c | 2 ++
2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 11f9096407fc..f4d3e1e2abe2 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -2,6 +2,8 @@
#ifndef __BPF_TRACING_H__
#define __BPF_TRACING_H__
+#include <bpf/bpf_helpers.h>
+
/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
#if defined(__TARGET_ARCH_x86)
#define bpf_target_x86
@@ -140,7 +142,7 @@ struct pt_regs___s390 {
#define __PT_RC_REG gprs[2]
#define __PT_SP_REG gprs[15]
#define __PT_IP_REG psw.addr
-#define PT_REGS_PARM1_SYSCALL(x) ({ _Pragma("GCC error \"use PT_REGS_PARM1_CORE_SYSCALL() instead\""); 0l; })
+#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
#define PT_REGS_PARM1_CORE_SYSCALL(x) BPF_CORE_READ((const struct pt_regs___s390 *)(x), orig_gpr2)
#elif defined(bpf_target_arm)
@@ -174,7 +176,7 @@ struct pt_regs___arm64 {
#define __PT_RC_REG regs[0]
#define __PT_SP_REG sp
#define __PT_IP_REG pc
-#define PT_REGS_PARM1_SYSCALL(x) ({ _Pragma("GCC error \"use PT_REGS_PARM1_CORE_SYSCALL() instead\""); 0l; })
+#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
#define PT_REGS_PARM1_CORE_SYSCALL(x) BPF_CORE_READ((const struct pt_regs___arm64 *)(x), orig_x0)
#elif defined(bpf_target_mips)
@@ -493,39 +495,62 @@ typeof(name(0)) name(struct pt_regs *ctx) \
} \
static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
+/* If kernel has CONFIG_ARCH_HAS_SYSCALL_WRAPPER, read pt_regs directly */
#define ___bpf_syscall_args0() ctx
-#define ___bpf_syscall_args1(x) ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE_SYSCALL(regs)
-#define ___bpf_syscall_args2(x, args...) ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE_SYSCALL(regs)
-#define ___bpf_syscall_args3(x, args...) ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE_SYSCALL(regs)
-#define ___bpf_syscall_args4(x, args...) ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE_SYSCALL(regs)
-#define ___bpf_syscall_args5(x, args...) ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE_SYSCALL(regs)
+#define ___bpf_syscall_args1(x) ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_SYSCALL(regs)
+#define ___bpf_syscall_args2(x, args...) ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_SYSCALL(regs)
+#define ___bpf_syscall_args3(x, args...) ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_SYSCALL(regs)
+#define ___bpf_syscall_args4(x, args...) ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_SYSCALL(regs)
+#define ___bpf_syscall_args5(x, args...) ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_SYSCALL(regs)
#define ___bpf_syscall_args(args...) ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
+/* If kernel doesn't have CONFIG_ARCH_HAS_SYSCALL_WRAPPER, we have to BPF_CORE_READ from pt_regs */
+#define ___bpf_syswrap_args0() ctx
+#define ___bpf_syswrap_args1(x) ___bpf_syswrap_args0(), (void *)PT_REGS_PARM1_CORE_SYSCALL(regs)
+#define ___bpf_syswrap_args2(x, args...) ___bpf_syswrap_args1(args), (void *)PT_REGS_PARM2_CORE_SYSCALL(regs)
+#define ___bpf_syswrap_args3(x, args...) ___bpf_syswrap_args2(args), (void *)PT_REGS_PARM3_CORE_SYSCALL(regs)
+#define ___bpf_syswrap_args4(x, args...) ___bpf_syswrap_args3(args), (void *)PT_REGS_PARM4_CORE_SYSCALL(regs)
+#define ___bpf_syswrap_args5(x, args...) ___bpf_syswrap_args4(args), (void *)PT_REGS_PARM5_CORE_SYSCALL(regs)
+#define ___bpf_syswrap_args(args...) ___bpf_apply(___bpf_syswrap_args, ___bpf_narg(args))(args)
+
/*
- * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
+ * BPF_KSYSCALL is a variant of BPF_KPROBE, which is intended for
* tracing syscall functions, like __x64_sys_close. It hides the underlying
* platform-specific low-level way of getting syscall input arguments from
* struct pt_regs, and provides a familiar typed and named function arguments
* syntax and semantics of accessing syscall input parameters.
*
- * Original struct pt_regs* context is preserved as 'ctx' argument. This might
+ * Original struct pt_regs * context is preserved as 'ctx' argument. This might
* be necessary when using BPF helpers like bpf_perf_event_output().
*
- * This macro relies on BPF CO-RE support.
+ * At the moment BPF_KSYSCALL does not handle all the calling convention
+ * quirks for mmap(), clone() and compat syscalls transparrently. This may or
+ * may not change in the future. User needs to take extra measures to handle
+ * such quirks explicitly, if necessary.
+ *
+ * This macro relies on BPF CO-RE support and virtual __kconfig externs.
*/
-#define BPF_KPROBE_SYSCALL(name, args...) \
+#define BPF_KSYSCALL(name, args...) \
name(struct pt_regs *ctx); \
+extern _Bool LINUX_HAS_SYSCALL_WRAPPER __kconfig; \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args); \
typeof(name(0)) name(struct pt_regs *ctx) \
{ \
- struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
+ struct pt_regs *regs = LINUX_HAS_SYSCALL_WRAPPER \
+ ? (struct pt_regs *)PT_REGS_PARM1(ctx) \
+ : ctx; \
_Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
- return ____##name(___bpf_syscall_args(args)); \
+ if (LINUX_HAS_SYSCALL_WRAPPER) \
+ return ____##name(___bpf_syswrap_args(args)); \
+ else \
+ return ____##name(___bpf_syscall_args(args)); \
_Pragma("GCC diagnostic pop") \
} \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args)
+#define BPF_KPROBE_SYSCALL BPF_KSYSCALL
+
#endif
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4bae67767f82..7af12e03fc9b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7528,6 +7528,8 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
}
} else if (strcmp(ext->name, "LINUX_HAS_BPF_COOKIE") == 0) {
value = kernel_supports(obj, FEAT_BPF_COOKIE);
+ } else if (strcmp(ext->name, "LINUX_HAS_SYSCALL_WRAPPER") == 0) {
+ value = kernel_supports(obj, FEAT_SYSCALL_WRAPPER);
} else if (!str_has_pfx(ext->name, "LINUX_") || !ext->is_weak) {
/* Currently libbpf supports only CONFIG_ and LINUX_ prefixed
* __kconfig externs, where LINUX_ ones are virtual and filled out
--
2.30.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH bpf-next 4/5] libbpf: add ksyscall/kretsyscall sections support for syscall kprobes
2022-07-13 1:52 [PATCH bpf-next 0/5] Add SEC("ksyscall") support Andrii Nakryiko
` (2 preceding siblings ...)
2022-07-13 1:53 ` [PATCH bpf-next 3/5] libbpf: improve BPF_KPROBE_SYSCALL macro and rename it to BPF_KSYSCALL Andrii Nakryiko
@ 2022-07-13 1:53 ` Andrii Nakryiko
2022-07-13 1:53 ` [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests Andrii Nakryiko
4 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 1:53 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
Add SEC("ksyscall")/SEC("ksyscall/<syscall_name>") and corresponding
kretsyscall variants (for return kprobes) to allow users to kprobe
syscall functions in kernel. These special sections allow to ignore
complexities and differences between kernel versions and host
architectures when it comes to syscall wrapper and corresponding
__<arch>_sys_<syscall> vs __se_sys_<syscall> differences, depending on
whether host kernel has CONFIG_ARCH_HAS_SYSCALL_WRAPPER (though libbpf
itself doesn't rely on /proc/config.gz for detecting this, see
BPF_KSYSCALL patch for how it's done internally).
Combined with the use of BPF_KSYSCALL() macro, this allows to just
specify intended syscall name and expected input arguments and leave
dealing with all the variations to libbpf.
In addition to SEC("ksyscall+") and SEC("kretsyscall+") add
bpf_program__attach_ksyscall() API which allows to specify syscall name
at runtime and provide associated BPF cookie value.
At the moment SEC("ksyscall") and bpf_program__attach_ksyscall() do not
handle all the calling convention quirks for mmap(), clone() and compat
syscalls. It also only attaches to "native" syscall interfaces. If host
system supports compat syscalls or defines 32-bit syscalls in 64-bit
kernel, such syscall interfaces won't be attached to by libbpf.
These limitations may or may not change in the future. Therefore it is
recommended to use SEC("kprobe") for these syscalls or if working with
compat and 32-bit interfaces is required.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 117 +++++++++++++++++++++++++++++---
tools/lib/bpf/libbpf.h | 46 +++++++++++++
tools/lib/bpf/libbpf.map | 1 +
tools/lib/bpf/libbpf_internal.h | 2 +
4 files changed, 157 insertions(+), 9 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7af12e03fc9b..3a38813716e3 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4664,6 +4664,8 @@ static int probe_kern_btf_enum64(void)
strs, sizeof(strs)));
}
+static int probe_kern_syscall_wrapper(void);
+
enum kern_feature_result {
FEAT_UNKNOWN = 0,
FEAT_SUPPORTED = 1,
@@ -4732,6 +4734,9 @@ static struct kern_feature_desc {
[FEAT_BTF_ENUM64] = {
"BTF_KIND_ENUM64 support", probe_kern_btf_enum64,
},
+ [FEAT_SYSCALL_WRAPPER] = {
+ "Kernel using syscall wrapper", probe_kern_syscall_wrapper,
+ },
};
bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id feat_id)
@@ -8414,6 +8419,7 @@ int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log
static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf_link **link);
static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf_link **link);
+static int attach_ksyscall(const struct bpf_program *prog, long cookie, struct bpf_link **link);
static int attach_usdt(const struct bpf_program *prog, long cookie, struct bpf_link **link);
static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
@@ -8434,6 +8440,8 @@ static const struct bpf_sec_def section_defs[] = {
SEC_DEF("uretprobe.s+", KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
SEC_DEF("kprobe.multi+", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
SEC_DEF("kretprobe.multi+", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
+ SEC_DEF("ksyscall+", KPROBE, 0, SEC_NONE, attach_ksyscall),
+ SEC_DEF("kretsyscall+", KPROBE, 0, SEC_NONE, attach_ksyscall),
SEC_DEF("usdt+", KPROBE, 0, SEC_NONE, attach_usdt),
SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE),
@@ -9790,7 +9798,7 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
{
struct perf_event_attr attr = {};
char errmsg[STRERR_BUFSIZE];
- int type, pfd, err;
+ int type, pfd;
if (ref_ctr_off >= (1ULL << PERF_UPROBE_REF_CTR_OFFSET_BITS))
return -EINVAL;
@@ -9826,14 +9834,7 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
pid < 0 ? -1 : pid /* pid */,
pid == -1 ? 0 : -1 /* cpu */,
-1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
- if (pfd < 0) {
- err = -errno;
- pr_warn("%s perf_event_open() failed: %s\n",
- uprobe ? "uprobe" : "kprobe",
- libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
- return err;
- }
- return pfd;
+ return pfd >= 0 ? pfd : -errno;
}
static int append_to_file(const char *file, const char *fmt, ...)
@@ -9938,6 +9939,60 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
return err;
}
+static const char *arch_specific_syscall_pfx(void)
+{
+#if defined(__x86_64__)
+ return "x64";
+#elif defined(__i386__)
+ return "ia32";
+#elif defined(__s390x__)
+ return "s390x";
+#elif defined(__s390__)
+ return "s390";
+#elif defined(__arm__)
+ return "arm";
+#elif defined(__aarch64__)
+ return "arm64";
+#elif defined(__mips__)
+ return "mips";
+#elif defined(__riscv)
+ return "riscv";
+#else
+ return NULL;
+#endif
+}
+
+static int probe_kern_syscall_wrapper(void)
+{
+ char syscall_name[64];
+ const char *ksys_pfx;
+
+ ksys_pfx = arch_specific_syscall_pfx();
+ if (!ksys_pfx)
+ return 0;
+
+ snprintf(syscall_name, sizeof(syscall_name), "__%s_sys_bpf", ksys_pfx);
+
+ if (determine_kprobe_perf_type() >= 0) {
+ int pfd;
+
+ pfd = perf_event_open_probe(false, false, syscall_name, 0, getpid(), 0);
+ if (pfd >= 0)
+ close(pfd);
+
+ return pfd >= 0 ? 1 : 0;
+ } else { /* legacy mode */
+ char probe_name[128];
+
+ gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
+ if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
+ return 0;
+
+ (void)remove_kprobe_event_legacy(probe_name, false);
+ return 1;
+ }
+}
+
struct bpf_link *
bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
const char *func_name,
@@ -10023,6 +10078,29 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
return bpf_program__attach_kprobe_opts(prog, func_name, &opts);
}
+struct bpf_link *bpf_program__attach_ksyscall(const struct bpf_program *prog,
+ const char *syscall_name,
+ const struct bpf_ksyscall_opts *opts)
+{
+ LIBBPF_OPTS(bpf_kprobe_opts, kprobe_opts);
+ char func_name[128];
+
+ if (!OPTS_VALID(opts, bpf_ksyscall_opts))
+ return libbpf_err_ptr(-EINVAL);
+
+ if (kernel_supports(prog->obj, FEAT_SYSCALL_WRAPPER)) {
+ snprintf(func_name, sizeof(func_name), "__%s_sys_%s",
+ arch_specific_syscall_pfx(), syscall_name);
+ } else {
+ snprintf(func_name, sizeof(func_name), "__se_sys_%s", syscall_name);
+ }
+
+ kprobe_opts.retprobe = OPTS_GET(opts, retprobe, false);
+ kprobe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
+
+ return bpf_program__attach_kprobe_opts(prog, func_name, &kprobe_opts);
+}
+
/* Adapted from perf/util/string.c */
static bool glob_match(const char *str, const char *pat)
{
@@ -10193,6 +10271,27 @@ static int attach_kprobe(const struct bpf_program *prog, long cookie, struct bpf
return libbpf_get_error(*link);
}
+static int attach_ksyscall(const struct bpf_program *prog, long cookie, struct bpf_link **link)
+{
+ LIBBPF_OPTS(bpf_ksyscall_opts, opts);
+ const char *syscall_name;
+
+ *link = NULL;
+
+ /* no auto-attach for SEC("ksyscall") and SEC("kretsyscall") */
+ if (strcmp(prog->sec_name, "ksyscall") == 0 || strcmp(prog->sec_name, "kretsyscall") == 0)
+ return 0;
+
+ opts.retprobe = str_has_pfx(prog->sec_name, "kretsyscall/");
+ if (opts.retprobe)
+ syscall_name = prog->sec_name + sizeof("kretsyscall/") - 1;
+ else
+ syscall_name = prog->sec_name + sizeof("ksyscall/") - 1;
+
+ *link = bpf_program__attach_ksyscall(prog, syscall_name, &opts);
+ return *link ? 0 : -errno;
+}
+
static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link)
{
LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index e4d5353f757b..d10234aaa8df 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -457,6 +457,52 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
const char *pattern,
const struct bpf_kprobe_multi_opts *opts);
+struct bpf_ksyscall_opts {
+ /* size of this struct, for forward/backward compatiblity */
+ size_t sz;
+ /* custom user-provided value fetchable through bpf_get_attach_cookie() */
+ __u64 bpf_cookie;
+ /* attach as return probe? */
+ bool retprobe;
+ size_t :0;
+};
+#define bpf_ksyscall_opts__last_field retprobe
+
+/**
+ * @brief **bpf_program__attach_ksyscall()** attaches a BPF program
+ * to kernel syscall handler of a specified syscall. Optionally it's possible
+ * to request to install retprobe that will be triggered at syscall exit. It's
+ * also possible to associate BPF cookie (though options).
+ *
+ * Libbpf automatically will determine correct full kernel function name,
+ * which depending on system architecture and kernel version/configuration
+ * could be of the form __<arch>_sys_<syscall> or __se_sys_<syscall>, and will
+ * attach specified program using kprobe/kretprobe mechanism.
+ *
+ * **bpf_program__attach_ksyscall()** is an API counterpart of declarative
+ * **SEC("ksyscall/<syscall>")** annotation of BPF programs.
+ *
+ * At the moment **SEC("ksyscall")** and **bpf_program__attach_ksyscall()** do
+ * not handle all the calling convention quirks for mmap(), clone() and compat
+ * syscalls. It also only attaches to "native" syscall interfaces. If host
+ * system supports compat syscalls or defines 32-bit syscalls in 64-bit
+ * kernel, such syscall interfaces won't be attached to by libbpf.
+ *
+ * These limitations may or may not change in the future. Therefore it is
+ * recommended to use SEC("kprobe") for these syscalls or if working with
+ * compat and 32-bit interfaces is required.
+ *
+ * @param prog BPF program to attach
+ * @param syscall_name Symbolic name of the syscall (e.g., "bpf")
+ * @param opts Additional options (see **struct bpf_ksyscall_opts**)
+ * @return Reference to the newly created BPF link; or NULL is returned on
+ * error, error code is stored in errno
+ */
+LIBBPF_API struct bpf_link *
+bpf_program__attach_ksyscall(const struct bpf_program *prog,
+ const char *syscall_name,
+ const struct bpf_ksyscall_opts *opts);
+
struct bpf_uprobe_opts {
/* size of this struct, for forward/backward compatiblity */
size_t sz;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 94b589ecfeaa..63c059a02a0d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -356,6 +356,7 @@ LIBBPF_0.8.0 {
LIBBPF_1.0.0 {
global:
bpf_prog_query_opts;
+ bpf_program__attach_ksyscall;
btf__add_enum64;
btf__add_enum64_value;
libbpf_bpf_attach_type_str;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 9cd7829cbe41..f01dbab49da9 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -352,6 +352,8 @@ enum kern_feature_id {
FEAT_BPF_COOKIE,
/* BTF_KIND_ENUM64 support and BTF_KIND_ENUM kflag support */
FEAT_BTF_ENUM64,
+ /* Kernel uses syscall wrapper (CONFIG_ARCH_HAS_SYSCALL_WRAPPER) */
+ FEAT_SYSCALL_WRAPPER,
__FEAT_CNT,
};
--
2.30.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 1:52 [PATCH bpf-next 0/5] Add SEC("ksyscall") support Andrii Nakryiko
` (3 preceding siblings ...)
2022-07-13 1:53 ` [PATCH bpf-next 4/5] libbpf: add ksyscall/kretsyscall sections support for syscall kprobes Andrii Nakryiko
@ 2022-07-13 1:53 ` Andrii Nakryiko
2022-07-13 16:29 ` sdf
4 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 1:53 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
Convert few selftest that used plain SEC("kprobe") with arch-specific
syscall wrapper prefix to ksyscall/kretsyscall and corresponding
BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
simplification.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
.../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
.../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
3 files changed, 16 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
index 05838ed9b89c..e1e11897e99b 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
@@ -64,9 +64,9 @@ int BPF_KPROBE(handle_sys_prctl)
return 0;
}
-SEC("kprobe/" SYS_PREFIX "sys_prctl")
-int BPF_KPROBE_SYSCALL(prctl_enter, int option, unsigned long arg2,
- unsigned long arg3, unsigned long arg4, unsigned long arg5)
+SEC("ksyscall/prctl")
+int BPF_KSYSCALL(prctl_enter, int option, unsigned long arg2,
+ unsigned long arg3, unsigned long arg4, unsigned long arg5)
{
pid_t pid = bpf_get_current_pid_tgid() >> 32;
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index f1c88ad368ef..a1e45fec8938 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -1,11 +1,10 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017 Facebook
-#include <linux/ptrace.h>
-#include <linux/bpf.h>
+#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
-#include <stdbool.h>
+#include <bpf/bpf_core_read.h>
#include "bpf_misc.h"
int kprobe_res = 0;
@@ -31,8 +30,8 @@ int handle_kprobe(struct pt_regs *ctx)
return 0;
}
-SEC("kprobe/" SYS_PREFIX "sys_nanosleep")
-int BPF_KPROBE(handle_kprobe_auto)
+SEC("ksyscall/nanosleep")
+int BPF_KSYSCALL(handle_kprobe_auto, struct __kernel_timespec *req, struct __kernel_timespec *rem)
{
kprobe2_res = 11;
return 0;
@@ -56,11 +55,11 @@ int handle_kretprobe(struct pt_regs *ctx)
return 0;
}
-SEC("kretprobe/" SYS_PREFIX "sys_nanosleep")
-int BPF_KRETPROBE(handle_kretprobe_auto)
+SEC("kretsyscall/nanosleep")
+int BPF_KRETPROBE(handle_kretprobe_auto, int ret)
{
kretprobe2_res = 22;
- return 0;
+ return ret;
}
SEC("uprobe")
diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c b/tools/testing/selftests/bpf/progs/test_probe_user.c
index 702578a5e496..8e1495008e4d 100644
--- a/tools/testing/selftests/bpf/progs/test_probe_user.c
+++ b/tools/testing/selftests/bpf/progs/test_probe_user.c
@@ -1,35 +1,20 @@
// SPDX-License-Identifier: GPL-2.0
-
-#include <linux/ptrace.h>
-#include <linux/bpf.h>
-
-#include <netinet/in.h>
-
+#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
#include "bpf_misc.h"
static struct sockaddr_in old;
-SEC("kprobe/" SYS_PREFIX "sys_connect")
-int BPF_KPROBE(handle_sys_connect)
+SEC("ksyscall/connect")
+int BPF_KSYSCALL(handle_sys_connect, int fd, struct sockaddr_in *uservaddr, int addrlen)
{
-#if SYSCALL_WRAPPER == 1
- struct pt_regs *real_regs;
-#endif
struct sockaddr_in new;
- void *ptr;
-
-#if SYSCALL_WRAPPER == 0
- ptr = (void *)PT_REGS_PARM2(ctx);
-#else
- real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
- bpf_probe_read_kernel(&ptr, sizeof(ptr), &PT_REGS_PARM2(real_regs));
-#endif
- bpf_probe_read_user(&old, sizeof(old), ptr);
+ bpf_probe_read_user(&old, sizeof(old), uservaddr);
__builtin_memset(&new, 0xab, sizeof(new));
- bpf_probe_write_user(ptr, &new, sizeof(new));
+ bpf_probe_write_user(uservaddr, &new, sizeof(new));
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 1:53 ` [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests Andrii Nakryiko
@ 2022-07-13 16:29 ` sdf
2022-07-13 17:57 ` Andrii Nakryiko
0 siblings, 1 reply; 18+ messages in thread
From: sdf @ 2022-07-13 16:29 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, kernel-team
On 07/12, Andrii Nakryiko wrote:
> Convert few selftest that used plain SEC("kprobe") with arch-specific
> syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> simplification.
That looks super nice! I'm assuming the goal is probably
to get rid of that SYS_PREFIX everywhere eventually? And have a simple
test that exercises fentry/etc parsing?
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> .../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
> .../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
> .../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
> 3 files changed, 16 insertions(+), 32 deletions(-)
> diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> index 05838ed9b89c..e1e11897e99b 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> @@ -64,9 +64,9 @@ int BPF_KPROBE(handle_sys_prctl)
> return 0;
> }
> -SEC("kprobe/" SYS_PREFIX "sys_prctl")
> -int BPF_KPROBE_SYSCALL(prctl_enter, int option, unsigned long arg2,
> - unsigned long arg3, unsigned long arg4, unsigned long arg5)
> +SEC("ksyscall/prctl")
> +int BPF_KSYSCALL(prctl_enter, int option, unsigned long arg2,
> + unsigned long arg3, unsigned long arg4, unsigned long arg5)
> {
> pid_t pid = bpf_get_current_pid_tgid() >> 32;
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index f1c88ad368ef..a1e45fec8938 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -1,11 +1,10 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2017 Facebook
> -#include <linux/ptrace.h>
> -#include <linux/bpf.h>
> +#include "vmlinux.h"
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> -#include <stdbool.h>
> +#include <bpf/bpf_core_read.h>
> #include "bpf_misc.h"
> int kprobe_res = 0;
> @@ -31,8 +30,8 @@ int handle_kprobe(struct pt_regs *ctx)
> return 0;
> }
> -SEC("kprobe/" SYS_PREFIX "sys_nanosleep")
> -int BPF_KPROBE(handle_kprobe_auto)
> +SEC("ksyscall/nanosleep")
> +int BPF_KSYSCALL(handle_kprobe_auto, struct __kernel_timespec *req,
> struct __kernel_timespec *rem)
> {
> kprobe2_res = 11;
> return 0;
> @@ -56,11 +55,11 @@ int handle_kretprobe(struct pt_regs *ctx)
> return 0;
> }
> -SEC("kretprobe/" SYS_PREFIX "sys_nanosleep")
> -int BPF_KRETPROBE(handle_kretprobe_auto)
> +SEC("kretsyscall/nanosleep")
> +int BPF_KRETPROBE(handle_kretprobe_auto, int ret)
> {
> kretprobe2_res = 22;
> - return 0;
> + return ret;
> }
> SEC("uprobe")
> diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c
> b/tools/testing/selftests/bpf/progs/test_probe_user.c
> index 702578a5e496..8e1495008e4d 100644
> --- a/tools/testing/selftests/bpf/progs/test_probe_user.c
> +++ b/tools/testing/selftests/bpf/progs/test_probe_user.c
> @@ -1,35 +1,20 @@
> // SPDX-License-Identifier: GPL-2.0
> -
> -#include <linux/ptrace.h>
> -#include <linux/bpf.h>
> -
> -#include <netinet/in.h>
> -
> +#include "vmlinux.h"
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_core_read.h>
> #include "bpf_misc.h"
> static struct sockaddr_in old;
> -SEC("kprobe/" SYS_PREFIX "sys_connect")
> -int BPF_KPROBE(handle_sys_connect)
> +SEC("ksyscall/connect")
> +int BPF_KSYSCALL(handle_sys_connect, int fd, struct sockaddr_in
> *uservaddr, int addrlen)
> {
> -#if SYSCALL_WRAPPER == 1
> - struct pt_regs *real_regs;
> -#endif
> struct sockaddr_in new;
> - void *ptr;
> -
> -#if SYSCALL_WRAPPER == 0
> - ptr = (void *)PT_REGS_PARM2(ctx);
> -#else
> - real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
> - bpf_probe_read_kernel(&ptr, sizeof(ptr), &PT_REGS_PARM2(real_regs));
> -#endif
> - bpf_probe_read_user(&old, sizeof(old), ptr);
> + bpf_probe_read_user(&old, sizeof(old), uservaddr);
> __builtin_memset(&new, 0xab, sizeof(new));
> - bpf_probe_write_user(ptr, &new, sizeof(new));
> + bpf_probe_write_user(uservaddr, &new, sizeof(new));
> return 0;
> }
> --
> 2.30.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 16:29 ` sdf
@ 2022-07-13 17:57 ` Andrii Nakryiko
2022-07-13 18:57 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 17:57 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 9:29 AM <sdf@google.com> wrote:
>
> On 07/12, Andrii Nakryiko wrote:
> > Convert few selftest that used plain SEC("kprobe") with arch-specific
> > syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> > BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> > simplification.
>
> That looks super nice! I'm assuming the goal is probably
Thanks!
> to get rid of that SYS_PREFIX everywhere eventually? And have a simple
> test that exercises fentry/etc parsing?
All the other uses of SYS_PREFIX in selftests right now are
fentry/fexit. If the consensus is that this sort of higher-level
wrapper around fentry/fexit specifically for syscalls is useful, it's
not a lot of work to add something like SEC("fsyscall") and
SEC("fretsyscall") with the same approach.
One possible argument against this (and I need to double check my
assumptions first), is that with SYSCALL_WRAPPER used (which is true
for "major" platforms like x86_64), fentry doesn't provide much
benefit because __<arch>_sys_<syscall>() function will have only one
typed argument - struct pt_regs, and so we'll have to use
BPF_CORE_READ() to fetch actual arguments, at which point BPF verifier
will lose track of type information. So it's just a slightly more
performant (in terms of invocation overhead) kprobe at that point, but
with no added benefit of BTF types for input arguments.
But curious to hear what others think about this.
>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > .../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
> > .../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
> > .../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
> > 3 files changed, 16 insertions(+), 32 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 17:57 ` Andrii Nakryiko
@ 2022-07-13 18:57 ` Stanislav Fomichev
2022-07-13 20:30 ` Andrii Nakryiko
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-13 18:57 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 10:57 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 9:29 AM <sdf@google.com> wrote:
> >
> > On 07/12, Andrii Nakryiko wrote:
> > > Convert few selftest that used plain SEC("kprobe") with arch-specific
> > > syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> > > BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> > > simplification.
> >
> > That looks super nice! I'm assuming the goal is probably
>
> Thanks!
>
> > to get rid of that SYS_PREFIX everywhere eventually? And have a simple
> > test that exercises fentry/etc parsing?
>
> All the other uses of SYS_PREFIX in selftests right now are
> fentry/fexit. If the consensus is that this sort of higher-level
> wrapper around fentry/fexit specifically for syscalls is useful, it's
> not a lot of work to add something like SEC("fsyscall") and
> SEC("fretsyscall") with the same approach.
>
> One possible argument against this (and I need to double check my
> assumptions first), is that with SYSCALL_WRAPPER used (which is true
> for "major" platforms like x86_64), fentry doesn't provide much
> benefit because __<arch>_sys_<syscall>() function will have only one
> typed argument - struct pt_regs, and so we'll have to use
> BPF_CORE_READ() to fetch actual arguments, at which point BPF verifier
> will lose track of type information. So it's just a slightly more
> performant (in terms of invocation overhead) kprobe at that point, but
> with no added benefit of BTF types for input arguments.
>
> But curious to hear what others think about this.
What would be nice (but not sure if possible, I haven't looked
closely), if these same ksyscall sections would pick the best
underlying implementation: if fentry is available -> attach to fentry,
if not -> fallback to kprobe (and do all this __<prefix>_sys vs __sys
dance behind the scenes). Any reasons the users should care if it's
really a kprobe or an fentry?
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > > .../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
> > > .../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
> > > .../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
> > > 3 files changed, 16 insertions(+), 32 deletions(-)
> >
>
> [...]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 18:57 ` Stanislav Fomichev
@ 2022-07-13 20:30 ` Andrii Nakryiko
2022-07-13 21:17 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-13 20:30 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Wed, Jul 13, 2022 at 10:57 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 9:29 AM <sdf@google.com> wrote:
> > >
> > > On 07/12, Andrii Nakryiko wrote:
> > > > Convert few selftest that used plain SEC("kprobe") with arch-specific
> > > > syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> > > > BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> > > > simplification.
> > >
> > > That looks super nice! I'm assuming the goal is probably
> >
> > Thanks!
> >
> > > to get rid of that SYS_PREFIX everywhere eventually? And have a simple
> > > test that exercises fentry/etc parsing?
> >
> > All the other uses of SYS_PREFIX in selftests right now are
> > fentry/fexit. If the consensus is that this sort of higher-level
> > wrapper around fentry/fexit specifically for syscalls is useful, it's
> > not a lot of work to add something like SEC("fsyscall") and
> > SEC("fretsyscall") with the same approach.
> >
> > One possible argument against this (and I need to double check my
> > assumptions first), is that with SYSCALL_WRAPPER used (which is true
> > for "major" platforms like x86_64), fentry doesn't provide much
> > benefit because __<arch>_sys_<syscall>() function will have only one
> > typed argument - struct pt_regs, and so we'll have to use
> > BPF_CORE_READ() to fetch actual arguments, at which point BPF verifier
> > will lose track of type information. So it's just a slightly more
> > performant (in terms of invocation overhead) kprobe at that point, but
> > with no added benefit of BTF types for input arguments.
> >
> > But curious to hear what others think about this.
>
> What would be nice (but not sure if possible, I haven't looked
> closely), if these same ksyscall sections would pick the best
> underlying implementation: if fentry is available -> attach to fentry,
> if not -> fallback to kprobe (and do all this __<prefix>_sys vs __sys
> dance behind the scenes). Any reasons the users should care if it's
> really a kprobe or an fentry?
It's technically possible to choose kprobe vs fentry, but I'm not
comfortable with that level of autonomy for libbpf. There might be
subtle differences between kprobes and fentry (and I did run into some
limitations with fentry due to extra BTF information that verifier
enforces, while I need to only get raw integer value of some pointer;
had to work around that in retsnoop, for example), so I generally
follow the philosophy that user needs to be explicit about what they
want, and libbpf shouldn't try to guess (which differs from BCC's
approach in a number of areas and I'm pretty pleased how that turns
out for libbpf and its users in general).
So if we do this shim for fentry/fexit, I think it should be explicit
SEC("fsyscall/fretsyscall") or something along those lines.
>
>
>
> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > > ---
> > > > .../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
> > > > .../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
> > > > .../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
> > > > 3 files changed, 16 insertions(+), 32 deletions(-)
> > >
> >
> > [...]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: use BPF_KSYSCALL and SEC("ksyscall") in selftests
2022-07-13 20:30 ` Andrii Nakryiko
@ 2022-07-13 21:17 ` Stanislav Fomichev
0 siblings, 0 replies; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-13 21:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Jul 13, 2022 at 1:30 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 10:57 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 9:29 AM <sdf@google.com> wrote:
> > > >
> > > > On 07/12, Andrii Nakryiko wrote:
> > > > > Convert few selftest that used plain SEC("kprobe") with arch-specific
> > > > > syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> > > > > BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> > > > > simplification.
> > > >
> > > > That looks super nice! I'm assuming the goal is probably
> > >
> > > Thanks!
> > >
> > > > to get rid of that SYS_PREFIX everywhere eventually? And have a simple
> > > > test that exercises fentry/etc parsing?
> > >
> > > All the other uses of SYS_PREFIX in selftests right now are
> > > fentry/fexit. If the consensus is that this sort of higher-level
> > > wrapper around fentry/fexit specifically for syscalls is useful, it's
> > > not a lot of work to add something like SEC("fsyscall") and
> > > SEC("fretsyscall") with the same approach.
> > >
> > > One possible argument against this (and I need to double check my
> > > assumptions first), is that with SYSCALL_WRAPPER used (which is true
> > > for "major" platforms like x86_64), fentry doesn't provide much
> > > benefit because __<arch>_sys_<syscall>() function will have only one
> > > typed argument - struct pt_regs, and so we'll have to use
> > > BPF_CORE_READ() to fetch actual arguments, at which point BPF verifier
> > > will lose track of type information. So it's just a slightly more
> > > performant (in terms of invocation overhead) kprobe at that point, but
> > > with no added benefit of BTF types for input arguments.
> > >
> > > But curious to hear what others think about this.
> >
> > What would be nice (but not sure if possible, I haven't looked
> > closely), if these same ksyscall sections would pick the best
> > underlying implementation: if fentry is available -> attach to fentry,
> > if not -> fallback to kprobe (and do all this __<prefix>_sys vs __sys
> > dance behind the scenes). Any reasons the users should care if it's
> > really a kprobe or an fentry?
>
> It's technically possible to choose kprobe vs fentry, but I'm not
> comfortable with that level of autonomy for libbpf. There might be
> subtle differences between kprobes and fentry (and I did run into some
> limitations with fentry due to extra BTF information that verifier
> enforces, while I need to only get raw integer value of some pointer;
> had to work around that in retsnoop, for example), so I generally
> follow the philosophy that user needs to be explicit about what they
> want, and libbpf shouldn't try to guess (which differs from BCC's
> approach in a number of areas and I'm pretty pleased how that turns
> out for libbpf and its users in general).
>
> So if we do this shim for fentry/fexit, I think it should be explicit
> SEC("fsyscall/fretsyscall") or something along those lines.
In this case yeah, if there are user-visible differences, doing a
separate fsyscall is better.
(removing SYS_PREFIX seems like a good goal)
^ permalink raw reply [flat|nested] 18+ messages in thread