On Mon, 27 Nov 2023 at 02:27, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> So I've picked up your patch (vfs.misc). It's clever alright so thanks
> for the comments in there otherwise I would've stared at this for far
> too long.
Note that I should probably have commented on one other thing: that
whole "just load from fd[0] is always safe, because the fd[] array
always exists".
IOW, that whole "load and mask" thing only works when you know the
array exists at all.
Doing that "just mask the index" wouldn't be valid if "size = 0" is an
option and might mean that we don't have an array at all (ie if "->fd"
itself could be NULL.
But we never have a completely empty file descriptor array, and
fdp->fd is never NULL. At a minimum 'max_fds' is NR_OPEN_DEFAULT.
(The whole 'tsk->files' could be NULL, but only for kernel threads or
when exiting, so fget_task() will check for *that*, but it's a
separate thing)
So that's why it's safe to *entirely* remove the whole
if (unlikely(fd >= fdt->max_fds))
test, and do it *all* with just "mask the index, and mask the resulting load".
Because we can *always* do that load at "fdt->fd[0]", and we want to
check the result for NULL anyway, so the "mask at the end and check
for NULL" is both natural and generates very good code.
Anyway, not a big deal, bit it might be worth noting before somebody
tries the same trick on some other array that *could* be zero-sized
and with a NULL base pointer, and where that 'array[0]' access isn't
necessarily guaranteed to be ok.
> It's a little unpleasant because of the cast-orama going on before we
> check the file pointer but I don't see that it's in any way wrong.
In my cleanup phase - which was a bit messy - I did wonder if I should
have some helper for it, since it shows up in both __fget_files_rcu()
and in files_lookup_fd_raw().
So I *could* have tried to add something like a
"masked_rcu_dereference()" that took the base pointer, the index, and
the mask, and did that whole dance.
Or I could have had just a "mask_pointer()" function, which we do
occasionally do in other places too (ie we hide data in low bits, and
then we mask them away when the pointer is used as a pointer).
But with only two users, it seemed to add more conceptual complexity
than it's worth, and I was not convinced that we'd want to expose that
pattern and have others use it.
So having a helper might clarify things, but it might also encourage
wrong users. I dunno.
I suspect the only real use for this ends up being this very special
"access the fdt->fd[] array using a file descriptor".
Anyway, that's why I largely just did it with comments, and commented
both places - and just kept the cast there in the open.
Linus
