| Message ID | 20230830093502.1436694-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | [RFC/PATCH,bpf-next] bpf: Fix d_path test after last fs update | expand |
On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > Recent commit [1] broken d_path test, because now filp_close is not > called directly from sys_close, but eventually later when the file > is finally released. > > I can't see any other solution than to hook filp_flush function and > that also means we need to add it to btf_allowlist_d_path list, so > it can use the d_path helper. > > But it's probably not very stable because filp_flush is static so it > could be potentially inlined. looks like llvm makes it inlined (from CI) Error: #68/1 d_path/basic libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 jirka > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > for it to be called so user space can go with checks, then it looks > like d_path might not work properly when the task is no longer around. > > thoughts? > jirka > > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)") > --- > kernel/trace/bpf_trace.c | 1 + > tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index a7264b2c17ad..c829e24af246 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > +BTF_ID(func, filp_flush) > BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c > index 84e1f883f97b..3467d1b8098c 100644 > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, > return 0; > } > > -SEC("fentry/filp_close") > -int BPF_PROG(prog_close, struct file *file, void *id) > +SEC("fentry/filp_flush") > +int BPF_PROG(prog_close, struct file *file) > { > pid_t pid = bpf_get_current_pid_tgid() >> 32; > __u32 cnt = cnt_close; > -- > 2.41.0 >
On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > Recent commit [1] broken d_path test, because now filp_close is not > > called directly from sys_close, but eventually later when the file > > is finally released. > > > > I can't see any other solution than to hook filp_flush function and > > that also means we need to add it to btf_allowlist_d_path list, so > > it can use the d_path helper. > > > > But it's probably not very stable because filp_flush is static so it > > could be potentially inlined. > > looks like llvm makes it inlined (from CI) > > Error: #68/1 d_path/basic > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > jirka I played with it for a bit, but haven't got a good solution. Maybe we should just remove the test for close()? Thanks, Song > > > > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > > for it to be called so user space can go with checks, then it looks > > like d_path might not work properly when the task is no longer around. [...]
On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote: > On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > > Recent commit [1] broken d_path test, because now filp_close is not > > > called directly from sys_close, but eventually later when the file > > > is finally released. > > > > > > I can't see any other solution than to hook filp_flush function and > > > that also means we need to add it to btf_allowlist_d_path list, so > > > it can use the d_path helper. > > > > > > But it's probably not very stable because filp_flush is static so it > > > could be potentially inlined. > > > > looks like llvm makes it inlined (from CI) > > > > Error: #68/1 d_path/basic > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > jirka > > I played with it for a bit, but haven't got a good solution. Maybe we should > just remove the test for close()? I was thinking the same.. also we have some example with filp_close in bpftrace docs, I think we'll need to add some note with explanation in there jirka > > Thanks, > Song > > > > > > > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > > > for it to be called so user space can go with checks, then it looks > > > like d_path might not work properly when the task is no longer around. > > [...]
On Wed, Aug 30, 2023 at 12:04 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote: > > On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > > > Recent commit [1] broken d_path test, because now filp_close is not > > > > called directly from sys_close, but eventually later when the file > > > > is finally released. > > > > > > > > I can't see any other solution than to hook filp_flush function and > > > > that also means we need to add it to btf_allowlist_d_path list, so > > > > it can use the d_path helper. > > > > > > > > But it's probably not very stable because filp_flush is static so it > > > > could be potentially inlined. > > > > > > looks like llvm makes it inlined (from CI) > > > > > > Error: #68/1 d_path/basic > > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > > > jirka > > > > I played with it for a bit, but haven't got a good solution. Maybe we should > > just remove the test for close()? > > I was thinking the same.. also we have some example with filp_close in bpftrace > docs, I think we'll need to add some note with explanation in there Maybe use __x64_sys_close in the test and recommend bpftrace scripts to do the same?
On 8/30/23 9:27 AM, Jiri Olsa wrote: > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: >> Recent commit [1] broken d_path test, because now filp_close is not >> called directly from sys_close, but eventually later when the file >> is finally released. >> >> I can't see any other solution than to hook filp_flush function and >> that also means we need to add it to btf_allowlist_d_path list, so >> it can use the d_path helper. >> >> But it's probably not very stable because filp_flush is static so it >> could be potentially inlined. > > looks like llvm makes it inlined (from CI) > > Error: #68/1 d_path/basic > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > jirka > >> >> Also if we'd keep the current filp_close hook and find a way how to 'wait' >> for it to be called so user space can go with checks, then it looks >> like d_path might not work properly when the task is no longer around. >> >> thoughts? Jiri, The following patch works fine for me: $ git diff diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index a7264b2c17ad..fdeec712338f 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) BTF_ID(func, filp_close) +BTF_ID(func, __fput_sync) BTF_SET_END(btf_allowlist_d_path) static bool bpf_d_path_allowed(const struct bpf_prog *prog) diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c index 84e1f883f97b..672897197c2a 100644 --- a/tools/testing/selftests/bpf/progs/test_d_path.c +++ b/tools/testing/selftests/bpf/progs/test_d_path.c @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, return 0; } -SEC("fentry/filp_close") -int BPF_PROG(prog_close, struct file *file, void *id) +SEC("fentry/__fput_sync") +int BPF_PROG(prog_close, struct file *file) { pid_t pid = bpf_get_current_pid_tgid() >> 32; __u32 cnt = cnt_close; >> jirka >> >> >> [1] 021a160abf62 ("fs: use __fput_sync in close(2)") >> --- >> kernel/trace/bpf_trace.c | 1 + >> tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++-- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index a7264b2c17ad..c829e24af246 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) >> BTF_ID(func, dentry_open) >> BTF_ID(func, vfs_getattr) >> BTF_ID(func, filp_close) >> +BTF_ID(func, filp_flush) >> BTF_SET_END(btf_allowlist_d_path) >> >> static bool bpf_d_path_allowed(const struct bpf_prog *prog) >> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c >> index 84e1f883f97b..3467d1b8098c 100644 >> --- a/tools/testing/selftests/bpf/progs/test_d_path.c >> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c >> @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, >> return 0; >> } >> >> -SEC("fentry/filp_close") >> -int BPF_PROG(prog_close, struct file *file, void *id) >> +SEC("fentry/filp_flush") >> +int BPF_PROG(prog_close, struct file *file) >> { >> pid_t pid = bpf_get_current_pid_tgid() >> 32; >> __u32 cnt = cnt_close; >> -- >> 2.41.0 >> >
On Wed, Aug 30, 2023 at 6:17 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > On 8/30/23 9:27 AM, Jiri Olsa wrote: > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > >> Recent commit [1] broken d_path test, because now filp_close is not > >> called directly from sys_close, but eventually later when the file > >> is finally released. > >> > >> I can't see any other solution than to hook filp_flush function and > >> that also means we need to add it to btf_allowlist_d_path list, so > >> it can use the d_path helper. > >> > >> But it's probably not very stable because filp_flush is static so it > >> could be potentially inlined. > > > > looks like llvm makes it inlined (from CI) > > > > Error: #68/1 d_path/basic > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > jirka > > > >> > >> Also if we'd keep the current filp_close hook and find a way how to 'wait' > >> for it to be called so user space can go with checks, then it looks > >> like d_path might not work properly when the task is no longer around. > >> > >> thoughts? > > Jiri, > > The following patch works fine for me: > > $ git diff > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index a7264b2c17ad..fdeec712338f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > +BTF_ID(func, __fput_sync) > BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c > b/tools/testing/selftests/bpf/progs/test_d_path.c > index 84e1f883f97b..672897197c2a 100644 > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct > kstat *stat, > return 0; > } > > -SEC("fentry/filp_close") > -int BPF_PROG(prog_close, struct file *file, void *id) > +SEC("fentry/__fput_sync") > +int BPF_PROG(prog_close, struct file *file) > { > pid_t pid = bpf_get_current_pid_tgid() >> 32; > __u32 cnt = cnt_close; Yeah, I guess this is the easiest fix at the moment. Related, shall we have resolve_btfids fail for missing ID? Something like: diff --git i/scripts/link-vmlinux.sh w/scripts/link-vmlinux.sh index a432b171be82..9a194152da49 100755 --- i/scripts/link-vmlinux.sh +++ w/scripts/link-vmlinux.sh @@ -274,7 +274,10 @@ vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o} # fill in BTF IDs if is_enabled CONFIG_DEBUG_INFO_BTF && is_enabled CONFIG_BPF; then info BTFIDS vmlinux - ${RESOLVE_BTFIDS} vmlinux + if ! ${RESOLVE_BTFIDS} vmlinux ; then + echo >&2 Failed to resolve BTF IDs + exit 1 + fi fi mksysmap vmlinux System.map ${kallsymso} diff --git i/tools/bpf/resolve_btfids/main.c w/tools/bpf/resolve_btfids/main.c index 27a23196d58e..2940fe004220 100644 --- i/tools/bpf/resolve_btfids/main.c +++ w/tools/bpf/resolve_btfids/main.c @@ -599,8 +599,10 @@ static int id_patch(struct object *obj, struct btf_id *id) int i; /* For set, set8, id->id may be 0 */ - if (!id->id && !id->is_set && !id->is_set8) - pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); + if (!id->id && !id->is_set && !id->is_set8) { + pr_err("FAILED resolve_btfids: unresolved symbol %s\n", id->name); + return -1; + } for (i = 0; i < id->addr_cnt; i++) { unsigned long addr = id->addr[i]; Thanks, Song
Hi On 8/30/2023 5:35 PM, Jiri Olsa wrote: > Recent commit [1] broken d_path test, because now filp_close is not > called directly from sys_close, but eventually later when the file > is finally released. To make test_d_path self-test pass, beside attaching to a different function (e.g., __fput_sync or filp_flush), we could also use close_range() or even dup2() to close the created fd, because these syscalls still use filp_close() to close the opened file. > > I can't see any other solution than to hook filp_flush function and > that also means we need to add it to btf_allowlist_d_path list, so > it can use the d_path helper. > > But it's probably not very stable because filp_flush is static so it > could be potentially inlined. > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > for it to be called so user space can go with checks, then it looks > like d_path might not work properly when the task is no longer around. It seems there is no need to wait for it to be called, because filp_close() is still called synchronously by some syscall (e.g., close_range or io_uring). So if the bpf program tries to collect many close event as possible, it should be attach to both filp_close() and __fput_sync(), right ? > > thoughts? > jirka > > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)") > --- > kernel/trace/bpf_trace.c | 1 + > tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index a7264b2c17ad..c829e24af246 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > +BTF_ID(func, filp_flush) > BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c > index 84e1f883f97b..3467d1b8098c 100644 > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, > return 0; > } > > -SEC("fentry/filp_close") > -int BPF_PROG(prog_close, struct file *file, void *id) > +SEC("fentry/filp_flush") > +int BPF_PROG(prog_close, struct file *file) > { > pid_t pid = bpf_get_current_pid_tgid() >> 32; > __u32 cnt = cnt_close;
On Wed, Aug 30, 2023 at 06:11:52PM -0400, Yonghong Song wrote: > > > On 8/30/23 9:27 AM, Jiri Olsa wrote: > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > > Recent commit [1] broken d_path test, because now filp_close is not > > > called directly from sys_close, but eventually later when the file > > > is finally released. > > > > > > I can't see any other solution than to hook filp_flush function and > > > that also means we need to add it to btf_allowlist_d_path list, so > > > it can use the d_path helper. > > > > > > But it's probably not very stable because filp_flush is static so it > > > could be potentially inlined. > > > > looks like llvm makes it inlined (from CI) > > > > Error: #68/1 d_path/basic > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > jirka > > > > > > > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > > > for it to be called so user space can go with checks, then it looks > > > like d_path might not work properly when the task is no longer around. > > > > > > thoughts? > > Jiri, > > The following patch works fine for me: > > $ git diff > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index a7264b2c17ad..fdeec712338f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > +BTF_ID(func, __fput_sync) hum right, that should work.. there are several callers of that, but AFAICT they all seem ok (not holding mount_lock or rename_lock) to run d_path helper I'll send patch with the change thanks, jirka > BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c > b/tools/testing/selftests/bpf/progs/test_d_path.c > index 84e1f883f97b..672897197c2a 100644 > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat > *stat, > return 0; > } > > -SEC("fentry/filp_close") > -int BPF_PROG(prog_close, struct file *file, void *id) > +SEC("fentry/__fput_sync") > +int BPF_PROG(prog_close, struct file *file) > { > pid_t pid = bpf_get_current_pid_tgid() >> 32; > __u32 cnt = cnt_close; > > > > jirka > > > > > > > > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)") > > > --- > > > kernel/trace/bpf_trace.c | 1 + > > > tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++-- > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > > index a7264b2c17ad..c829e24af246 100644 > > > --- a/kernel/trace/bpf_trace.c > > > +++ b/kernel/trace/bpf_trace.c > > > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > > > BTF_ID(func, dentry_open) > > > BTF_ID(func, vfs_getattr) > > > BTF_ID(func, filp_close) > > > +BTF_ID(func, filp_flush) > > > BTF_SET_END(btf_allowlist_d_path) > > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c > > > index 84e1f883f97b..3467d1b8098c 100644 > > > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > > > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > > > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, > > > return 0; > > > } > > > -SEC("fentry/filp_close") > > > -int BPF_PROG(prog_close, struct file *file, void *id) > > > +SEC("fentry/filp_flush") > > > +int BPF_PROG(prog_close, struct file *file) > > > { > > > pid_t pid = bpf_get_current_pid_tgid() >> 32; > > > __u32 cnt = cnt_close; > > > -- > > > 2.41.0 > > > > >
On Thu, Aug 31, 2023 at 01:24:10AM -0400, Song Liu wrote: > On Wed, Aug 30, 2023 at 6:17 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > > > > > On 8/30/23 9:27 AM, Jiri Olsa wrote: > > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > >> Recent commit [1] broken d_path test, because now filp_close is not > > >> called directly from sys_close, but eventually later when the file > > >> is finally released. > > >> > > >> I can't see any other solution than to hook filp_flush function and > > >> that also means we need to add it to btf_allowlist_d_path list, so > > >> it can use the d_path helper. > > >> > > >> But it's probably not very stable because filp_flush is static so it > > >> could be potentially inlined. > > > > > > looks like llvm makes it inlined (from CI) > > > > > > Error: #68/1 d_path/basic > > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > > > jirka > > > > > >> > > >> Also if we'd keep the current filp_close hook and find a way how to 'wait' > > >> for it to be called so user space can go with checks, then it looks > > >> like d_path might not work properly when the task is no longer around. > > >> > > >> thoughts? > > > > Jiri, > > > > The following patch works fine for me: > > > > $ git diff > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index a7264b2c17ad..fdeec712338f 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) > > BTF_ID(func, dentry_open) > > BTF_ID(func, vfs_getattr) > > BTF_ID(func, filp_close) > > +BTF_ID(func, __fput_sync) > > BTF_SET_END(btf_allowlist_d_path) > > > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c > > b/tools/testing/selftests/bpf/progs/test_d_path.c > > index 84e1f883f97b..672897197c2a 100644 > > --- a/tools/testing/selftests/bpf/progs/test_d_path.c > > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c > > @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct > > kstat *stat, > > return 0; > > } > > > > -SEC("fentry/filp_close") > > -int BPF_PROG(prog_close, struct file *file, void *id) > > +SEC("fentry/__fput_sync") > > +int BPF_PROG(prog_close, struct file *file) > > { > > pid_t pid = bpf_get_current_pid_tgid() >> 32; > > __u32 cnt = cnt_close; > > Yeah, I guess this is the easiest fix at the moment. > > Related, shall we have resolve_btfids fail for missing ID? Something > like: > > diff --git i/scripts/link-vmlinux.sh w/scripts/link-vmlinux.sh > index a432b171be82..9a194152da49 100755 > --- i/scripts/link-vmlinux.sh > +++ w/scripts/link-vmlinux.sh > @@ -274,7 +274,10 @@ vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o} > # fill in BTF IDs > if is_enabled CONFIG_DEBUG_INFO_BTF && is_enabled CONFIG_BPF; then > info BTFIDS vmlinux > - ${RESOLVE_BTFIDS} vmlinux > + if ! ${RESOLVE_BTFIDS} vmlinux ; then > + echo >&2 Failed to resolve BTF IDs > + exit 1 > + fi IIUC link-vmlinux.sh will fail when ${RESOLVE_BTFIDS} returns != 0, and now the unresolved symbol is just a warning we used to have that but we decided to just warn: 5aad03685185 tools/resolve_btfids: Emit warnings and patch zero id for missing symbols jirka
On Thu, Aug 31, 2023 at 03:37:37PM +0800, Hou Tao wrote: > Hi > > On 8/30/2023 5:35 PM, Jiri Olsa wrote: > > Recent commit [1] broken d_path test, because now filp_close is not > > called directly from sys_close, but eventually later when the file > > is finally released. > > To make test_d_path self-test pass, beside attaching to a different > function (e.g., __fput_sync or filp_flush), we could also use > close_range() or even dup2() to close the created fd, because these > syscalls still use filp_close() to close the opened file. nice, I like the close_range solution ;-) patch below fixes the test > > > > > I can't see any other solution than to hook filp_flush function and > > that also means we need to add it to btf_allowlist_d_path list, so > > it can use the d_path helper. > > > > But it's probably not very stable because filp_flush is static so it > > could be potentially inlined. > > > > Also if we'd keep the current filp_close hook and find a way how to 'wait' > > for it to be called so user space can go with checks, then it looks > > like d_path might not work properly when the task is no longer around. > > It seems there is no need to wait for it to be called, because > filp_close() is still called synchronously by some syscall (e.g., > close_range or io_uring). So if the bpf program tries to collect many > close event as possible, it should be attach to both filp_close() and > __fput_sync(), right ? > right, when we hook to close_range it's still synchronous thanks, jirka --- diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c index 911345c526e6..8a558c980753 100644 --- a/tools/testing/selftests/bpf/prog_tests/d_path.c +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c @@ -91,6 +91,7 @@ static int trigger_fstat_events(pid_t pid) out_close: /* triggers filp_close */ +#define close(fd) close_range(fd, fd, 0) close(pipefd[0]); close(pipefd[1]); close(sockfd); @@ -98,6 +99,7 @@ static int trigger_fstat_events(pid_t pid) close(devfd); close(localfd); close(indicatorfd); +#undef close return ret; }
On Wed, Aug 30, 2023 at 01:29:18PM -0700, Alexei Starovoitov wrote: > On Wed, Aug 30, 2023 at 12:04 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote: > > > On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, Jiri Olsa wrote: > > > > > Recent commit [1] broken d_path test, because now filp_close is not > > > > > called directly from sys_close, but eventually later when the file > > > > > is finally released. > > > > > > > > > > I can't see any other solution than to hook filp_flush function and > > > > > that also means we need to add it to btf_allowlist_d_path list, so > > > > > it can use the d_path helper. > > > > > > > > > > But it's probably not very stable because filp_flush is static so it > > > > > could be potentially inlined. > > > > > > > > looks like llvm makes it inlined (from CI) > > > > > > > > Error: #68/1 d_path/basic > > > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3 > > > > > > > > jirka > > > > > > I played with it for a bit, but haven't got a good solution. Maybe we should > > > just remove the test for close()? > > > > I was thinking the same.. also we have some example with filp_close in bpftrace > > docs, I think we'll need to add some note with explanation in there > > Maybe use __x64_sys_close in the test and recommend bpftrace scripts > to do the same? we need struct file pointer as an argument, __x64_sys_close has pt_regs pointer jirka
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index a7264b2c17ad..c829e24af246 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate) BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) BTF_ID(func, filp_close) +BTF_ID(func, filp_flush) BTF_SET_END(btf_allowlist_d_path) static bool bpf_d_path_allowed(const struct bpf_prog *prog) diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c index 84e1f883f97b..3467d1b8098c 100644 --- a/tools/testing/selftests/bpf/progs/test_d_path.c +++ b/tools/testing/selftests/bpf/progs/test_d_path.c @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat, return 0; } -SEC("fentry/filp_close") -int BPF_PROG(prog_close, struct file *file, void *id) +SEC("fentry/filp_flush") +int BPF_PROG(prog_close, struct file *file) { pid_t pid = bpf_get_current_pid_tgid() >> 32; __u32 cnt = cnt_close;
Recent commit [1] broken d_path test, because now filp_close is not called directly from sys_close, but eventually later when the file is finally released. I can't see any other solution than to hook filp_flush function and that also means we need to add it to btf_allowlist_d_path list, so it can use the d_path helper. But it's probably not very stable because filp_flush is static so it could be potentially inlined. Also if we'd keep the current filp_close hook and find a way how to 'wait' for it to be called so user space can go with checks, then it looks like d_path might not work properly when the task is no longer around. thoughts? jirka [1] 021a160abf62 ("fs: use __fput_sync in close(2)") --- kernel/trace/bpf_trace.c | 1 + tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)