| Message ID | 20201215233702.3301881-3-songliubraving@fb.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce bpf_iter for task_vma | expand |
On 12/15/20 3:37 PM, Song Liu wrote: > task_file and task_vma iter programs have access to file->f_path. Enable > bpf_d_path to print paths of these file. > > bpf_iter programs are generally called in sleepable context. However, it > is still necessary to diffientiate sleepable and non-sleepable bpf_iter > programs: sleepable programs have access to bpf_d_path; non-sleepable > programs have access to bpf_spin_lock. > > Signed-off-by: Song Liu <songliubraving@fb.com> Agreed. So far bpf_iter programs all called from process context. Acked-by: Yonghong Song <yhs@fb.com>
On Wed, Dec 16, 2020 at 1:06 AM Song Liu <songliubraving@fb.com> wrote: > > task_file and task_vma iter programs have access to file->f_path. Enable > bpf_d_path to print paths of these file. > > bpf_iter programs are generally called in sleepable context. However, it > is still necessary to diffientiate sleepable and non-sleepable bpf_iter > programs: sleepable programs have access to bpf_d_path; non-sleepable > programs have access to bpf_spin_lock. > > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > kernel/trace/bpf_trace.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 4be771df5549a..9e5f9b968355f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1191,6 +1191,11 @@ BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > { > + if (prog->type == BPF_PROG_TYPE_TRACING && > + prog->expected_attach_type == BPF_TRACE_ITER && > + prog->aux->sleepable) > + return true; For the sleepable/non-sleepable we have been (until now) checking this in bpf_tracing_func_proto (or bpf_lsm_func_proto) eg. case BPF_FUNC_copy_from_user: return prog->aux->sleepable ? &bpf_copy_from_user_proto : NULL; But even beyond that, I don't think this is needed. We have originally exposed the helper to both sleepable and non-sleepable LSM and tracing programs with an allow list. For LSM the allow list is bpf_lsm_is_sleepable_hook) but that's just an initial allow list and thus causes some confusion w.r.t to sleep ability (maybe we should add a comment there). Based on the current logic, my understanding is that it's okay to use the helper in the allowed hooks in both "lsm.s/" and "lsm/" (and the same for BPF_PROG_TYPE_TRACING). We would have required sleepable only if this helper called "dput" (which can sleep). > + > if (prog->type == BPF_PROG_TYPE_LSM) > return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id); > > -- > 2.24.1 >
On Wed, Dec 16, 2020 at 7:15 PM KP Singh <kpsingh@kernel.org> wrote: > > On Wed, Dec 16, 2020 at 1:06 AM Song Liu <songliubraving@fb.com> wrote: > > > > task_file and task_vma iter programs have access to file->f_path. Enable > > bpf_d_path to print paths of these file. > > > > bpf_iter programs are generally called in sleepable context. However, it > > is still necessary to diffientiate sleepable and non-sleepable bpf_iter > > programs: sleepable programs have access to bpf_d_path; non-sleepable > > programs have access to bpf_spin_lock. > > > > Signed-off-by: Song Liu <songliubraving@fb.com> > > --- > > kernel/trace/bpf_trace.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 4be771df5549a..9e5f9b968355f 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1191,6 +1191,11 @@ BTF_SET_END(btf_allowlist_d_path) > > > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > > { > > + if (prog->type == BPF_PROG_TYPE_TRACING && > > + prog->expected_attach_type == BPF_TRACE_ITER && > > + prog->aux->sleepable) > > + return true; > Another try to send it on the list. > For the sleepable/non-sleepable we have been (until now) checking > this in bpf_tracing_func_proto (or bpf_lsm_func_proto) > > eg. > > case BPF_FUNC_copy_from_user: > return prog->aux->sleepable ? &bpf_copy_from_user_proto : NULL; > > But even beyond that, I don't think this is needed. > > We have originally exposed the helper to both sleepable and > non-sleepable LSM and tracing programs with an allow list. > > For LSM the allow list is bpf_lsm_is_sleepable_hook) but > that's just an initial allow list and thus causes some confusion > w.r.t to sleep ability (maybe we should add a comment there). > > Based on the current logic, my understanding is that > it's okay to use the helper in the allowed hooks in both > "lsm.s/" and "lsm/" (and the same for > BPF_PROG_TYPE_TRACING). > > We would have required sleepable only if this helper called "dput" > (which can sleep). > > > + > > if (prog->type == BPF_PROG_TYPE_LSM) > > return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id); > > > > -- > > 2.24.1 > >
On Tue, Dec 15, 2020 at 03:37:00PM -0800, Song Liu wrote: > task_file and task_vma iter programs have access to file->f_path. Enable > bpf_d_path to print paths of these file. > > bpf_iter programs are generally called in sleepable context. However, it > is still necessary to diffientiate sleepable and non-sleepable bpf_iter > programs: sleepable programs have access to bpf_d_path; non-sleepable > programs have access to bpf_spin_lock. > > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > kernel/trace/bpf_trace.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 4be771df5549a..9e5f9b968355f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1191,6 +1191,11 @@ BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > { > + if (prog->type == BPF_PROG_TYPE_TRACING && > + prog->expected_attach_type == BPF_TRACE_ITER && > + prog->aux->sleepable) > + return true; > + > if (prog->type == BPF_PROG_TYPE_LSM) > return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id); > > -- > 2.24.1 > would be great to have this merged for bpftrace Tested-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 4be771df5549a..9e5f9b968355f 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1191,6 +1191,11 @@ BTF_SET_END(btf_allowlist_d_path) static bool bpf_d_path_allowed(const struct bpf_prog *prog) { + if (prog->type == BPF_PROG_TYPE_TRACING && + prog->expected_attach_type == BPF_TRACE_ITER && + prog->aux->sleepable) + return true; + if (prog->type == BPF_PROG_TYPE_LSM) return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id);
task_file and task_vma iter programs have access to file->f_path. Enable bpf_d_path to print paths of these file. bpf_iter programs are generally called in sleepable context. However, it is still necessary to diffientiate sleepable and non-sleepable bpf_iter programs: sleepable programs have access to bpf_d_path; non-sleepable programs have access to bpf_spin_lock. Signed-off-by: Song Liu <songliubraving@fb.com> --- kernel/trace/bpf_trace.c | 5 +++++ 1 file changed, 5 insertions(+)