Re: [PATCH 4/4] file: Fix file descriptor leak in copy_fd_bitmaps()

From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Fedor Pchelkin <aissur0002@gmail.com>,
	Alexey Khoroshilov <khoroshilov@ispras.ru>,
	Eric Biggers <ebiggers@kernel.org>,
	Christian Brauner <brauner@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] file: Fix file descriptor leak in copy_fd_bitmaps()
Date: Tue, 29 Mar 2022 23:08:55 -0700	[thread overview]
Message-ID: <CAHk-=wgPwyQTnSF2s7WSb+KnGn4FTM58NJ+-v-561W7xnDk2OA@mail.gmail.com> (raw)
In-Reply-To: <YkPo0N/CVHFDlB6v@zx2c4.com>

On Tue, Mar 29, 2022 at 10:21 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Peppering some printks, it looks like in `max_fds = ALIGN(max_fds,
> BITS_PER_LONG);`, max_fds is sometimes 4294967295 before the call, and
> that ALIGN winds up bringing it to 0.

Gaah. I actually went back and forth on the location of the ALIGN().

And then ended up putting it where it is for just a "smallest patch"
reason, which was clearly not the right thing to do.

The easier and more obvious fix was to just make the ALIGN() be at the
final 'return' statement, but I ended up moving it up just because I
didn't like how complicated the expression looked.

That was obviously very very wrong of me.

So does it help if you just remove that

        max_fds = ALIGN(max_fds, BITS_PER_LONG);

and instead make the final return be

        return ALIGN(min(count, max_fds), BITS_PER_LONG);

instead?

And now I feel like I should as penance just do what I tried to get
Christian to do, which was to just integrate the whole "don't even
bother looking past that passed-in max_fds" in count_open_files().

The whole organization of that "calculate current highest fd, only to
then ignore it if we didn't want that many file descriptors" is just
historical baggage.

                 Linus