From: sdf@google.com
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
kernel-team@fb.com
Subject: Re: [PATCH bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals
Date: Fri, 17 Sep 2021 10:27:58 -0700 [thread overview]
Message-ID: <YUTQHir6qZZHUNZm@google.com> (raw)
In-Reply-To: <20210917061020.821711-6-andrii@kernel.org>
On 09/16, Andrii Nakryiko wrote:
> Move closer to not relying on bpf_sec_def internals that won't be part
> of public API, when pluggable SEC() handlers will be allowed. Drop
> pre-calculated prefix length, and in various helpers don't rely on this
> prefix length availability. Also minimize reliance on knowing
> bpf_sec_def's prefix for few places where section prefix shortcuts are
> supported (e.g., tp vs tracepoint, raw_tp vs raw_tracepoint).
> Given checking some string for having a given string-constant prefix is
> such a common operation and so annoying to be done with pure C code, add
> a small macro helper, str_has_pfx(), and reuse it throughout libbpf.c
> where prefix comparison is performed. With __builtin_constant_p() it's
> possible to have a convenient helper that checks some string for having
> a given prefix, where prefix is either string literal (or compile-time
> known string due to compiler optimization) or just a runtime string
> pointer, which is quite convenient and saves a lot of typing and string
> literal duplication.
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> tools/lib/bpf/libbpf.c | 41 ++++++++++++++++++---------------
> tools/lib/bpf/libbpf_internal.h | 7 ++++++
> 2 files changed, 30 insertions(+), 18 deletions(-)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8ab2edbf7a3b..52c398fae0af 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -226,7 +226,6 @@ typedef struct bpf_link *(*attach_fn_t)(const struct
> bpf_program *prog, long coo
> struct bpf_sec_def {
> const char *sec;
> - size_t len;
> enum bpf_prog_type prog_type;
> enum bpf_attach_type expected_attach_type;
> bool is_exp_attach_type_optional;
> @@ -1671,7 +1670,7 @@ static int bpf_object__process_kconfig_line(struct
> bpf_object *obj,
> void *ext_val;
> __u64 num;
> - if (strncmp(buf, "CONFIG_", 7))
> + if (!str_has_pfx(buf, "CONFIG_"))
> return 0;
> sep = strchr(buf, '=');
> @@ -2919,7 +2918,7 @@ static Elf_Data *elf_sec_data(const struct
> bpf_object *obj, Elf_Scn *scn)
> static bool is_sec_name_dwarf(const char *name)
> {
> /* approximation, but the actual list is too long */
> - return strncmp(name, ".debug_", sizeof(".debug_") - 1) == 0;
> + return str_has_pfx(name, ".debug_");
> }
> static bool ignore_elf_section(GElf_Shdr *hdr, const char *name)
> @@ -2941,7 +2940,7 @@ static bool ignore_elf_section(GElf_Shdr *hdr,
> const char *name)
> if (is_sec_name_dwarf(name))
> return true;
> - if (strncmp(name, ".rel", sizeof(".rel") - 1) == 0) {
> + if (str_has_pfx(name, ".rel")) {
> name += sizeof(".rel") - 1;
> /* DWARF section relocations */
> if (is_sec_name_dwarf(name))
> @@ -6890,8 +6889,7 @@ static int bpf_object__resolve_externs(struct
> bpf_object *obj,
> if (err)
> return err;
> pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> - } else if (ext->type == EXT_KCFG &&
> - strncmp(ext->name, "CONFIG_", 7) == 0) {
> + } 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)
> @@ -7955,7 +7953,6 @@ void bpf_program__set_expected_attach_type(struct
> bpf_program *prog,
> attachable, attach_btf) \
> { \
> .sec = string, \
> - .len = sizeof(string) - 1, \
> .prog_type = ptype, \
> .expected_attach_type = eatype, \
> .is_exp_attach_type_optional = eatype_optional, \
> @@ -7986,7 +7983,6 @@ void bpf_program__set_expected_attach_type(struct
> bpf_program *prog,
> #define SEC_DEF(sec_pfx, ptype, ...) { \
> .sec = sec_pfx, \
> - .len = sizeof(sec_pfx) - 1, \
> .prog_type = BPF_PROG_TYPE_##ptype, \
> .preload_fn = libbpf_preload_prog, \
> __VA_ARGS__ \
> @@ -8160,10 +8156,8 @@ static const struct bpf_sec_def
> *find_sec_def(const char *sec_name)
> int i, n = ARRAY_SIZE(section_defs);
> for (i = 0; i < n; i++) {
> - if (strncmp(sec_name,
> - section_defs[i].sec, section_defs[i].len))
> - continue;
> - return §ion_defs[i];
> + if (str_has_pfx(sec_name, section_defs[i].sec))
> + return §ion_defs[i];
> }
> return NULL;
> }
> @@ -8517,7 +8511,7 @@ static int libbpf_find_attach_btf_id(struct
> bpf_program *prog, int *btf_obj_fd,
> prog->sec_name);
> return -ESRCH;
> }
> - attach_name = prog->sec_name + prog->sec_def->len;
> + attach_name = prog->sec_name + strlen(prog->sec_def->sec);
> /* BPF program's BTF ID */
> if (attach_prog_fd) {
> @@ -9454,8 +9448,11 @@ static struct bpf_link *attach_kprobe(const struct
> bpf_program *prog, long cooki
> char *func;
> int n, err;
> - func_name = prog->sec_name + prog->sec_def->len;
> - opts.retprobe = strcmp(prog->sec_def->sec, "kretprobe/") == 0;
> + opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> + if (opts.retprobe)
> + func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> + else
> + func_name = prog->sec_name + sizeof("kprobe/") - 1;
> n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
> if (n < 1) {
> @@ -9627,8 +9624,11 @@ static struct bpf_link *attach_tp(const struct
> bpf_program *prog, long cookie)
> if (!sec_name)
> return libbpf_err_ptr(-ENOMEM);
> - /* extract "tp/<category>/<name>" */
> - tp_cat = sec_name + prog->sec_def->len;
> + /* extract "tp/<category>/<name>" or "tracepoint/<category>/<name>" */
> + if (str_has_pfx(prog->sec_name, "tp/"))
> + tp_cat = sec_name + sizeof("tp/") - 1;
> + else
> + tp_cat = sec_name + sizeof("tracepoint/") - 1;
> tp_name = strchr(tp_cat, '/');
> if (!tp_name) {
> free(sec_name);
> @@ -9674,7 +9674,12 @@ struct bpf_link
> *bpf_program__attach_raw_tracepoint(const struct bpf_program *pr
> static struct bpf_link *attach_raw_tp(const struct bpf_program *prog,
> long cookie)
> {
> - const char *tp_name = prog->sec_name + prog->sec_def->len;
> + const char *tp_name;
> +
> + if (str_has_pfx(prog->sec_name, "raw_tp/"))
> + tp_name = prog->sec_name + sizeof("raw_tp/") - 1;
> + else
> + tp_name = prog->sec_name + sizeof("raw_tracepoint/") - 1;
> return bpf_program__attach_raw_tracepoint(prog, tp_name);
> }
> diff --git a/tools/lib/bpf/libbpf_internal.h
> b/tools/lib/bpf/libbpf_internal.h
> index ceb0c98979bc..ec79400517d4 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -89,6 +89,13 @@
> (offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD))
> #endif
> +/* Check whether a string `str` has prefix `pfx`, regardless if `pfx` is
> + * a string literal known at compilation time or char * pointer known
> only at
> + * runtime.
> + */
> +#define str_has_pfx(str, pfx) \
> + (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 :
> strlen(pfx)) == 0)
> +
Nit: maybe 'starts_with'? c++ has that in stdlib and iirc python also
has either starts_with or statswith.
next prev parent reply other threads:[~2021-09-17 17:28 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-17 6:10 [PATCH bpf-next 0/9] libbpf: stricter BPF program section name handling Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 1/9] selftests/bpf: normalize XDP section names in selftests Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 2/9] selftests/bpf: normalize SEC("classifier") usage Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 3/9] selftests/bpf: normalize all the rest SEC() uses Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 5/9] libbpf: reduce reliance of attach_fns on sec_def internals Andrii Nakryiko
2021-09-17 17:27 ` sdf [this message]
2021-09-17 18:08 ` Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 6/9] libbpf: refactor ELF section handler definitions Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 8/9] libbpf: add opt-in strict BPF program section name handling logic Andrii Nakryiko
2021-09-17 17:26 ` sdf
2021-09-17 18:13 ` Andrii Nakryiko
2021-09-17 23:11 ` sdf
2021-09-17 23:21 ` Andrii Nakryiko
2021-09-17 23:39 ` sdf
2021-09-19 17:31 ` Andrii Nakryiko
2021-09-17 6:10 ` [PATCH bpf-next 9/9] selftests/bpf: switch sk_lookup selftests to strict SEC("sk_lookup") use Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YUTQHir6qZZHUNZm@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).