Date:   Thu, 2 Feb 2023 15:27:48 -0600
From:   David Vernet <void@manifault.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@meta.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Kernel Team <kernel-team@meta.com>,
        Toke =?iso-8859-1?Q?H=F8iland-J=F8rgensen?= <toke@redhat.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/3] bpf: Add KF_DEPRECATED kfunc flag
Message-ID: <Y9wq1Fy8sgpGB+pe@maniforge>
References: <20230202163056.658641-1-void@manifault.com>
 <20230202163056.658641-3-void@manifault.com>
 <CAADnVQJjmnEpXWL8-SAPt5zYXnFYeF8-wXXpA9shOhqUXNPw=g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAADnVQJjmnEpXWL8-SAPt5zYXnFYeF8-wXXpA9shOhqUXNPw=g@mail.gmail.com>
User-Agent: Mutt/2.2.9 (2022-11-12)
Precedence: bulk

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.