netdev - Re: [PATCH][CFT][experimental] net/socket.c: use straight fdget/fdput (resend)

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: <CAHk-=wixYUyQcS9tDNVvnCvEi37puqqpQ=CN+zP=a9Q9Fp5e-Q@mail.gmail.com>
Date: Sun, 26 May 2024 15:16:37 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: netdev@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH][CFT][experimental] net/socket.c: use straight fdget/fdput (resend)

On Sun, 26 May 2024 at 12:27, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Not really.  The real reason is different - there is a constraint on
> possible values of struct fd.  No valid instance can ever have NULL
> file and non-zero flags.
>
> The usual pattern is this:

[ snip snip ]

Ugh. I still hate it, including your new version. I suspect it will
easily generate the extra test at fd_empty() time, and your new
version would instead just move that extra test at fdput() time
instead.

Hopefully in most cases the compiler sees the previous test for
fd.file, realizes the new test is unnecessary and optimizes it away.

Except we most definitely pass around 'struct fd *' in some places (at
least ovlfs), so I doubt that  will be the case everywhere.

What would make more sense is if you make the "fd_empty()" test be
about the _flags_, and then both the fp_empty() test and the test
inside fdput() would be testing the same things.

Sadly, we'd need another bit in the flags. One option is easy enough -
we'd just have to make 'struct file' always be 8-byte aligned, which
it effectively always is.

Or we'd need to make the rule be that FDPUT_POS_UNLOCK only gets set
if FDPUT_FPUT is set.

Because I think we could have even a two-bit tag value have that "no fd" case:

 00 - no fd
 01 - fd but no need for fput
 10 - fd needs fput
 11 - fd needs pos unlock and fput

but as it is, that's not what we have. Right now we have

  00 - no fd or fd with no need for fput ("look at fd.file to decide")
  01 - fd needs fput
  10 - fd pos unlock but no fput
  11 - fd pos unlock and fput

but that 10 case looks odd to me. Why would we ever need a pos unlock
but no fput? The reason we don't need an fput is that we're the only
thread that has access to the file pointer, but that also implies that
we shouldn't need to lock the position.

So now I've just confused myself. Why *do* we have that 10 pattern?

Adding a separate bit would certainly avoid any complexity, and then
you'd have "flags==0 means no file pointer" and the "fd_empty()" test
would then make the fdput) test obviously unnecessary in the usual
pattern.

             Linus