Date: Tue, 19 Dec 2023 08:42:48 -0800
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Linus Torvalds <torvalds@...uxfoundation.org>, Andrii Nakryiko <andrii@...nel.org>, 
	Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	Daniel Borkmann <daniel@...earbox.net>, Peter Zijlstra <peterz@...radead.org>, 
	Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Kernel Team <kernel-team@...com>, Linux-Fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: pull-request: bpf-next 2023-12-18

On Tue, Dec 19, 2023 at 2:23 AM Christian Brauner <brauner@...nel.org> wrote:
>
> On Mon, Dec 18, 2023 at 05:11:23PM -0800, Linus Torvalds wrote:
> > On Mon, 18 Dec 2023 at 16:05, Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > 2) Introduce BPF token object, from Andrii Nakryiko.
> >
> > I assume this is why I and some other unusual recipients are cc'd,
> > because the networking people feel like they can't judge this and
> > shouldn't merge non-networking code like this.
> >
> > Honestly, I was told - and expected - that this part would come in a
> > branch of its own, so that it would be sanely reviewable.
> >
> > Now it's mixed in with everything else.
> >
> > This is *literally* why we have branches in git, so that people can
> > make more independent changes and judgements, and so that we don't
> > have to be in a situation where "look, here's ten different things,
> > pull it all or nothing".
> >
> > Many of the changes *look* like they are in branches, but they've been
> > the "fake branches" that are just done as "patch series in a branch,
> > with the cover letter as the merge message".
> >
> > Which is great for maintaining that cover letter information and a
> > certain amount of historical clarity, but not helpful AT ALL for the
> > "independent changes" thing when it is all mixed up in history, where
> > independent things are mostly serialized and not actually independent
> > in history at all.
> >
> > So now it appears to be one big mess, and exactly that "all or
> > nothing" thing that isn't great, since the whole point was that the
> > networking people weren't comfortable with the reviewing filesystem
> > side.
> >
> > And honestly, the bpf side *still* seems to be absolutely conbfused
> > and complkete crap when it comes to file descriptors.
> >
> > I took a quick look, and I *still* see new code being introduced there
> > that thinks that file descriptor zero is special, and we tols you a
> > *year* ago that that wasn't true, and that you need to fix this.
> >
> > I literally see complete garbage like tghis:
> >
> >         ..
> >         __u32 btf_token_fd;
> >         ...
> >         if (attr->btf_token_fd) {
> >                 token = bpf_token_get_from_fd(attr->btf_token_fd);
> >
> > and this is all *new* code that makes that same bogus sh*t-for-brains
> > mistake that was wrong the first time.
> >
> > So now I'm saying NAK. Enough is enough.  No more of this crazy "I
> > don't understand even the _basics_ of file descriptors, and yet I'm
> > introducing new random interfaces".
> >
> > I know you thought fd zero was something invalid. You were told
> > otherwise. Apparently you just ignored being wrong, and have decided
> > to double down on being wrong.
> >
> > We don't take this kind of flat-Earther crap.
> >
> > File descriptors don't start at 1. Deal with reality. Stop making the
> > same mistake over and over. If you ant to have a "no file descriptor"
> > flag, you use a signed type, and a signed value for that, because file
> > descriptor zero is perfectly valid, and I don't want to hear any more
> > uninformed denialism.
> >
> > Stop polluting the kernel with incorrect assumptions.
> >
> > So yes, I will keep NAK'ing this until this kind of fundamental
> > mistake is fixed. This is not rocket science, and this is not
> > something that wasn't discussed before. Your ignorance has now turned
> > from "I didn't know" to "I didn 't care", and at that point I really
> > don't want to see new code any more.
>
> Alexei, Andrii, this is a massive breach of trust and flatout
> disrespectful. I barely reword mails and believe me I've reworded this
> mail many times. I'm furious.
>
> Over the last couple of months since LSFMM in May 2023 until almost last
> week I've given you extensive design and review for this whole approach
> to get this into even remotely sane shape from a VFS perspective.
>
> The VFS maintainers including Linus have explicitly NAKed this "zero is
> not a valid fd nonsense" and told you to stop doing that. We told you
> that such fundamental VFS semantics are not yours to decide.
>
> And yet you put a patch into a series that did exactly that and then had
> the unbelievable audacity to repeatedly ask me to put my ACK under this
> - both in person and on list.
>
> I'm glad I only gave my ACK to the two patches that I extensivly
> reviewed and never to the whole series.

fwiw to three patches:
https://lore.kernel.org/bpf/20231208-besessen-vibrieren-4e963e3ca3ba@brauner/
which are all the main bits of it.

The patch 4 that does:
if (attr->map_token_fd)
wasn't sneaked in in any way.
You were cc-ed on it just like linux-fsdevel@...r
during all 12 revisions of the token series over many months.

So this accusation of breach of trust is baseless.

Indeed we didn't internalize that you guys hate fd=0 so much.
In the past you made it clear fd=0 shouldn't be an alias to AT_FDCWD.
We got that part. Meaning of fd=0 here wasn't a special new thing.
We made this mistake in the past and assumed it's ok-ish to continue
in similar situations.
As I said. Point taken. We'll use flag+fd approach as Linus suggested.

Powered by blists - more mailing lists