Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <c35fbb4cb0a3a9b4653f9a032698469d94ca6e9c.1688123230.git.legion@kernel.org>
 <babdf7a8-9663-6d71-821a-34da2aff80e2@huaweicloud.com> <20230704-anrollen-beenden-9187c7b1b570@brauner>
In-Reply-To: <20230704-anrollen-beenden-9187c7b1b570@brauner>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Wed, 5 Jul 2023 18:10:32 -0700
Message-ID: <CAADnVQLAhDepRpbbi_EU6Ca3wnuBtSuAPO9mE6pGoxj8i9=caQ@mail.gmail.com>
Subject: Re: [PATCH v1] fs: Add kfuncs to handle idmapped mounts
To:     Christian Brauner <brauner@kernel.org>
Cc:     Hou Tao <houtao@huaweicloud.com>,
        Alexey Gladkov <legion@kernel.org>, bpf <bpf@vger.kernel.org>,
        Linux-Fsdevel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 4, 2023 at 6:01=E2=80=AFAM Christian Brauner <brauner@kernel.or=
g> wrote:
>
> > > +/**
> > > + * bpf_is_idmapped_mnt - check whether a mount is idmapped
> > > + * @mnt: the mount to check
> > > + *
> > > + * Return: true if mount is mapped, false if not.
> > > + */
> > > +__bpf_kfunc bool bpf_is_idmapped_mnt(struct vfsmount *mnt)
> > > +{
> > > +   return is_idmapped_mnt(mnt);
> > > +}
...
>
> I don't want any of these helpers as kfuncs as they are peeking deeply
> into implementation details that we reserve to change. Specifically in
> the light of:
>
>     3. kfunc lifecycle expectations part b):
>
>     "Unlike with regular kernel symbols, this is expected behavior for BP=
F
>      symbols, and out-of-tree BPF programs that use kfuncs should be cons=
idered
>      relevant to discussions and decisions around modifying and removing =
those
>      kfuncs. The BPF community will take an active role in participating =
in
>      upstream discussions when necessary to ensure that the perspectives =
of such
>      users are taken into account."
>
> That's too much stability for my taste for these helpers. The helpers
> here exposed have been modified multiple times and once we wean off
> idmapped mounts from user namespaces completely they will change again.
> So I'm fine if they're traceable but not as kfuncs with any - even
> minimal - stability guarantees.

Christian,
That quote is taken out of context.
In the first place the Documentation/bpf/kfuncs.rst says:
"
kfuncs provide a kernel <-> kernel API, and thus are not bound by any of th=
e
strict stability restrictions associated with kernel <-> user UAPIs. This m=
eans
they can be thought of as similar to EXPORT_SYMBOL_GPL, and can therefore b=
e
modified or removed by a maintainer of the subsystem they're defined in whe=
n
it's deemed necessary.
"

bpf_get_file_vfs_ids is vfs related, so you guys decide when and how
to add/remove them. It's ok that you don't want this particular one
for whatever reason, but that reason shouldn't be 'stability guarantees'.
There are really none. The kernel kfuncs can change at any time.
There are plenty of examples in git log where we added and then
tweaked/removed kfuncs.

The doc also says:
"
As described above, while sometimes a maintainer may find that a kfunc must=
 be
changed or removed immediately to accommodate some changes in their subsyst=
em,
"
and git log of such cases proves the point.

The quote about out-of-tree bpf progs is necessary today, since
very few bpf progs are in-tree, so when maintainers of a subsystem
want to remove kfunc the program authors need something in the doc
to point to and explain why and how they use the kfunc otherwise
maintainers will just say 'go away. you're out-of-tree'.
The users need their voice to be heard. Even if the result is the same.
In other words the part you quoted is needed to make kfuncs usable.
Otherwise 'kfunc is 100% unstable and maintainers can rename it
every release just to make life of bpf prog writers harder'
becomes a real possibility in the minds of bpf users.
The kfunc doc makes it 100% clear that there are no stability guarantees.
So please don't say 'minimal stability'.

In your other reply:

> we can look at the in-kernel users of is_idmapped_mnt(),
> convert them and then kill this thing off if we wanted to.

you can absolutely do that even if is_idmapped_mnt() is exposed as a kfunc.
You'll just delete it with zero notice if you like.
Just like what you would do with a normal export_symbol.
The doc is pretty clear about it and there are examples where we did
such things.