MIME-Version: 1.0
References: <20230202163056.658641-1-void@manifault.com> <20230202163056.658641-3-void@manifault.com>
 <CAADnVQJjmnEpXWL8-SAPt5zYXnFYeF8-wXXpA9shOhqUXNPw=g@mail.gmail.com>
 <Y9wq1Fy8sgpGB+pe@maniforge> <1ea9adb3-851c-0c04-1655-07d9f3b7f3b0@iogearbox.net>
In-Reply-To: <1ea9adb3-851c-0c04-1655-07d9f3b7f3b0@iogearbox.net>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Thu, 2 Feb 2023 15:21:40 -0800
Message-ID: <CAADnVQJeaU=F9uym9RctfODHbeV-TTK8DiQFTFm_R=N+qF6gYA@mail.gmail.com>
Subject: Re: [PATCH bpf-next 2/3] bpf: Add KF_DEPRECATED kfunc flag
To:     Daniel Borkmann <daniel@iogearbox.net>
Cc:     David Vernet <void@manifault.com>, bpf <bpf@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        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>,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.