* [PATCH bpf-next 0/2] expand bpf_d_path helper allowlist
@ 2021-07-25 14:18 Hengqi Chen
2021-07-25 14:18 ` [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen
2021-07-25 14:18 ` [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen
0 siblings, 2 replies; 12+ messages in thread
From: Hengqi Chen @ 2021-07-25 14:18 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen
This patch series adds more functions to bpf_d_path allowlist.
Patch 1 resolves an issue of missing symbols of .BTF_ids.
Patch 2 expands bpf_d_path allowlist.
Changes since v2: [2]
- Andrii suggested that we should first address an issue of .BTF_ids
before adding more symbols to .BTF_ids. Fixed that.
- Yaniv proposed adding security_sb_mount and security_bprm_check.
Added them.
Changes since v1: [1]
- Alexei and Yonghong suggested that bpf_d_path helper could also
apply to vfs_* and security_file_* kernel functions. Added them.
[1] https://lore.kernel.org/bpf/20210712162424.2034006-1-hengqi.chen@gmail.com/
[2] https://lore.kernel.org/bpf/20210719151753.399227-1-hengqi.chen@gmail.com/
[3] https://github.com/iovisor/bcc/issues/3527
Hengqi Chen (2):
tools/resolve_btfids: emit warnings and patch zero id for missing
symbols
bpf: expose bpf_d_path helper to vfs_* and security_* functions
kernel/trace/bpf_trace.c | 52 +++++++++++++++++++++++++++++++--
tools/bpf/resolve_btfids/main.c | 13 +++++----
2 files changed, 57 insertions(+), 8 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-25 14:18 [PATCH bpf-next 0/2] expand bpf_d_path helper allowlist Hengqi Chen @ 2021-07-25 14:18 ` Hengqi Chen 2021-07-26 3:32 ` Yonghong Song 2021-07-26 20:16 ` Andrii Nakryiko 2021-07-25 14:18 ` [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen 1 sibling, 2 replies; 12+ messages in thread From: Hengqi Chen @ 2021-07-25 14:18 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen Kernel functions referenced by .BTF_ids may changed from global to static and get inlined and thus disappears from BTF. This causes kernel build failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. Update resolve_btfids to emit warning messages and patch zero id for missing symbols instead of aborting kernel build process. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/bpf/resolve_btfids/main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index 3ad9301b0f00..3ea19e33250d 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) sh->sh_addralign = expected; if (gelf_update_shdr(scn, sh) == 0) { - printf("FAILED cannot update section header: %s\n", + pr_err("FAILED cannot update section header: %s\n", elf_errmsg(-1)); return -1; } @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); if (!elf) { + close(fd); pr_err("FAILED cannot create ELF descriptor: %s\n", elf_errmsg(-1)); return -1; @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) err = libbpf_get_error(btf); if (err) { pr_err("FAILED: load BTF from %s: %s\n", - obj->path, strerror(-err)); + obj->btf ?: obj->path, strerror(-err)); return -1; } @@ -555,8 +556,7 @@ static int id_patch(struct object *obj, struct btf_id *id) int i; if (!id->id) { - pr_err("FAILED unresolved symbol %s\n", id->name); - return -EINVAL; + pr_err("WARN: unresolved symbol %s\n", id->name); } for (i = 0; i < id->addr_cnt; i++) { @@ -734,8 +734,9 @@ int main(int argc, const char **argv) err = 0; out: - if (obj.efile.elf) + if (obj.efile.elf) { elf_end(obj.efile.elf); - close(obj.efile.fd); + close(obj.efile.fd); + } return err; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-25 14:18 ` [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen @ 2021-07-26 3:32 ` Yonghong Song 2021-07-26 4:41 ` Hengqi Chen 2021-07-26 20:16 ` Andrii Nakryiko 1 sibling, 1 reply; 12+ messages in thread From: Yonghong Song @ 2021-07-26 3:32 UTC (permalink / raw) To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 7/25/21 7:18 AM, Hengqi Chen wrote: > Kernel functions referenced by .BTF_ids may changed from global to static > and get inlined and thus disappears from BTF. This causes kernel build the function could be renamed or removed too. > failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. > Update resolve_btfids to emit warning messages and patch zero id for missing > symbols instead of aborting kernel build process. > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> LGTM with one minor comment below. Acked-by: Yonghong Song <yhs@fb.com> > --- > tools/bpf/resolve_btfids/main.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > index 3ad9301b0f00..3ea19e33250d 100644 > --- a/tools/bpf/resolve_btfids/main.c > +++ b/tools/bpf/resolve_btfids/main.c > @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) > sh->sh_addralign = expected; > > if (gelf_update_shdr(scn, sh) == 0) { > - printf("FAILED cannot update section header: %s\n", > + pr_err("FAILED cannot update section header: %s\n", > elf_errmsg(-1)); > return -1; > } > @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) > > elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); > if (!elf) { > + close(fd); > pr_err("FAILED cannot create ELF descriptor: %s\n", > elf_errmsg(-1)); > return -1; > @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) > err = libbpf_get_error(btf); > if (err) { > pr_err("FAILED: load BTF from %s: %s\n", > - obj->path, strerror(-err)); > + obj->btf ?: obj->path, strerror(-err)); Why you change "obj->path" to "obj->btf ?: obj->path"? Note that obj->path cannot be NULL. > return -1; > } > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-26 3:32 ` Yonghong Song @ 2021-07-26 4:41 ` Hengqi Chen 2021-07-26 4:56 ` Yonghong Song 0 siblings, 1 reply; 12+ messages in thread From: Hengqi Chen @ 2021-07-26 4:41 UTC (permalink / raw) To: Yonghong Song, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 2021/7/26 11:32 AM, Yonghong Song wrote: > > > On 7/25/21 7:18 AM, Hengqi Chen wrote: >> Kernel functions referenced by .BTF_ids may changed from global to static >> and get inlined and thus disappears from BTF. This causes kernel build > > the function could be renamed or removed too. > >> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. >> Update resolve_btfids to emit warning messages and patch zero id for missing >> symbols instead of aborting kernel build process. >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > LGTM with one minor comment below. > > Acked-by: Yonghong Song <yhs@fb.com> > >> --- >> tools/bpf/resolve_btfids/main.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >> index 3ad9301b0f00..3ea19e33250d 100644 >> --- a/tools/bpf/resolve_btfids/main.c >> +++ b/tools/bpf/resolve_btfids/main.c >> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) >> sh->sh_addralign = expected; >> >> if (gelf_update_shdr(scn, sh) == 0) { >> - printf("FAILED cannot update section header: %s\n", >> + pr_err("FAILED cannot update section header: %s\n", >> elf_errmsg(-1)); >> return -1; >> } >> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) >> >> elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); >> if (!elf) { >> + close(fd); >> pr_err("FAILED cannot create ELF descriptor: %s\n", >> elf_errmsg(-1)); >> return -1; >> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) >> err = libbpf_get_error(btf); >> if (err) { >> pr_err("FAILED: load BTF from %s: %s\n", >> - obj->path, strerror(-err)); >> + obj->btf ?: obj->path, strerror(-err)); > > Why you change "obj->path" to "obj->btf ?: obj->path"? > Note that obj->path cannot be NULL. The diff didn't see the whole picture. Let me quote it here: ``` btf = btf__parse(obj->btf ?: obj->path, NULL); err = libbpf_get_error(btf); if (err) { pr_err("FAILED: load BTF from %s: %s\n", obj->path, strerror(-err)); return -1; } ``` Because btf__parse parses either obj->btf or obj->path, I think the error message should reveal this. > >> return -1; >> } >> > [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-26 4:41 ` Hengqi Chen @ 2021-07-26 4:56 ` Yonghong Song 2021-07-26 5:22 ` Hengqi Chen 0 siblings, 1 reply; 12+ messages in thread From: Yonghong Song @ 2021-07-26 4:56 UTC (permalink / raw) To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 7/25/21 9:41 PM, Hengqi Chen wrote: > > > On 2021/7/26 11:32 AM, Yonghong Song wrote: >> >> >> On 7/25/21 7:18 AM, Hengqi Chen wrote: >>> Kernel functions referenced by .BTF_ids may changed from global to static >>> and get inlined and thus disappears from BTF. This causes kernel build >> >> the function could be renamed or removed too. >> >>> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. >>> Update resolve_btfids to emit warning messages and patch zero id for missing >>> symbols instead of aborting kernel build process. >>> >>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> >> LGTM with one minor comment below. >> >> Acked-by: Yonghong Song <yhs@fb.com> >> >>> --- >>> tools/bpf/resolve_btfids/main.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >>> index 3ad9301b0f00..3ea19e33250d 100644 >>> --- a/tools/bpf/resolve_btfids/main.c >>> +++ b/tools/bpf/resolve_btfids/main.c >>> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) >>> sh->sh_addralign = expected; >>> >>> if (gelf_update_shdr(scn, sh) == 0) { >>> - printf("FAILED cannot update section header: %s\n", >>> + pr_err("FAILED cannot update section header: %s\n", >>> elf_errmsg(-1)); >>> return -1; >>> } >>> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) >>> >>> elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); >>> if (!elf) { >>> + close(fd); >>> pr_err("FAILED cannot create ELF descriptor: %s\n", >>> elf_errmsg(-1)); >>> return -1; >>> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) >>> err = libbpf_get_error(btf); >>> if (err) { >>> pr_err("FAILED: load BTF from %s: %s\n", >>> - obj->path, strerror(-err)); >>> + obj->btf ?: obj->path, strerror(-err)); >> >> Why you change "obj->path" to "obj->btf ?: obj->path"? >> Note that obj->path cannot be NULL. > > The diff didn't see the whole picture. Let me quote it here: > ``` > btf = btf__parse(obj->btf ?: obj->path, NULL); > err = libbpf_get_error(btf); > if (err) { > pr_err("FAILED: load BTF from %s: %s\n", > obj->path, strerror(-err)); > return -1; > } > ``` > > Because btf__parse parses either obj->btf or obj->path, > I think the error message should reveal this. Okay, I see, obj->btf may not be NULL due to OPT_STRING(0, "btf", &obj.btf, "BTF data", "BTF data"), How about obj->btf ? "input BTF data" : obj->path The error message like FAILED: load BTF from : <error msg> does not sound good. > >> >>> return -1; >>> } >>> >> [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-26 4:56 ` Yonghong Song @ 2021-07-26 5:22 ` Hengqi Chen 2021-07-26 5:33 ` Yonghong Song 0 siblings, 1 reply; 12+ messages in thread From: Hengqi Chen @ 2021-07-26 5:22 UTC (permalink / raw) To: Yonghong Song, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 2021/7/26 12:56 PM, Yonghong Song wrote: > > > On 7/25/21 9:41 PM, Hengqi Chen wrote: >> >> >> On 2021/7/26 11:32 AM, Yonghong Song wrote: >>> >>> >>> On 7/25/21 7:18 AM, Hengqi Chen wrote: >>>> Kernel functions referenced by .BTF_ids may changed from global to static >>>> and get inlined and thus disappears from BTF. This causes kernel build >>> >>> the function could be renamed or removed too. >>> >>>> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. >>>> Update resolve_btfids to emit warning messages and patch zero id for missing >>>> symbols instead of aborting kernel build process. >>>> >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>> >>> LGTM with one minor comment below. >>> >>> Acked-by: Yonghong Song <yhs@fb.com> >>> >>>> --- >>>> tools/bpf/resolve_btfids/main.c | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >>>> index 3ad9301b0f00..3ea19e33250d 100644 >>>> --- a/tools/bpf/resolve_btfids/main.c >>>> +++ b/tools/bpf/resolve_btfids/main.c >>>> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) >>>> sh->sh_addralign = expected; >>>> >>>> if (gelf_update_shdr(scn, sh) == 0) { >>>> - printf("FAILED cannot update section header: %s\n", >>>> + pr_err("FAILED cannot update section header: %s\n", >>>> elf_errmsg(-1)); >>>> return -1; >>>> } >>>> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) >>>> >>>> elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); >>>> if (!elf) { >>>> + close(fd); >>>> pr_err("FAILED cannot create ELF descriptor: %s\n", >>>> elf_errmsg(-1)); >>>> return -1; >>>> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) >>>> err = libbpf_get_error(btf); >>>> if (err) { >>>> pr_err("FAILED: load BTF from %s: %s\n", >>>> - obj->path, strerror(-err)); >>>> + obj->btf ?: obj->path, strerror(-err)); >>> >>> Why you change "obj->path" to "obj->btf ?: obj->path"? >>> Note that obj->path cannot be NULL. >> >> The diff didn't see the whole picture. Let me quote it here: >> ``` >> btf = btf__parse(obj->btf ?: obj->path, NULL); >> err = libbpf_get_error(btf); >> if (err) { >> pr_err("FAILED: load BTF from %s: %s\n", >> obj->path, strerror(-err)); >> return -1; >> } >> ``` >> >> Because btf__parse parses either obj->btf or obj->path, >> I think the error message should reveal this. > > Okay, I see, obj->btf may not be NULL due to > OPT_STRING(0, "btf", &obj.btf, "BTF data", > "BTF data"), > > How about > obj->btf ? "input BTF data" : obj->path > > The error message like > FAILED: load BTF from : <error msg> > does not sound good. > Sorry, I am confused. If obj->btf is set, say, vmlinux.btf, the message should look like: FAILED: load BTF from vmlinux.btf: <error msg> Otherwise, it should look like: FAILED: load BTF from vmlinux: <error msg> Am I missing something ? >> >>> >>>> return -1; >>>> } >>>> >>> [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-26 5:22 ` Hengqi Chen @ 2021-07-26 5:33 ` Yonghong Song 0 siblings, 0 replies; 12+ messages in thread From: Yonghong Song @ 2021-07-26 5:33 UTC (permalink / raw) To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 7/25/21 10:22 PM, Hengqi Chen wrote: > > > On 2021/7/26 12:56 PM, Yonghong Song wrote: >> >> >> On 7/25/21 9:41 PM, Hengqi Chen wrote: >>> >>> >>> On 2021/7/26 11:32 AM, Yonghong Song wrote: >>>> >>>> >>>> On 7/25/21 7:18 AM, Hengqi Chen wrote: >>>>> Kernel functions referenced by .BTF_ids may changed from global to static >>>>> and get inlined and thus disappears from BTF. This causes kernel build >>>> >>>> the function could be renamed or removed too. >>>> >>>>> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. >>>>> Update resolve_btfids to emit warning messages and patch zero id for missing >>>>> symbols instead of aborting kernel build process. >>>>> >>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>>> >>>> LGTM with one minor comment below. >>>> >>>> Acked-by: Yonghong Song <yhs@fb.com> >>>> >>>>> --- >>>>> tools/bpf/resolve_btfids/main.c | 13 +++++++------ >>>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >>>>> index 3ad9301b0f00..3ea19e33250d 100644 >>>>> --- a/tools/bpf/resolve_btfids/main.c >>>>> +++ b/tools/bpf/resolve_btfids/main.c >>>>> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) >>>>> sh->sh_addralign = expected; >>>>> >>>>> if (gelf_update_shdr(scn, sh) == 0) { >>>>> - printf("FAILED cannot update section header: %s\n", >>>>> + pr_err("FAILED cannot update section header: %s\n", >>>>> elf_errmsg(-1)); >>>>> return -1; >>>>> } >>>>> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) >>>>> >>>>> elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); >>>>> if (!elf) { >>>>> + close(fd); >>>>> pr_err("FAILED cannot create ELF descriptor: %s\n", >>>>> elf_errmsg(-1)); >>>>> return -1; >>>>> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) >>>>> err = libbpf_get_error(btf); >>>>> if (err) { >>>>> pr_err("FAILED: load BTF from %s: %s\n", >>>>> - obj->path, strerror(-err)); >>>>> + obj->btf ?: obj->path, strerror(-err)); >>>> >>>> Why you change "obj->path" to "obj->btf ?: obj->path"? >>>> Note that obj->path cannot be NULL. >>> >>> The diff didn't see the whole picture. Let me quote it here: >>> ``` >>> btf = btf__parse(obj->btf ?: obj->path, NULL); >>> err = libbpf_get_error(btf); >>> if (err) { >>> pr_err("FAILED: load BTF from %s: %s\n", >>> obj->path, strerror(-err)); >>> return -1; >>> } >>> ``` >>> >>> Because btf__parse parses either obj->btf or obj->path, >>> I think the error message should reveal this. >> >> Okay, I see, obj->btf may not be NULL due to >> OPT_STRING(0, "btf", &obj.btf, "BTF data", >> "BTF data"), >> >> How about >> obj->btf ? "input BTF data" : obj->path >> >> The error message like >> FAILED: load BTF from : <error msg> >> does not sound good. >> > > Sorry, I am confused. > > If obj->btf is set, say, vmlinux.btf, the message should look like: > FAILED: load BTF from vmlinux.btf: <error msg> > > Otherwise, it should look like: > FAILED: load BTF from vmlinux: <error msg> > > Am I missing something ? Ah, you are right. Your patch looks good. I didn't pay attention and thought OPT_STRING(0, "btf", &obj.btf, "BTF data","BTF data") is a string input for *btf data*, but actually it is actually /sys/kernel/btf/vmlinux. > >>> >>>> >>>>> return -1; >>>>> } >>>>> >>>> [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-25 14:18 ` [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen 2021-07-26 3:32 ` Yonghong Song @ 2021-07-26 20:16 ` Andrii Nakryiko 2021-07-27 2:59 ` Hengqi Chen 1 sibling, 1 reply; 12+ messages in thread From: Andrii Nakryiko @ 2021-07-26 20:16 UTC (permalink / raw) To: Hengqi Chen Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, john fastabend, Jiri Olsa, Yaniv Agman On Sun, Jul 25, 2021 at 7:18 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Kernel functions referenced by .BTF_ids may changed from global to static > and get inlined and thus disappears from BTF. This causes kernel build > failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. > Update resolve_btfids to emit warning messages and patch zero id for missing > symbols instead of aborting kernel build process. > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > tools/bpf/resolve_btfids/main.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > index 3ad9301b0f00..3ea19e33250d 100644 > --- a/tools/bpf/resolve_btfids/main.c > +++ b/tools/bpf/resolve_btfids/main.c > @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) > sh->sh_addralign = expected; > > if (gelf_update_shdr(scn, sh) == 0) { > - printf("FAILED cannot update section header: %s\n", > + pr_err("FAILED cannot update section header: %s\n", > elf_errmsg(-1)); > return -1; > } > @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) > > elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); > if (!elf) { > + close(fd); > pr_err("FAILED cannot create ELF descriptor: %s\n", > elf_errmsg(-1)); > return -1; > @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) > err = libbpf_get_error(btf); > if (err) { > pr_err("FAILED: load BTF from %s: %s\n", > - obj->path, strerror(-err)); > + obj->btf ?: obj->path, strerror(-err)); > return -1; > } > > @@ -555,8 +556,7 @@ static int id_patch(struct object *obj, struct btf_id *id) > int i; > > if (!id->id) { > - pr_err("FAILED unresolved symbol %s\n", id->name); > - return -EINVAL; > + pr_err("WARN: unresolved symbol %s\n", id->name); we should probably give a bit more information for people to get back to us for this. For starters, maybe prefix the message with "resolve_btfids:" so that people at least can grep something relevant? > } > > for (i = 0; i < id->addr_cnt; i++) { > @@ -734,8 +734,9 @@ int main(int argc, const char **argv) > > err = 0; > out: > - if (obj.efile.elf) > + if (obj.efile.elf) { > elf_end(obj.efile.elf); > - close(obj.efile.fd); > + close(obj.efile.fd); > + } > return err; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-26 20:16 ` Andrii Nakryiko @ 2021-07-27 2:59 ` Hengqi Chen 0 siblings, 0 replies; 12+ messages in thread From: Hengqi Chen @ 2021-07-27 2:59 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, john fastabend, Jiri Olsa, Yaniv Agman On 2021/7/27 4:16 AM, Andrii Nakryiko wrote: > On Sun, Jul 25, 2021 at 7:18 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> Kernel functions referenced by .BTF_ids may changed from global to static >> and get inlined and thus disappears from BTF. This causes kernel build >> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. >> Update resolve_btfids to emit warning messages and patch zero id for missing >> symbols instead of aborting kernel build process. >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- >> tools/bpf/resolve_btfids/main.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >> index 3ad9301b0f00..3ea19e33250d 100644 >> --- a/tools/bpf/resolve_btfids/main.c >> +++ b/tools/bpf/resolve_btfids/main.c >> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) >> sh->sh_addralign = expected; >> >> if (gelf_update_shdr(scn, sh) == 0) { >> - printf("FAILED cannot update section header: %s\n", >> + pr_err("FAILED cannot update section header: %s\n", >> elf_errmsg(-1)); >> return -1; >> } >> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) >> >> elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); >> if (!elf) { >> + close(fd); >> pr_err("FAILED cannot create ELF descriptor: %s\n", >> elf_errmsg(-1)); >> return -1; >> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) >> err = libbpf_get_error(btf); >> if (err) { >> pr_err("FAILED: load BTF from %s: %s\n", >> - obj->path, strerror(-err)); >> + obj->btf ?: obj->path, strerror(-err)); >> return -1; >> } >> >> @@ -555,8 +556,7 @@ static int id_patch(struct object *obj, struct btf_id *id) >> int i; >> >> if (!id->id) { >> - pr_err("FAILED unresolved symbol %s\n", id->name); >> - return -EINVAL; >> + pr_err("WARN: unresolved symbol %s\n", id->name); > > we should probably give a bit more information for people to get back > to us for this. For starters, maybe prefix the message with > "resolve_btfids:" so that people at least can grep something relevant? > OK, will do. >> } >> >> for (i = 0; i < id->addr_cnt; i++) { >> @@ -734,8 +734,9 @@ int main(int argc, const char **argv) >> >> err = 0; >> out: >> - if (obj.efile.elf) >> + if (obj.efile.elf) { >> elf_end(obj.efile.elf); >> - close(obj.efile.fd); >> + close(obj.efile.fd); >> + } >> return err; >> } >> -- >> 2.25.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-07-25 14:18 [PATCH bpf-next 0/2] expand bpf_d_path helper allowlist Hengqi Chen 2021-07-25 14:18 ` [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen @ 2021-07-25 14:18 ` Hengqi Chen 2021-07-26 6:20 ` Yonghong Song 1 sibling, 1 reply; 12+ messages in thread From: Hengqi Chen @ 2021-07-25 14:18 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen Add vfs_* and security_* to bpf_d_path allowlist, so that we can use bpf_d_path helper to extract full file path from these functions' `struct path *` and `struct file *` arguments. This will help tools like IOVisor's filetop[2]/filelife to get full file path. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index c5e0b6a64091..355777b5bf63 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -850,16 +850,64 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) BTF_SET_START(btf_allowlist_d_path) #ifdef CONFIG_SECURITY BTF_ID(func, security_file_permission) -BTF_ID(func, security_inode_getattr) BTF_ID(func, security_file_open) +BTF_ID(func, security_file_ioctl) +BTF_ID(func, security_file_free) +BTF_ID(func, security_file_alloc) +BTF_ID(func, security_file_lock) +BTF_ID(func, security_file_fcntl) +BTF_ID(func, security_file_set_fowner) +BTF_ID(func, security_file_receive) +BTF_ID(func, security_inode_getattr) +BTF_ID(func, security_sb_mount) +BTF_ID(func, security_bprm_check) #endif #ifdef CONFIG_SECURITY_PATH BTF_ID(func, security_path_truncate) +BTF_ID(func, security_path_notify) +BTF_ID(func, security_path_unlink) +BTF_ID(func, security_path_mkdir) +BTF_ID(func, security_path_rmdir) +BTF_ID(func, security_path_mknod) +BTF_ID(func, security_path_symlink) +BTF_ID(func, security_path_link) +BTF_ID(func, security_path_rename) +BTF_ID(func, security_path_chmod) +BTF_ID(func, security_path_chown) +BTF_ID(func, security_path_chroot) #endif BTF_ID(func, vfs_truncate) BTF_ID(func, vfs_fallocate) -BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) +BTF_ID(func, vfs_fadvise) +BTF_ID(func, vfs_fchmod) +BTF_ID(func, vfs_fchown) +BTF_ID(func, vfs_open) +BTF_ID(func, vfs_setpos) +BTF_ID(func, vfs_llseek) +BTF_ID(func, vfs_read) +BTF_ID(func, vfs_write) +BTF_ID(func, vfs_iocb_iter_read) +BTF_ID(func, vfs_iter_read) +BTF_ID(func, vfs_readv) +BTF_ID(func, vfs_iocb_iter_write) +BTF_ID(func, vfs_iter_write) +BTF_ID(func, vfs_writev) +BTF_ID(func, vfs_copy_file_range) +BTF_ID(func, vfs_getattr_nosec) +BTF_ID(func, vfs_ioctl) +BTF_ID(func, vfs_fsync_range) +BTF_ID(func, vfs_fsync) +BTF_ID(func, vfs_utimes) +BTF_ID(func, vfs_statfs) +BTF_ID(func, vfs_dedupe_file_range_one) +BTF_ID(func, vfs_dedupe_file_range) +BTF_ID(func, vfs_clone_file_range) +BTF_ID(func, vfs_cancel_lock) +BTF_ID(func, vfs_test_lock) +BTF_ID(func, vfs_setlease) +BTF_ID(func, vfs_lock_file) +BTF_ID(func, dentry_open) BTF_ID(func, filp_close) BTF_SET_END(btf_allowlist_d_path) -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-07-25 14:18 ` [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen @ 2021-07-26 6:20 ` Yonghong Song 2021-07-26 7:16 ` Hengqi Chen 0 siblings, 1 reply; 12+ messages in thread From: Yonghong Song @ 2021-07-26 6:20 UTC (permalink / raw) To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 7/25/21 7:18 AM, Hengqi Chen wrote: > Add vfs_* and security_* to bpf_d_path allowlist, so that we can use > bpf_d_path helper to extract full file path from these functions' > `struct path *` and `struct file *` arguments. This will help tools > like IOVisor's filetop[2]/filelife to get full file path. Please use bcc intead of IOVisor. What is "[2]" in "filetop[2]"? > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> LGTM with minor comments below. Acked-by: Yonghong Song <yhs@fb.com> > --- > kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 50 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index c5e0b6a64091..355777b5bf63 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -850,16 +850,64 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) > BTF_SET_START(btf_allowlist_d_path) > #ifdef CONFIG_SECURITY > BTF_ID(func, security_file_permission) > -BTF_ID(func, security_inode_getattr) > BTF_ID(func, security_file_open) > +BTF_ID(func, security_file_ioctl) > +BTF_ID(func, security_file_free) > +BTF_ID(func, security_file_alloc) > +BTF_ID(func, security_file_lock) > +BTF_ID(func, security_file_fcntl) > +BTF_ID(func, security_file_set_fowner) > +BTF_ID(func, security_file_receive) > +BTF_ID(func, security_inode_getattr) > +BTF_ID(func, security_sb_mount) > +BTF_ID(func, security_bprm_check) Here and also below "segments" (security_path_* functions, and later vfs_*/dentry_open/filp_close functions), maybe you can list functions with increasing alphabet order. This will make it easy to check whether a particular function exists or not and whether we miss anything. There are more security_bprm_* functions, e.g., security_bprm_creds_from_file, security_bprm_committing_creds and security_bprm_committed_creds. These functions all have "struct linux_binprm *bprm" parameters. Maybe we can add these few functions as well in this round. > #endif > #ifdef CONFIG_SECURITY_PATH > BTF_ID(func, security_path_truncate) > +BTF_ID(func, security_path_notify) > +BTF_ID(func, security_path_unlink) > +BTF_ID(func, security_path_mkdir) > +BTF_ID(func, security_path_rmdir) > +BTF_ID(func, security_path_mknod) > +BTF_ID(func, security_path_symlink) > +BTF_ID(func, security_path_link) > +BTF_ID(func, security_path_rename) > +BTF_ID(func, security_path_chmod) > +BTF_ID(func, security_path_chown) > +BTF_ID(func, security_path_chroot) > #endif > BTF_ID(func, vfs_truncate) > BTF_ID(func, vfs_fallocate) > -BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > +BTF_ID(func, vfs_fadvise) > +BTF_ID(func, vfs_fchmod) > +BTF_ID(func, vfs_fchown) > +BTF_ID(func, vfs_open) > +BTF_ID(func, vfs_setpos) > +BTF_ID(func, vfs_llseek) > +BTF_ID(func, vfs_read) > +BTF_ID(func, vfs_write) > +BTF_ID(func, vfs_iocb_iter_read) > +BTF_ID(func, vfs_iter_read) > +BTF_ID(func, vfs_readv) > +BTF_ID(func, vfs_iocb_iter_write) > +BTF_ID(func, vfs_iter_write) > +BTF_ID(func, vfs_writev) > +BTF_ID(func, vfs_copy_file_range) > +BTF_ID(func, vfs_getattr_nosec) > +BTF_ID(func, vfs_ioctl) > +BTF_ID(func, vfs_fsync_range) > +BTF_ID(func, vfs_fsync) > +BTF_ID(func, vfs_utimes) > +BTF_ID(func, vfs_statfs) > +BTF_ID(func, vfs_dedupe_file_range_one) > +BTF_ID(func, vfs_dedupe_file_range) > +BTF_ID(func, vfs_clone_file_range) > +BTF_ID(func, vfs_cancel_lock) > +BTF_ID(func, vfs_test_lock) > +BTF_ID(func, vfs_setlease) > +BTF_ID(func, vfs_lock_file) I double checked that for the above three lock related functions (vfs_cancel_lock, vfs_test_lock, vfs_lock_file), I double checked d_path does not use these locks, so we should be fine. > +BTF_ID(func, dentry_open) > BTF_ID(func, filp_close) > BTF_SET_END(btf_allowlist_d_path) > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-07-26 6:20 ` Yonghong Song @ 2021-07-26 7:16 ` Hengqi Chen 0 siblings, 0 replies; 12+ messages in thread From: Hengqi Chen @ 2021-07-26 7:16 UTC (permalink / raw) To: Yonghong Song, bpf; +Cc: ast, daniel, andrii, john.fastabend, jolsa, yanivagman On 2021/7/26 2:20 PM, Yonghong Song wrote: > > > On 7/25/21 7:18 AM, Hengqi Chen wrote: >> Add vfs_* and security_* to bpf_d_path allowlist, so that we can use >> bpf_d_path helper to extract full file path from these functions' >> `struct path *` and `struct file *` arguments. This will help tools >> like IOVisor's filetop[2]/filelife to get full file path. > > Please use bcc intead of IOVisor. > What is "[2]" in "filetop[2]"? OK. Some links are missed in the commit messages and version number is not added to subject. > >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > LGTM with minor comments below. > > Acked-by: Yonghong Song <yhs@fb.com> > >> --- >> kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 50 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index c5e0b6a64091..355777b5bf63 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -850,16 +850,64 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) >> BTF_SET_START(btf_allowlist_d_path) >> #ifdef CONFIG_SECURITY >> BTF_ID(func, security_file_permission) >> -BTF_ID(func, security_inode_getattr) >> BTF_ID(func, security_file_open) >> +BTF_ID(func, security_file_ioctl) >> +BTF_ID(func, security_file_free) >> +BTF_ID(func, security_file_alloc) >> +BTF_ID(func, security_file_lock) >> +BTF_ID(func, security_file_fcntl) >> +BTF_ID(func, security_file_set_fowner) >> +BTF_ID(func, security_file_receive) >> +BTF_ID(func, security_inode_getattr) >> +BTF_ID(func, security_sb_mount) >> +BTF_ID(func, security_bprm_check) > > Here and also below "segments" (security_path_* functions, and > later vfs_*/dentry_open/filp_close functions), > maybe you can list functions with increasing alphabet order. > This will make it easy to check whether a particular function > exists or not and whether we miss anything. > > There are more security_bprm_* functions, e.g., > security_bprm_creds_from_file, security_bprm_committing_creds > and security_bprm_committed_creds. > These functions all have "struct linux_binprm *bprm" > parameters. Maybe we can add these few functions as well > in this round. > Thanks. Will send a v4 for review. >> #endif >> #ifdef CONFIG_SECURITY_PATH >> BTF_ID(func, security_path_truncate) >> +BTF_ID(func, security_path_notify) >> +BTF_ID(func, security_path_unlink) >> +BTF_ID(func, security_path_mkdir) >> +BTF_ID(func, security_path_rmdir) >> +BTF_ID(func, security_path_mknod) >> +BTF_ID(func, security_path_symlink) >> +BTF_ID(func, security_path_link) >> +BTF_ID(func, security_path_rename) >> +BTF_ID(func, security_path_chmod) >> +BTF_ID(func, security_path_chown) >> +BTF_ID(func, security_path_chroot) >> #endif >> BTF_ID(func, vfs_truncate) >> BTF_ID(func, vfs_fallocate) >> -BTF_ID(func, dentry_open) >> BTF_ID(func, vfs_getattr) >> +BTF_ID(func, vfs_fadvise) >> +BTF_ID(func, vfs_fchmod) >> +BTF_ID(func, vfs_fchown) >> +BTF_ID(func, vfs_open) >> +BTF_ID(func, vfs_setpos) >> +BTF_ID(func, vfs_llseek) >> +BTF_ID(func, vfs_read) >> +BTF_ID(func, vfs_write) >> +BTF_ID(func, vfs_iocb_iter_read) >> +BTF_ID(func, vfs_iter_read) >> +BTF_ID(func, vfs_readv) >> +BTF_ID(func, vfs_iocb_iter_write) >> +BTF_ID(func, vfs_iter_write) >> +BTF_ID(func, vfs_writev) >> +BTF_ID(func, vfs_copy_file_range) >> +BTF_ID(func, vfs_getattr_nosec) >> +BTF_ID(func, vfs_ioctl) >> +BTF_ID(func, vfs_fsync_range) >> +BTF_ID(func, vfs_fsync) >> +BTF_ID(func, vfs_utimes) >> +BTF_ID(func, vfs_statfs) >> +BTF_ID(func, vfs_dedupe_file_range_one) >> +BTF_ID(func, vfs_dedupe_file_range) >> +BTF_ID(func, vfs_clone_file_range) >> +BTF_ID(func, vfs_cancel_lock) >> +BTF_ID(func, vfs_test_lock) >> +BTF_ID(func, vfs_setlease) >> +BTF_ID(func, vfs_lock_file) > > I double checked that for the above three lock > related functions (vfs_cancel_lock, vfs_test_lock, > vfs_lock_file), I double checked d_path > does not use these locks, so we should be fine. > >> +BTF_ID(func, dentry_open) >> BTF_ID(func, filp_close) >> BTF_SET_END(btf_allowlist_d_path) >> >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-07-27 2:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-25 14:18 [PATCH bpf-next 0/2] expand bpf_d_path helper allowlist Hengqi Chen 2021-07-25 14:18 ` [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen 2021-07-26 3:32 ` Yonghong Song 2021-07-26 4:41 ` Hengqi Chen 2021-07-26 4:56 ` Yonghong Song 2021-07-26 5:22 ` Hengqi Chen 2021-07-26 5:33 ` Yonghong Song 2021-07-26 20:16 ` Andrii Nakryiko 2021-07-27 2:59 ` Hengqi Chen 2021-07-25 14:18 ` [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen 2021-07-26 6:20 ` Yonghong Song 2021-07-26 7:16 ` Hengqi Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.