diff mbox series

[bpf-next] bpf: update docs on CONFIG_FUNCTION_ERROR_INJECTION

Message ID 20241010184556.985660-1-martin.kelly@crowdstrike.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] bpf: update docs on CONFIG_FUNCTION_ERROR_INJECTION | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
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-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 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-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-24 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-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 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-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-32 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-36 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-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 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-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-8 success 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-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success 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-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success 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-30 success 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-38 success 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-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next-0

Commit Message

Martin Kelly Oct. 10, 2024, 6:45 p.m. UTC 
The documentation says CONFIG_FUNCTION_ERROR_INJECTION is supported only
on x86. This was presumably true at the time of writing, but it's now
supported on many other architectures too, so drop the part of the
statement mentioning x86.

Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
---
 include/uapi/linux/bpf.h       | 3 +--
 tools/include/uapi/linux/bpf.h | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

Comments

Alexei Starovoitov Oct. 10, 2024, 6:54 p.m. UTC | #1 
On Thu, Oct 10, 2024 at 11:47 AM Martin Kelly
<martin.kelly@crowdstrike.com> wrote:
>
> The documentation says CONFIG_FUNCTION_ERROR_INJECTION is supported only
> on x86. This was presumably true at the time of writing, but it's now
> supported on many other architectures too, so drop the part of the
> statement mentioning x86.
>
> Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
> ---
>  include/uapi/linux/bpf.h       | 3 +--
>  tools/include/uapi/linux/bpf.h | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 8ab4d8184b9d..a2ddfc8c8ed9 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3105,8 +3105,7 @@ union bpf_attr {
>   *             **ALLOW_ERROR_INJECTION** in the kernel code.
>   *
>   *             Also, the helper is only available for the architectures having
> - *             the CONFIG_FUNCTION_ERROR_INJECTION option. As of this writing,
> - *             x86 architecture is the only one to support this feature.
> + *             the CONFIG_FUNCTION_ERROR_INJECTION option.

Something like this is good to add to
Documentation/fault-injection/fault-injection.rst
and may be a link to it somewhere in Documentation/bpf/.

But uapi/bpf.h is not such place.

pw-bot: cr
Martin Kelly Oct. 10, 2024, 6:57 p.m. UTC | #2 
On Thu, 2024-10-10 at 11:54 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 11:47 AM Martin Kelly
> <martin.kelly@crowdstrike.com> wrote:
> > 
> > The documentation says CONFIG_FUNCTION_ERROR_INJECTION is supported
> > only
> > on x86. This was presumably true at the time of writing, but it's
> > now
> > supported on many other architectures too, so drop the part of the
> > statement mentioning x86.
> > 
> > Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
> > ---
> >  include/uapi/linux/bpf.h       | 3 +--
> >  tools/include/uapi/linux/bpf.h | 3 +--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 8ab4d8184b9d..a2ddfc8c8ed9 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3105,8 +3105,7 @@ union bpf_attr {
> >   *             **ALLOW_ERROR_INJECTION** in the kernel code.
> >   *
> >   *             Also, the helper is only available for the
> > architectures having
> > - *             the CONFIG_FUNCTION_ERROR_INJECTION option. As of
> > this writing,
> > - *             x86 architecture is the only one to support this
> > feature.
> > + *             the CONFIG_FUNCTION_ERROR_INJECTION option.
> 
> Something like this is good to add to
> Documentation/fault-injection/fault-injection.rst
> and may be a link to it somewhere in Documentation/bpf/.
> 
> But uapi/bpf.h is not such place.
> 
> pw-bot: cr

Would you prefer to just remove the sentence altogether? Currently,
this statement is already in the headers, so I think it's best to
either correct it or remove it, but not leave it the way it is (which
is not very accurate).
Alexei Starovoitov Oct. 10, 2024, 7:06 p.m. UTC | #3 
On Thu, Oct 10, 2024 at 11:57 AM Martin Kelly
<martin.kelly@crowdstrike.com> wrote:
>
> On Thu, 2024-10-10 at 11:54 -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 10, 2024 at 11:47 AM Martin Kelly
> > <martin.kelly@crowdstrike.com> wrote:
> > >
> > > The documentation says CONFIG_FUNCTION_ERROR_INJECTION is supported
> > > only
> > > on x86. This was presumably true at the time of writing, but it's
> > > now
> > > supported on many other architectures too, so drop the part of the
> > > statement mentioning x86.
> > >
> > > Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
> > > ---
> > >  include/uapi/linux/bpf.h       | 3 +--
> > >  tools/include/uapi/linux/bpf.h | 3 +--
> > >  2 files changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 8ab4d8184b9d..a2ddfc8c8ed9 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -3105,8 +3105,7 @@ union bpf_attr {
> > >   *             **ALLOW_ERROR_INJECTION** in the kernel code.
> > >   *
> > >   *             Also, the helper is only available for the
> > > architectures having
> > > - *             the CONFIG_FUNCTION_ERROR_INJECTION option. As of
> > > this writing,
> > > - *             x86 architecture is the only one to support this
> > > feature.
> > > + *             the CONFIG_FUNCTION_ERROR_INJECTION option.
> >
> > Something like this is good to add to
> > Documentation/fault-injection/fault-injection.rst
> > and may be a link to it somewhere in Documentation/bpf/.
> >
> > But uapi/bpf.h is not such place.
> >
> > pw-bot: cr
>
> Would you prefer to just remove the sentence altogether? Currently,
> this statement is already in the headers, so I think it's best to
> either correct it or remove it, but not leave it the way it is (which
> is not very accurate).

I say let's remove the whole paragraph then.

.h already says
"It is only available if the kernel was compiled
 with the **CONFIG_BPF_KPROBE_OVERRIDE** configuration
 option,"

which in turn depends on:

config BPF_KPROBE_OVERRIDE
        bool "Enable BPF programs to override a kprobed function"
        depends on BPF_EVENTS
        depends on FUNCTION_ERROR_INJECTION

No need to duplicate kconfig dependencies as a doc in .h
Martin Kelly Oct. 10, 2024, 7:34 p.m. UTC | #4 
On Thu, 2024-10-10 at 12:06 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 11:57 AM Martin Kelly
> <martin.kelly@crowdstrike.com> wrote:
> > 
> > On Thu, 2024-10-10 at 11:54 -0700, Alexei Starovoitov wrote:
> > > On Thu, Oct 10, 2024 at 11:47 AM Martin Kelly
> > > <martin.kelly@crowdstrike.com> wrote:
> > > > 
> > > > The documentation says CONFIG_FUNCTION_ERROR_INJECTION is
> > > > supported
> > > > only
> > > > on x86. This was presumably true at the time of writing, but
> > > > it's
> > > > now
> > > > supported on many other architectures too, so drop the part of
> > > > the
> > > > statement mentioning x86.
> > > > 
> > > > Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 3 +--
> > > >  tools/include/uapi/linux/bpf.h | 3 +--
> > > >  2 files changed, 2 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/include/uapi/linux/bpf.h
> > > > b/include/uapi/linux/bpf.h
> > > > index 8ab4d8184b9d..a2ddfc8c8ed9 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -3105,8 +3105,7 @@ union bpf_attr {
> > > >   *             **ALLOW_ERROR_INJECTION** in the kernel code.
> > > >   *
> > > >   *             Also, the helper is only available for the
> > > > architectures having
> > > > - *             the CONFIG_FUNCTION_ERROR_INJECTION option. As
> > > > of
> > > > this writing,
> > > > - *             x86 architecture is the only one to support
> > > > this
> > > > feature.
> > > > + *             the CONFIG_FUNCTION_ERROR_INJECTION option.
> > > 
> > > Something like this is good to add to
> > > Documentation/fault-injection/fault-injection.rst
> > > and may be a link to it somewhere in Documentation/bpf/.
> > > 
> > > But uapi/bpf.h is not such place.
> > > 
> > > pw-bot: cr
> > 
> > Would you prefer to just remove the sentence altogether? Currently,
> > this statement is already in the headers, so I think it's best to
> > either correct it or remove it, but not leave it the way it is
> > (which
> > is not very accurate).
> 
> I say let's remove the whole paragraph then.
> 
> .h already says
> "It is only available if the kernel was compiled
>  with the **CONFIG_BPF_KPROBE_OVERRIDE** configuration
>  option,"
> 
> which in turn depends on:
> 
> config BPF_KPROBE_OVERRIDE
>         bool "Enable BPF programs to override a kprobed function"
>         depends on BPF_EVENTS
>         depends on FUNCTION_ERROR_INJECTION
> 
> No need to duplicate kconfig dependencies as a doc in .h

Agreed. I sent a v2 with the paragraph removed.
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8ab4d8184b9d..a2ddfc8c8ed9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3105,8 +3105,7 @@  union bpf_attr {
  * 		**ALLOW_ERROR_INJECTION** in the kernel code.
  *
  * 		Also, the helper is only available for the architectures having
- * 		the CONFIG_FUNCTION_ERROR_INJECTION option. As of this writing,
- * 		x86 architecture is the only one to support this feature.
+ * 		the CONFIG_FUNCTION_ERROR_INJECTION option.
  * 	Return
  * 		0
  *
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 7610883c8191..15c9364b8d7d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3105,8 +3105,7 @@  union bpf_attr {
  * 		**ALLOW_ERROR_INJECTION** in the kernel code.
  *
  * 		Also, the helper is only available for the architectures having
- * 		the CONFIG_FUNCTION_ERROR_INJECTION option. As of this writing,
- * 		x86 architecture is the only one to support this feature.
+ * 		the CONFIG_FUNCTION_ERROR_INJECTION option.
  * 	Return
  * 		0
  *