netdev - Re: pull-request: bpf-next 2023-12-18

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzYtD+bp8f+NvGMfVK=nL=E+EGsvz1MGySa1h4P8vA=bPw@mail.gmail.com>
Date: Mon, 18 Dec 2023 20:34:20 -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 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.

> 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