Re: [PATCH bpf-next 2/3] bpf: Add KF_DEPRECATED kfunc flag

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:

> On Thu, Feb 2, 2023 at 3:11 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>>
>> On 2/2/23 10:27 PM, David Vernet wrote:
>> > On Thu, Feb 02, 2023 at 01:21:19PM -0800, Alexei Starovoitov wrote:
>> >> On Thu, Feb 2, 2023 at 8:31 AM David Vernet <void@xxxxxxxxxxxxx> wrote:
>> >>>
>> >>> Now that we have our kfunc lifecycle expectations clearly documented,
>> >>> and that KF_DEPRECATED is documented as an optional method for kfunc
>> >>> developers and maintainers to provide a deprecation story to BPF users,
>> >>> we need to actually implement the flag.
>> >>>
>> >>> This patch adds KF_DEPRECATED, and updates the verifier to issue a
>> >>> verifier log message if a deprecated kfunc is called. Currently, a BPF
>> >>> program either has to fail to verify, or be loaded with log level 2 in
>> >>> order to see the message. We could eventually enhance this to always
>> >>> be logged regardless of log level or verification status, or we could
>> >>> instead emit a warning to dmesg. This seems like the least controversial
>> >>> option for now.
>> >>>
>> >>> A subsequent patch will add a selftest that verifies this behavior.
>> >>>
>> >>> Signed-off-by: David Vernet <void@xxxxxxxxxxxxx>
>> >>> ---
>> >>>   include/linux/btf.h   | 1 +
>> >>>   kernel/bpf/verifier.c | 8 ++++++++
>> >>>   2 files changed, 9 insertions(+)
>> >>>
>> >>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> >>> index 49e0fe6d8274..a0ea788ee9b0 100644
>> >>> --- a/include/linux/btf.h
>> >>> +++ b/include/linux/btf.h
>> >>> @@ -71,6 +71,7 @@
>> >>>   #define KF_SLEEPABLE    (1 << 5) /* kfunc may sleep */
>> >>>   #define KF_DESTRUCTIVE  (1 << 6) /* kfunc performs destructive actions */
>> >>>   #define KF_RCU          (1 << 7) /* kfunc only takes rcu pointer arguments */
>> >>> +#define KF_DEPRECATED   (1 << 8) /* kfunc is slated to be removed or deprecated */
>> >>>
>> >>>   /*
>> >>>    * Tag marking a kernel function as a kfunc. This is meant to minimize the
>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> >>> index 4cc0e70ee71e..22adcf24f9e1 100644
>> >>> --- a/kernel/bpf/verifier.c
>> >>> +++ b/kernel/bpf/verifier.c
>> >>> @@ -8511,6 +8511,11 @@ static bool is_kfunc_rcu(struct bpf_kfunc_call_arg_meta *meta)
>> >>>          return meta->kfunc_flags & KF_RCU;
>> >>>   }
>> >>>
>> >>> +static bool is_kfunc_deprecated(const struct bpf_kfunc_call_arg_meta *meta)
>> >>> +{
>> >>> +       return meta->kfunc_flags & KF_DEPRECATED;
>> >>> +}
>> >>> +
>> >>>   static bool is_kfunc_arg_kptr_get(struct bpf_kfunc_call_arg_meta *meta, int arg)
>> >>>   {
>> >>>          return arg == 0 && (meta->kfunc_flags & KF_KPTR_GET);
>> >>> @@ -9646,6 +9651,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> >>>                          mark_btf_func_reg_size(env, regno, t->size);
>> >>>          }
>> >>>
>> >>> +       if (is_kfunc_deprecated(&meta))
>> >>> +               verbose(env, "calling deprecated kfunc %s\n", func_name);
>> >>> +
>> >>
>> >> Since prog will successfully load, no one will notice this message.
>> >>
>> >> I think we can skip patches 2 and 3 for now.
>>
>> +1, the KF_DEPRECATED could probably for the time being just mentioned
>> in doc.
>>
>> > I can leave them out of the v2 version of the patch set, but the reason
>> > I included them here is because I thought it would be odd to document
>> > KF_DEPRECATED without actually upstreaming it. Agreed that it is
>> > essentially 0 signal in its current form. Hopefully it could be expanded
>> > soon to be louder and more noticeable by not relying on the env log,
>> > which is wiped if the verifier passes, but that's separate from whether
>> > KF_DEPRECATED in general is the API that we want to provide kfunc
>> > developers (in which case at least 2 and 3 would add that in a
>> > non-controversial form).
>>
>> This ideally needs some form of prog load flag which would error upon
>> use of kfuncs with deprecation tag, such that tools probing kernel for
>> feature availability can notice.
>
> Interesting idea.
> By default we can reject loading progs that try to use KF_DEPRECATED,
> but still allow it with explicit load flag.

If we reject by default then adding the deprecation flag would break
applications just as much as just removing the kfunc, which would kinda
defeat the purpose of having the flag in the first place, wouldn't it? :)

-Toke