Message-ID: <CAEf4BzZvvFyRCJcWibv_n+aeusGvFQuvyfOux0-3RUT3R35Qwg@mail.gmail.com>
Date: Tue, 19 Dec 2023 10:31:17 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Christian Brauner <brauner@...nel.org>, 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 8:43 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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.

Yes, and I appreciate your reviews and feedback a lot. There was never
an intent to clandestinely land anything bad or outlandish, so I'm
sorry you feel this way. I've cc'ed you and fsdevel mailing list, just
like LSM folks, on every relevant patch set, each and every patch in
them, and incorporated all the feedback I got over the last multiple
months.

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

It's on me to have interpreted FD=0 as AT_CWD in my original patch set
for BPF_OBJ_PIN/BPF_OBJ_GET ([0]). It was totally my fault not
thinking through all the negative consequences of defaulting to
AT_CWD, and I acknowledged that and fixed it.

It's also my bad that I kept using the "fd=0 means no FD was
specified" approach which has been a consistent approach within bpf()
syscall API without really thinking twice about this and how much it
might irritate kernel people. I sent a fix ([1]) and going forward
I'll remember to always add a flag for any new FD-based field in BPF
UAPI.

  [0] https://lore.kernel.org/all/20230516001348.286414-1-andrii@kernel.org/
  [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231219053150.336991-1-andrii@kernel.org/

> >
> > 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 did ask for a review and an ack as a sign that it looks good to you.
Precisely to make sure that *everything* looks good overall from the
POV of people outside of the BPF subsystem. I didn't ask for rubber
stamping anything, if that's what is implied here.

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

Right, and AT_FDCWD interpretation for fd=0 had security implications
which was a clear and a bad bug.

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

Yes, I second the above.

Powered by blists - more mailing lists