[PATCH bpf-next 0/3] Document kfunc lifecycle / stability expectations

[PATCH bpf-next 0/3] Document kfunc lifecycle / stability expectations

[David Vernet]

BPF kernel <-> kernel API stability has been discussed at length over
the last several weeks and months. Now that we've largely aligned over
kfuncs being the way forward, and BPF helpers being considered frozen,
it's time to document the expectations for kfunc lifecycles and
stability so that everyone (BPF users, kfunc developers, and
maintainers) are all aligned, and have a crystal-clear understanding of
the expectations surrounding kfuncs.

This patch set adds that documentation to the main kfuncs documentation
page via a new 'kfunc lifecycle expectations' section. The documentation
describes how decisions are made in the kernel regarding whether to
include, keep, deprecate, or change / remove a kfunc. As described very
overtly in the patch set itself, but likely worth highlighting here:

"kfunc stability" does not mean, nor ever will mean, "BPF APIs may block
development elsewhere in the kernel".

Rather, the intention and expectation is for kfuncs to be treated like
EXPORT_SYMBOL_GPL symbols in the kernel. The goal is for kfuncs to be a
safe and valuable option for maintainers and kfunc developers to extend
the kernel, without tying anyone's hands, or imposing any kind of
restrictions on maintainers in the same way that UAPI changes do.

Note that other proposals for this documentation have been made as well.
Toke has proposed several iterations to this doc, with the latest being
[0].

[0]: https://lore.kernel.org/all/20230201174449.94650-1-toke@redhat.com/

David Vernet (3):
  bpf/docs: Document kfunc lifecycle / stability expectations
  bpf: Add KF_DEPRECATED kfunc flag
  selftests/bpf: Add a selftest for the KF_DEPRECATED kfunc flag

 Documentation/bpf/kfuncs.rst                  | 125 +++++++++++++++++-
 include/linux/btf.h                           |   1 +
 kernel/bpf/verifier.c                         |   8 ++
 net/bpf/test_run.c                            |   5 +
 .../selftests/bpf/prog_tests/kfunc_call.c     |   2 +
 .../selftests/bpf/progs/kfunc_call_test.c     |  10 ++
 6 files changed, 146 insertions(+), 5 deletions(-)

-- 
2.39.0

[PATCH bpf-next 1/3] bpf/docs: Document kfunc lifecycle / stability expectations

[David Vernet]

BPF kernel <-> kernel API stability has been discussed at length over
the last several weeks and months. Now that we've largely aligned over
kfuncs being the way forward, and BPF helpers being considered frozen,
it's time to document the expectations for kfunc lifecycles and
stability so that everyone (BPF users, kfunc developers, and
maintainers) are all aligned, and have a crystal-clear understanding of
the expectations surrounding kfuncs.

To do that, this patch adds that documentation to the main kfuncs
documentation page via a new 'kfunc lifecycle expectations' section. The
patch describes how decisions are made in the kernel regarding whether
to include, keep, deprecate, or change / remove a kfunc. As described
very overtly in the patch itself, but likely worth highlighting here:

"kfunc stability" does not mean, nor ever will mean, "BPF APIs may block
development elsewhere in the kernel".

Rather, the intention and expectation is for kfuncs to be treated like
EXPORT_SYMBOL_GPL symbols in the kernel. The goal is for kfuncs to be a
safe and valuable option for maintainers and kfunc developers to extend
the kernel, without tying anyone's hands, or imposing any kind of
restrictions on maintainers in the same way that UAPI changes do.

In addition to the 'kfunc lifecycle expectations' section, this patch
also adds documentation for a new KF_DEPRECATED kfunc flag which kfunc
authors or maintainers can choose to add to kfuncs if and when they
decide to deprecate them. Note that as described in the patch itself, a
kfunc need not be deprecated before being changed or removed -- this
flag is simply provided as an available deprecation mechanism for those
that want to provide a deprecation story / timeline to their users.
When necessary, kfuncs may be changed or removed to accommodate changes
elsewhere in the kernel without any deprecation at all.

