copy_file_range: Fix file offset handling

[markj]

Summary

One can ask copy_file_range(2) to use the file offsets of the file
descriptions that it copies from and to. We were updating those offsets
without any locking, which is incorrect and can lead to unkillable loops
in the event of a race (e.g., the check for overlapping ranges in
kern_copy_file_range() is subject to a TOCTOU race with the following
loop which range-locks the input and output file).

Use foffset_lock() to serialize updates to the file descriptions, as we
do for other, similar system calls.

Reported by: syzkaller
Fixes: bbbbeca3e9a3 ("Add kernel support for a Linux compatible copy_file_range(2) syscall.")

[kib]

sys/kern/vfs_syscalls.c

5015

Isn't it deadlock-prone? If one thread does c_f_r(f1, f2) and another c_f_r(f2, f1).

[rmacklem]

sys/kern/vfs_syscalls.c

5015

Yea, it looks like foffset_lock() needs a new FOF_NOBLOCK
flag, so that both locks can be acquired without deadlock.
(Returns something like -1 when it needs to sleep waiting for
the lock when FOF_NOBLOCK is passed in.)

(I'll admit I do not understand what the offset locking is for
at this point, but it certainly looks like it could deadlock.)

[markj]

sys/kern/vfs_syscalls.c

5015

Yep, I need to add a trylock operation and a loop which can safely lock a pair of offsets, as we do with rangelocks below.

[markj]

Avoid deadlocking when locking multiple file offsets.

[rmacklem]

sys/kern/vfs_syscalls.c

5018

This works. It might be cleaner to flip inoff and outoff
for each retry.
In other words, hold the lock on outfp, but change the
infp lock call to FOF_NOBLOCK.
(You could make the flag arguments for both calls variables
and swap them between 0 and FOF_NOBLOCK, I think?)

[markj]

Simplify control flow a bit.

[kib]

sys/kern/vfs_syscalls.c

4991

goto out assumes that file offsets are locked

5008–5009

Should the condition be '== NULL'?

5012

Similarly, == ?

5032

You might want to insert a call to vn_lock_pair_pause() there.

[markj]

sys/kern/vfs_syscalls.c

4991

It uses the savinoff == -1 check to guard this.

5008–5009

No, the check is asking "did the caller provide an offset for the input file?" If so, we only need to lock the output file's offset.

5012

No, this is just the mirror of the previous case.

5018

I rewrote this bit before seeing your comment, maybe it's a bit cleaner? I tried to follow the pattern of the rangelock loop below, and avoid complicated control flow.

5032

Hmm, maybe - should I add it for the rangelock loop below too?