[bpf-next,2/2] bpf: Check link_create parameter for multi_uprobe

Commit Message

Tao Chen March 31, 2025, 9:47 a.m. UTC

The target_fd and flags in link_create no used in multi_uprobe
, return -EINVAL if they assigned, keep it same as other link
attach apis.

Fixes: 89ae89f53d20 ("bpf: Add multi uprobe link")
Signed-off-by: Tao Chen <[e-mail]>
---
 kernel/trace/bpf_trace.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jiri Olsa April 1, 2025, 11:03 a.m. UTC | #1

On Mon, Mar 31, 2025 at 05:47:45PM +0800, Tao Chen wrote:
> The target_fd and flags in link_create no used in multi_uprobe
> , return -EINVAL if they assigned, keep it same as other link
> attach apis.
> 
> Fixes: 89ae89f53d20 ("bpf: Add multi uprobe link")
> Signed-off-by: Tao Chen <[e-mail]>
> ---
>  kernel/trace/bpf_trace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 2f206a2a2..f7ebf17e3 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3385,6 +3385,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	if (sizeof(u64) != sizeof(void *))
>  		return -EOPNOTSUPP;
>  
> +	if (attr->link_create.target_fd || attr->link_create.flags)
> +		return -EINVAL;

I think the CI is failing because usdt code does uprobe multi detection
with target_fd = -1 and it fails and perf-uprobe fallback will fail on
not having enough file descriptors

but I think at this stage we will brake some user apps by introducing
this check, link ebpf go library, which passes 0

jirka


> +
>  	if (!is_uprobe_multi(prog))
>  		return -EINVAL;
>  
> -- 
> 2.43.0
>

Tao Chen April 1, 2025, 12:40 p.m. UTC | #2

在 2025/4/1 19:03, Jiri Olsa 写道:
> On Mon, Mar 31, 2025 at 05:47:45PM +0800, Tao Chen wrote:
>> The target_fd and flags in link_create no used in multi_uprobe
>> , return -EINVAL if they assigned, keep it same as other link
>> attach apis.
>>
>> Fixes: 89ae89f53d20 ("bpf: Add multi uprobe link")
>> Signed-off-by: Tao Chen <[e-mail]>
>> ---
>>   kernel/trace/bpf_trace.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 2f206a2a2..f7ebf17e3 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -3385,6 +3385,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>   	if (sizeof(u64) != sizeof(void *))
>>   		return -EOPNOTSUPP;
>>   
>> +	if (attr->link_create.target_fd || attr->link_create.flags)
>> +		return -EINVAL;
> 
> I think the CI is failing because usdt code does uprobe multi detection
> with target_fd = -1 and it fails and perf-uprobe fallback will fail on
> not having enough file descriptors
> 

Hi jiri

As you said, i found it, thanks.

static int probe_uprobe_multi_link(int token_fd)
{
         LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
                 .expected_attach_type = BPF_TRACE_UPROBE_MULTI,
                 .token_fd = token_fd,
                 .prog_flags = token_fd ? BPF_F_TOKEN_FD : 0,
         );
         LIBBPF_OPTS(bpf_link_create_opts, link_opts);
         struct bpf_insn insns[] = {
                 BPF_MOV64_IMM(BPF_REG_0, 0),
                 BPF_EXIT_INSN(),
         };
         int prog_fd, link_fd, err;
         unsigned long offset = 0;

         prog_fd = bpf_prog_load(BPF_PROG_TYPE_KPROBE, NULL, "GPL",
                                 insns, ARRAY_SIZE(insns), &load_opts);
         if (prog_fd < 0)
                 return -errno;

         /* Creating uprobe in '/' binary should fail with -EBADF. */
         link_opts.uprobe_multi.path = "/";
         link_opts.uprobe_multi.offsets = &offset;
         link_opts.uprobe_multi.cnt = 1;

         link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, 
&link_opts);

> but I think at this stage we will brake some user apps by introducing
> this check, link ebpf go library, which passes 0
> 

So is it ok just check the flags?

> jirka
> 
> 
>> +
>>   	if (!is_uprobe_multi(prog))
>>   		return -EINVAL;
>>   
>> -- 
>> 2.43.0
>>

Andrii Nakryiko April 1, 2025, 10:06 p.m. UTC | #3

On Tue, Apr 1, 2025 at 5:40 AM Tao Chen <[e-mail]> wrote:
>
> 在 2025/4/1 19:03, Jiri Olsa 写道:
> > On Mon, Mar 31, 2025 at 05:47:45PM +0800, Tao Chen wrote:
> >> The target_fd and flags in link_create no used in multi_uprobe
> >> , return -EINVAL if they assigned, keep it same as other link
> >> attach apis.
> >>
> >> Fixes: 89ae89f53d20 ("bpf: Add multi uprobe link")
> >> Signed-off-by: Tao Chen <[e-mail]>
> >> ---
> >>   kernel/trace/bpf_trace.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index 2f206a2a2..f7ebf17e3 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -3385,6 +3385,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >>      if (sizeof(u64) != sizeof(void *))
> >>              return -EOPNOTSUPP;
> >>
> >> +    if (attr->link_create.target_fd || attr->link_create.flags)
> >> +            return -EINVAL;
> >
> > I think the CI is failing because usdt code does uprobe multi detection
> > with target_fd = -1 and it fails and perf-uprobe fallback will fail on
> > not having enough file descriptors
> >
>
> Hi jiri
>
> As you said, i found it, thanks.
>
> static int probe_uprobe_multi_link(int token_fd)
> {
>          LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
>                  .expected_attach_type = BPF_TRACE_UPROBE_MULTI,
>                  .token_fd = token_fd,
>                  .prog_flags = token_fd ? BPF_F_TOKEN_FD : 0,
>          );
>          LIBBPF_OPTS(bpf_link_create_opts, link_opts);
>          struct bpf_insn insns[] = {
>                  BPF_MOV64_IMM(BPF_REG_0, 0),
>                  BPF_EXIT_INSN(),
>          };
>          int prog_fd, link_fd, err;
>          unsigned long offset = 0;
>
>          prog_fd = bpf_prog_load(BPF_PROG_TYPE_KPROBE, NULL, "GPL",
>                                  insns, ARRAY_SIZE(insns), &load_opts);
>          if (prog_fd < 0)
>                  return -errno;
>
>          /* Creating uprobe in '/' binary should fail with -EBADF. */
>          link_opts.uprobe_multi.path = "/";
>          link_opts.uprobe_multi.offsets = &offset;
>          link_opts.uprobe_multi.cnt = 1;
>
>          link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI,
> &link_opts);
>
> > but I think at this stage we will brake some user apps by introducing
> > this check, link ebpf go library, which passes 0
> >
>
> So is it ok just check the flags?

good catch, Jiri! Yep, let's validate just flags?

pw-bot: cr

>
> > jirka
> >
> >
> >> +
> >>      if (!is_uprobe_multi(prog))
> >>              return -EINVAL;
> >>
> >> --
> >> 2.43.0
> >>
>
>
> --
> Best Regards
> Tao Chen
>

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 2f206a2a2..f7ebf17e3 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3385,6 +3385,9 @@  int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (sizeof(u64) != sizeof(void *))
 		return -EOPNOTSUPP;
 
+	if (attr->link_create.target_fd || attr->link_create.flags)
+		return -EINVAL;
+
 	if (!is_uprobe_multi(prog))
 		return -EINVAL;