A subsequent patch will add KF_DEPRECATED to <linux/btf.h>.

Signed-off-by: David Vernet <void@manifault.com>
---
 Documentation/bpf/kfuncs.rst | 125 +++++++++++++++++++++++++++++++++--
 1 file changed, 120 insertions(+), 5 deletions(-)

diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index 0bd07b39c2a4..e8452868827e 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -13,7 +13,7 @@ BPF Kernel Functions or more commonly known as kfuncs are functions in the Linux
 kernel which are exposed for use by BPF programs. Unlike normal BPF helpers,
 kfuncs do not have a stable interface and can change from one kernel release to
 another. Hence, BPF programs need to be updated in response to changes in the
-kernel.
+kernel. See :ref:`BPF_kfunc_lifecycle_expectations` for more information.
 
 2. Defining a kfunc
 ===================
@@ -238,6 +238,32 @@ single argument which must be a trusted argument or a MEM_RCU pointer.
 The argument may have reference count of 0 and the kfunc must take this
 into consideration.
 
+.. _KF_deprecated_flag:
+
+2.4.9 KF_DEPRECATED flag
+------------------------
+
+The KF_DEPRECATED flag is used for kfuncs which are expected to be changed or
+removed in a subsequent kernel release. Deprecated kfuncs may be removed at any
+time, though if possible (and when applicable), developers are encouraged to
+provide users with a deprecation window to ease the burden of migrating off of
+the kfunc.
+
+A kfunc that is marked with KF_DEPRECATED should also have any relevant
+information captured in its kernel doc. Such information typically includes the
+kfunc's expected remaining lifespan, a recommendation for new functionality
+that can replace it if any is available, and possibly a rationale for why it is
+being removed.
+
+Note that while on some occasions, a KF_DEPRECATED kfunc may continue to be
+supported and have its KF_DEPRECATED flag removed, it is likely to be far more
+difficult to remove a KF_DEPRECATED flag after it's been added than it is to
+prevent it from being added in the first place. As described in
+:ref:`BPF_kfunc_lifecycle_expectations`, users that rely on specific kfuncs are
+highly encouraged to make their use-cases known as early as possible, and
+participate in upstream discussions regarding whether to keep, change,
+deprecate, or remove those kfuncs if and when such discussions occur.
+
 2.5 Registering the kfuncs
 --------------------------
 
@@ -304,14 +330,103 @@ In order to accommodate such requirements, the verifier will enforce strict
 PTR_TO_BTF_ID type matching if two types have the exact same name, with one
 being suffixed with ``___init``.
 
