On Thu, Jul 06, 2023 at 03:34:10PM -0700, Andrii Nakryiko wrote:
SNIP
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 60a9d59beeab..a236139f08ce 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1036,6 +1036,7 @@ enum bpf_attach_type {
> > BPF_LSM_CGROUP,
> > BPF_STRUCT_OPS,
> > BPF_NETFILTER,
> > + BPF_TRACE_UPROBE_MULTI,
> > __MAX_BPF_ATTACH_TYPE
> > };
> >
> > @@ -1053,6 +1054,7 @@ enum bpf_link_type {
> > BPF_LINK_TYPE_KPROBE_MULTI = 8,
> > BPF_LINK_TYPE_STRUCT_OPS = 9,
> > BPF_LINK_TYPE_NETFILTER = 10,
> > + BPF_LINK_TYPE_UPROBE_MULTI = 11,
> >
> > MAX_BPF_LINK_TYPE,
> > };
> > @@ -1170,6 +1172,11 @@ enum bpf_link_type {
> > */
> > #define BPF_F_KPROBE_MULTI_RETURN (1U << 0)
> >
> > +/* link_create.uprobe_multi.flags used in LINK_CREATE command for
> > + * BPF_TRACE_UPROBE_MULTI attach type to create return probe.
> > + */
> > +#define BPF_F_UPROBE_MULTI_RETURN (1U << 0)
> > +
>
> any reason why we don't use anonymous ENUMs for all these UAPI
> constants? When we need to use these flags from BPF side (e.g., for
> BPF LSM), having them as #defines will be a PITA, as they won't be
> present in vmlinux.h
ugh right, we already did that before.. will change
>
>
> > /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
> > * the following extensions:
> > *
> > @@ -1579,6 +1586,13 @@ union bpf_attr {
> > __s32 priority;
> > __u32 flags;
> > } netfilter;
> > + struct {
> > + __u32 flags;
> > + __u32 cnt;
>
> total nit, but I'd move it after path/offsets/ref_ctr_offsets, and
> make the order cnt (as it applies to previous two
> offsets/ref_ctr_offsets) and then flags last. Seems like more logical
> order, but totally subjective
ok
>
> > + __aligned_u64 path;
> > + __aligned_u64 offsets;
> > + __aligned_u64 ref_ctr_offsets;
> > + } uprobe_multi;
> > };
> > } link_create;
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 9046ad0f9b4e..3b0582a64ce4 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2813,10 +2813,12 @@ static void bpf_link_free_id(int id)
> >
> > /* Clean up bpf_link and corresponding anon_inode file and FD. After
> > * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> > - * anon_inode's release() call. This helper marksbpf_link as
> > + * anon_inode's release() call. This helper marks bpf_link as
> > * defunct, releases anon_inode file and puts reserved FD. bpf_prog's refcnt
> > * is not decremented, it's the responsibility of a calling code that failed
> > * to complete bpf_link initialization.
> > + * This helper eventually calls link's dealloc callback, but does not call
> > + * link's release callback.
>
> Thanks for clarifying comments!
>
> > */
> > void bpf_link_cleanup(struct bpf_link_primer *primer)
> > {
> > @@ -3589,8 +3591,12 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> > if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI &&
> > attach_type != BPF_TRACE_KPROBE_MULTI)
> > return -EINVAL;
> > + if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI &&
> > + attach_type != BPF_TRACE_UPROBE_MULTI)
> > + return -EINVAL;
> > if (attach_type != BPF_PERF_EVENT &&
> > - attach_type != BPF_TRACE_KPROBE_MULTI)
> > + attach_type != BPF_TRACE_KPROBE_MULTI &&
> > + attach_type != BPF_TRACE_UPROBE_MULTI)
>
> if this keeps growing, we should think about having a switch in a
> switch to not repeat BPF_TRACE_UPROBE_MULTI and BPF_TRACE_KPROBE_MULTI
> twice
ok
SNIP
> > + for (i = 0; i < cnt; i++) {
> > + err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> > + uprobes[i].offset,
> > + ref_ctr_offsets ? ref_ctr_offsets[i] : 0,
> > + &uprobes[i].consumer);
> > + if (err) {
> > + bpf_uprobe_unregister(&path, uprobes, i);
> > + bpf_link_cleanup(&link_primer);
> > + kvfree(ref_ctr_offsets);
>
> are we missing path_put() in this error handling path? so maybe goto
> error_path_put here instead of return?
aaaah right path_put needs to go to dealloc callback :-\ will change, thanks
jirka
