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

[Linus Torvalds]

On Sat, Mar 26, 2022 at 3:15 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> but maybe I'm missing something.

Side note: the thing I'm _definitely_ missing is context for this patch.

I'm seeing "4/4" in the subject, but I can't find 1-3.

And the patch most definitely doesn't apply as-is, because that
'add_byte' logic that it removes lines around doesn't exist in my tree
at least.

And I _do_ think that regardless of anything else, having those
calculations use BITS_PER_BYTE - as if byte level operations were
valid - is misleading.

I do find something dodgy: alloc_fdtable().

That function very much tries to keep things to that multiple of
BITS_PER_LONG, and even has a comment to that effect, but I wonder if
it is broken.

In particular, that

  nr = ... | (BITS_PER_LONG - 1)) + 1;

case is only done for the "nr > sysctl_nr_open" case. The other case
*DOES* align things up, but not necessarily sufficiently, in that it
does

        nr /= (1024 / sizeof(struct file *));
        nr = roundup_pow_of_two(nr + 1);
        nr *= (1024 / sizeof(struct file *));

and I think that despite the round-up, it could easily be a smaller
power-of-two than BITS_PER_LONG.

So I think that code _intended_ for things to always be a multiple of
BITS_PER_LONG, but I'm not convinced it is.

It would be a good idea to just make it very explicit that it's
properly aligned, using

    --- a/fs/file.c
    +++ b/fs/file.c
    @@ -111,7 +111,8 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
         * bitmaps handling below becomes unpleasant, to put it mildly...
         */
        if (unlikely(nr > sysctl_nr_open))
    -           nr = ((sysctl_nr_open - 1) | (BITS_PER_LONG - 1)) + 1;
    +           nr = sysctl_nr_open;
    +   nr = ALIGN(nr, BITS_PER_LONG);

        fdt = kmalloc(sizeof(struct fdtable), GFP_KERNEL_ACCOUNT);
        if (!fdt)

although that would perhaps break the whole "we try to allocate in
comfortable page-tuned chunks" code, so that obvious patch may be
doing non-obvious bad things.

Maybe it's the roundup_pow_of_two() that should be fixed to be that
proper BITS_PER_LONG alignment instead?

And related to this all, we do have that BITS_PER_BYTE thing in a few
places, and they are all a bit dodgy:

 - copy_fd_bitmaps() uses BITS_PER_BYTE, but I do think that all the
values it operates on _should_ be BITS_PER_LONG aligned

 - alloc_fdtable() again does "2 * nr / BITS_PER_BYTE" for the bitmap
allocations

and we should make really really sure that we're always doing
BITS_PER_LONG, and that alloc_fdtable() calculation should be checked.

Hmm?

                   Linus