diff mbox series

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

Message ID 20250331094745.336010-2-chen.dylane@linux.dev (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,1/2] bpf: Check link_create parameter for multi_kprobe | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: mattbobrowski@google.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 fail Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 fail Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-27 fail Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-46 fail Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-47 fail Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 fail Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

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 <chen.dylane@linux.dev>
---
 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 <chen.dylane@linux.dev>
> ---
>  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 <chen.dylane@linux.dev>
>> ---
>>   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 <chen.dylane@linux.dev> 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 <chen.dylane@linux.dev>
> >> ---
> >>   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
>
Jiri Olsa April 2, 2025, 9:01 a.m. UTC | #4 
On Tue, Apr 01, 2025 at 03:06:22PM -0700, Andrii Nakryiko wrote:
> On Tue, Apr 1, 2025 at 5:40 AM Tao Chen <chen.dylane@linux.dev> 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 <chen.dylane@linux.dev>
> > >> ---
> > >>   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?

I think so.. I'll test that with ebpf/go to make sure we are safe
at least there ;-) I'll let you know

jirka

> 
> pw-bot: cr
> 
> >
> > > jirka
> > >
> > >
> > >> +
> > >>      if (!is_uprobe_multi(prog))
> > >>              return -EINVAL;
> > >>
> > >> --
> > >> 2.43.0
> > >>
> >
> >
> > --
> > Best Regards
> > Tao Chen
> >
Jiri Olsa April 2, 2025, 7:19 p.m. UTC | #5 
On Wed, Apr 02, 2025 at 11:01:48AM +0200, Jiri Olsa wrote:
> On Tue, Apr 01, 2025 at 03:06:22PM -0700, Andrii Nakryiko wrote:
> > On Tue, Apr 1, 2025 at 5:40 AM Tao Chen <chen.dylane@linux.dev> 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 <chen.dylane@linux.dev>
> > > >> ---
> > > >>   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?
> 
> I think so.. I'll test that with ebpf/go to make sure we are safe
> at least there ;-) I'll let you know

sorry, got stuck.. link_create.flags are initialized to zero,
so I think flags check should be fine (at least for ebpf/go)

jirka
Jiri Olsa April 2, 2025, 7:25 p.m. UTC | #6 
On Wed, Apr 02, 2025 at 09:19:45PM +0200, Jiri Olsa wrote:
> On Wed, Apr 02, 2025 at 11:01:48AM +0200, Jiri Olsa wrote:
> > On Tue, Apr 01, 2025 at 03:06:22PM -0700, Andrii Nakryiko wrote:
> > > On Tue, Apr 1, 2025 at 5:40 AM Tao Chen <chen.dylane@linux.dev> 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 <chen.dylane@linux.dev>
> > > > >> ---
> > > > >>   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?
> > 
> > I think so.. I'll test that with ebpf/go to make sure we are safe
> > at least there ;-) I'll let you know
> 
> sorry, got stuck.. link_create.flags are initialized to zero,
> so I think flags check should be fine (at least for ebpf/go)

sry forgot.. adding Timo to the loop (ebpf/go)

jirka
diff mbox series

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;