-3. Core kfuncs
+.. _BPF_kfunc_lifecycle_expectations:
+
+3. kfunc lifecycle expectations
+===============================
+
+Just as with kernel symbols exported with EXPORT_SYMBOL_GPL, kfuncs could be
+changed at any time by a maintainer of the subsystem they're defined in when
+deemed necessary. However, like any other change to the kernel, maintainers
+will not change or remove a kfunc without having some justification. Whether or
+not they'll choose to change a kfunc will ultimately depend on a variety of
+factors, such as how widely used the kfunc is, how long the kfunc has been in
+the kernel, whether an alternative kfunc exists, what the norm is in terms of
+stability for the subsystem in question, and of course what the technical cost
+is of continuing to support the kfunc.
+
+There are several implications of this:
+
+a) A kfunc will never have any hard stability guarantees. BPF APIs cannot and
+   will not ever hard-block a change in the kernel purely for stability
+   reasons. In other words, kfuncs have the exact same stability guarantees as
+   any other kernel API, such as those provided by EXPORT_SYMBOL_GPL, though
+   with perhaps less burden than EXPORT_SYMBOL_GPL changes thanks to BPF CO-RE.
+
+   That being said, kfuncs are features that are meant to solve problems and
+   provide value to users. The decision of whether to change or remove a kfunc
+   is a multivariate technical decision that is made on a case-by-case basis,
+   and which is informed by data points such as those mentioned above. It is
+   expected that a kfunc being removed or changed with no warning will not be a
+   common occurrence or take place without sound justification, but it is a
+   possibility that must be accepted if one is to use kfuncs.
+
+b) kfuncs that are widely used or have been in the kernel for a long time will
+   be more difficult to justify being changed or removed by a maintainer. Said
+   in a different way, kfuncs that are known to have a lot of users and provide
+   significant value provide stronger incentives for maintainers to invest the
+   time and complexity in supporting them. It is therefore important for
+   developers using kfuncs in their BPF programs to demonstrate how and why
+   those kfuncs are being used, and to participate in discussions regarding
+   those kfuncs when they occur upstream.
+
+c) Because many BPF programs are not upstreamed as part of the kernel tree, it
+   is often not possible to change them in-place when a kfunc changes, as it is
+   for e.g. an upstreamed module being updated in place when an
+   EXPORT_SYMBOL_GPL symbol is changed. Distributions that bundle BPF programs
+   that use kfuncs must therefore ensure that those BPF programs are linking
+   against the kfuncs that are supported by the kernel version being used for
+   any given release. Additionally, BPF developers are encouraged to upstream
+   their BPF programs so they can enjoy the same benefits as upstreamed
+   modules, and avoid code churn.
+
+3.1 kfunc deprecation
+---------------------
+
+As described above, while sometimes a maintainer may find that a kfunc must be
+changed or removed immediately to accommodate some changes in their subsystem,
+other kfuncs may be able to accommodate a longer and more measured deprecation
+process. For example, if a new kfunc comes along which provides superior
+functionality to an existing kfunc, the existing kfunc may be deprecated for
+some period of time to allow users to migrate their BPF programs to use the new
+one. Or, if a kfunc has no known users, a decision may be made to remove the
+kfunc (without providing an alternative API) after some deprecation period
+period so as to provide users with a window to notify the kfunc maintainer if
+it turns out that the kfunc is actually being used.
+
+kfuncs being deprecated (rather than changed or removed with no warning) is
+expected to be the common case, and as described in :ref:`KF_deprecated_flag`,
+the kfunc framework provides the KF_DEPRECATED flag to kfunc developers to
+signal to users that a kfunc has been deprecated. Once a kfunc has been marked
+with KF_DEPRECATED, the following procedure is followed for removal:
+
+1. Any relevant information for deprecated kfuncs is documented in the kfunc's
+   kernel docs. This documentation will typically include the kfunc's expected
+   remaining lifespan,  a recommendation for new functionality that can replace
+   the usage of the deprecated function (or an explanation as to why no such
+   replacement exists), etc.
+
+2. The deprecated kfunc is kept in the kernel for some period of time after it
+   was first marked as deprecated. This time period will be chosen on a
+   case-by-case basis, and will typically depend on how widespread the use of
+   the kfunc is, how long it has been in the kernel, and how hard it is to move
+   to alternatives. This deprecation time period is "best effort", and as
+   described :ref:`above<BPF_kfunc_lifecycle_expectations>`, circumstances may
+   sometimes dictate that the kfunc be removed before the full intended
+   deprecation period has elapsed.
+
+3. After the deprecation period, or sometimes earlier if necessary, the kfunc
+   will be removed. At this point, BPF programs calling the kfunc will be
+   rejected by the verifier.
+
+4. Core kfuncs
 ==============
 
 The BPF subsystem provides a number of "core" kfuncs that are potentially
 applicable to a wide variety of different possible use cases and programs.
 Those kfuncs are documented here.
 
-3.1 struct task_struct * kfuncs
+4.1 struct task_struct * kfuncs
 -------------------------------
 
 There are a number of kfuncs that allow ``struct task_struct *`` objects to be
@@ -387,7 +502,7 @@ Here is an example of it being used:
 		return 0;
 	}
 
