Message-ID: <CAEf4BzZDFgDOV9dxfC7g+b9QVQtA4j-_dnqnKOYWPwHbzQ5nJw@mail.gmail.com>
Date: Mon, 18 Dec 2023 21:38:43 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Linus Torvalds <torvalds@...uxfoundation.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, 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>, 
	Andrii Nakryiko <andrii@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Christian Brauner <brauner@...nel.org>, Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Kernel Team <kernel-team@...com>
Subject: Re: pull-request: bpf-next 2023-12-18

On Mon, Dec 18, 2023 at 8:34 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Mon, Dec 18, 2023 at 7:58 PM Linus Torvalds
> <torvalds@...uxfoundation.org> wrote:
> >
> > On Mon, 18 Dec 2023 at 17:48, Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > >
> > > Point taken.
> > > We can do s/__u32 token_fd/__u64 token/
> > > and waste upper 32-bit as flags that indicate that lower 32-bit is an FD
> > > or
> > > are you ok with __u32 token that is 'fd + 1'.
> >
> > No, you make it follow the standard pattern that Unix has always had:
> > file descriptors are _signed_ integer, and negative means error (or
> > special cases).
> >
> > Now, traditionally a 'fd' is literally just of type "int", but for
> > structures it's actually good to make it be a sized entity, so just
> > make it be __s32, and make any special cases be actual negative
> > numbers.
> >
> > Because I'll just go out on a limb and say that two billion file
> > descriptors is enough for anybody, and if we ever were to hit that
> > number, we'll have *way* more serious problems elsewhere long long
> > before. And in practice, "int" is 32-bit on all current and
> > near-future architectures, so "__s32" really is the same as "int" in
> > all practical respects, and making the size explicit is just a good
> > idea.
> >
> > You might want to perhaps pre-reserve a few negative numbers for
> > actual special cases, eg "openat()" uses
> >
> >     #define AT_FDCWD -100
> >
> > which I don't think is a great example to follow in the details: it
> > should have parenthesis, and "100" is a rather odd number to choose,
> > but it's certainly an example of a not-fundamentally-broken "not a
> > file descriptor, but a special case".
> >
> > Now, if you have a 'flags' or 'cmd' field for *other* reasons, then
> > you can certainly just use one of the flags for "I have a file
> > descriptor". But don't do some odd "translate values", and don't add
> > 32 bits just for that.
> >
>
> Makes sense. Yes, we do have flags for all commands accepting token
> FD, except for one, BPF_BTF_LOAD, but it's trivial to add flags there
> as well. I'll prepare a patch.

The patch is at [0], thanks.

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

>
> > That's also a perfectly fine traditional unix use (example: socket
> > control messages - "struct cmsghdr" with "cmsg_type = SCM_RIGHTS" in
> > unix domain sockets).
> >
> > But if you don't have some other reason for having a separate flag for
> > "I also have a file descriptor you should use", then just make a
> > negative number mean "no file descriptor".
> >
> > It's easy to test for the number being negative, but it's also just
> > easy to *not* test for, ie it's also perfectly fine to just do
> > something like
> >
> >         struct fd f = fdget(fd);
> >
> > without ever even bothering to test whether 'fd' is negative or not.
> > It is guaranteed to fail for negative numbers and just look exactly
> > like the "not open" case, so if you don't care about the difference
> > between "invalid" and "not open", then a negative fd also works just
> > as-is with no extra code at all.
> >
> >                    Linus
> >
> >                      Linus

Powered by blists - more mailing lists