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