-3.2 struct cgroup * kfuncs
+4.2 struct cgroup * kfuncs
 --------------------------
 
 ``struct cgroup *`` objects also have acquire and release functions:
@@ -502,7 +617,7 @@ the verifier. bpf_cgroup_ancestor() can be used as follows:
 		return 0;
 	}
 
-3.3 struct cpumask * kfuncs
+4.3 struct cpumask * kfuncs
 ---------------------------
 
 BPF provides a set of kfuncs that can be used to query, allocate, mutate, and
-- 
2.39.0

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

[David Vernet]

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@manifault.com>
---
 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);
+
 	return 0;
 }
 
-- 
2.39.0

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

[Alexei Starovoitov]

On Thu, Feb 2, 2023 at 8:31 AM David Vernet <void@manifault.com> 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@manifault.com>
> ---
>  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.

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

[David Vernet]

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@manifault.com> 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@manifault.com>
> > ---
> >  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.

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).

Let me know how you'd like to proceed. Either just removing patches 2
and 3 and still mentioning KF_DEPRECATED, removing 2 and 3 and removing
any mention of KF_DEPRECATED, or leaving 2 and 3.

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

[Daniel Borkmann]

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@manifault.com> 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@manifault.com>
>>> ---
>>>   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.

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

[Alexei Starovoitov]

On Thu, Feb 2, 2023 at 3:11 PM Daniel Borkmann <daniel@iogearbox.net> 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@manifault.com> 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@manifault.com>
> >>> ---
> >>>   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.

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

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Thu, Feb 2, 2023 at 3:11 PM Daniel Borkmann <daniel@iogearbox.net> 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@manifault.com> 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@manifault.com>
>> >>> ---
>> >>>   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

[PATCH bpf-next 3/3] selftests/bpf: Add a selftest for the KF_DEPRECATED kfunc flag

[David Vernet]

Now that we have KF_DEPRECATED, we should add a selftest for it. This
patch implements that selftest by adding a new test
bpf_kfunc_call_test_deprecated() that includes the KF_DEPRECATED flag,
and adding a kfunc_call_test_deprecated testcase to the kfunc_call_test
suite which verifies that the program is loaded and has the expected
verifier error message.

Signed-off-by: David Vernet <void@manifault.com>
---
 net/bpf/test_run.c                                  |  5 +++++
 tools/testing/selftests/bpf/prog_tests/kfunc_call.c |  2 ++
 tools/testing/selftests/bpf/progs/kfunc_call_test.c | 10 ++++++++++
 3 files changed, 17 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index e6f773d12045..945d6f7c1825 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -746,6 +746,10 @@ __bpf_kfunc static u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused
 	return arg;
 }
 
+__bpf_kfunc void bpf_kfunc_call_test_deprecated(void)
+{
+}
+
 __diag_pop();
 
 BTF_SET8_START(bpf_test_modify_return_ids)
@@ -785,6 +789,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_deprecated, KF_DEPRECATED)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index a543742cd7bd..98398dba5718 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -319,4 +319,6 @@ void test_kfunc_call(void)
 
 	if (test__start_subtest("destructive"))
 		test_destructive();
+
+	RUN_TESTS(kfunc_call_test);
 }
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
index 7daa8f5720b9..b605438e42dd 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2021 Facebook */
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
 
 extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
 extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
@@ -18,6 +19,7 @@ extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
 extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
 extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
 extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
+extern void bpf_kfunc_call_test_deprecated(void) __ksym;
 
 SEC("tc")
 int kfunc_call_test4(struct __sk_buff *skb)
@@ -192,4 +194,12 @@ int kfunc_call_test_static_unused_arg(struct __sk_buff *skb)
 	return actual != expected ? -1 : 0;
 }
 
+SEC("tc")
+__success __log_level(2) __msg("calling deprecated kfunc bpf_kfunc_call_test_deprecated")
+int kfunc_call_test_deprecated(struct __sk_buff *skb)
+{
+	bpf_kfunc_call_test_deprecated();
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.39.0