Re: [PATCH] Documentation/bpf: make it clear that kfuncs should be non-static

[Roman Gushchin]

JP Kobryn <jp.kobryn@xxxxxxxxx> writes:

> The kfunc documentation mentions how the macro __bpf_kfunc prevents
> inlining for static functions. This makes it sound like static kfuncs are
> acceptable. Although static kfuncs may happen to work, it is by chance that
> the compiler chose not to rename these functions and BTF resolution still
> succeeds.
>
> Make it clear in the documentation why kfuncs should not be declared
> static. First, remove wording that makes it sound like static is ok. Then
> point out the external naming needed for BTF resolution. Finally point out
> that sparse may warn on unreferenced kfuncs and that this warning can be
> ignored.
>
> Signed-off-by: JP Kobryn <jp.kobryn@xxxxxxxxx>
> ---
>  Documentation/bpf/kfuncs.rst | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index 4c814ff6061e..1dbaff8d4805 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -276,19 +276,26 @@ This set encodes the BTF ID of each kfunc listed above, and encodes the flags
>  along with it. Ofcourse, it is also allowed to specify no flags.
>  
>  kfunc definitions should also always be annotated with the ``__bpf_kfunc``
> -macro. This prevents issues such as the compiler inlining the kfunc if it's a
> -static kernel function, or the function being elided in an LTO build as it's
> -not used in the rest of the kernel. Developers should not manually add
> -annotations to their kfunc to prevent these issues. If an annotation is
> -required to prevent such an issue with your kfunc, it is a bug and should be
> -added to the definition of the macro so that other kfuncs are similarly
> -protected. An example is given below::
> +macro. This prevents issues such as the compiler inlining the kfunc, or the
> +function being elided in an LTO build as it's not used in the rest of the
> +kernel. Developers should not manually add annotations to their kfunc to prevent
> +these issues. If an annotation is required to prevent such an issue with your
> +kfunc, it is a bug and should be added to the definition of the macro so that
> +other kfuncs are similarly protected. An example is given below::
>  
>          __bpf_kfunc struct task_struct *bpf_get_task_pid(s32 pid)
>          {
>          ...
>          }
>  
> +Note that kfuncs must not be declared ``static``. A kfunc can be called from a
> +BPF program ``*.c`` file outside the compilation unit that defines it, so its
> +externally visible name must remain available for BTF ID lookup. ``static``
> +linkage allows the compiler to rename the function, which can break this
> +BTF-based kfunc resolution. Further note that sparse may warn that an otherwise
> +unreferenced kfunc should be static. Such warnings should be ignored for kfunc
> +definitions.
> +
>  2.5.1 KF_ACQUIRE flag
>  ---------------------

Acked-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>

Thanks