Page MenuHomeFreeBSD
Differential D49440

copy_file_range: Fix file offset handling
ClosedPublic
Actions

Authored by markj on Fri, Mar 21, 2:09 PM.
Tags
None
Referenced Files
F114422571: D49440.id152614.diff
Sat, Apr 12, 9:26 AM
Unknown Object (File)
Fri, Apr 11, 2:55 PM
Unknown Object (File)
Mon, Apr 7, 1:09 AM
Unknown Object (File)
Fri, Apr 4, 9:21 PM
Unknown Object (File)
Mon, Mar 31, 12:26 PM
Unknown Object (File)
Mon, Mar 31, 7:07 AM
Unknown Object (File)
Mon, Mar 31, 7:07 AM
Unknown Object (File)
Fri, Mar 28, 8:08 AM
View All 21 Files
Subscribers
imp
mjg
olce

Details

Reviewers
rmacklem
kib
Commits
rG197997a4c36d: file: Fix offset handling in kern_copy_file_range()
rG12ecb0fe0afd: file: Add foffset_lock_pair()
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.")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 63094
Build 59978: arc lint + arc unit

Event Timeline

markj created this revision.Fri, Mar 21, 2:09 PM
Herald added subscribers: olce, imp. · View Herald TranscriptFri, Mar 21, 2:09 PM
markj requested review of this revision.Fri, Mar 21, 2:09 PM
Harbormaster completed remote builds in B63026: Diff 152498.Fri, Mar 21, 2:09 PM
kib added inline comments.Fri, Mar 21, 10:11 PM
sys/kern/vfs_syscalls.c
5009

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

rmacklem added inline comments.Sat, Mar 22, 1:46 AM
sys/kern/vfs_syscalls.c
5009

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 added inline comments.Sat, Mar 22, 2:05 AM
sys/kern/vfs_syscalls.c
5009

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 updated this revision to Diff 152531.Sat, Mar 22, 3:06 AM

Avoid deadlocking when locking multiple file offsets.

Harbormaster completed remote builds in B63052: Diff 152531.Sat, Mar 22, 3:06 AM
rmacklem added inline comments.Sat, Mar 22, 10:20 PM
sys/kern/vfs_syscalls.c
5012

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 updated this revision to Diff 152540.Sun, Mar 23, 2:38 AM

Simplify control flow a bit.

Harbormaster completed remote builds in B63059: Diff 152540.Sun, Mar 23, 2:38 AM
kib added inline comments.Sun, Mar 23, 6:53 AM
sys/kern/vfs_syscalls.c
4992

goto out assumes that file offsets are locked

5002–5003

Should the condition be '== NULL'?

5006

Similarly, == ?

5026

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

markj marked an inline comment as done.Sun, Mar 23, 9:45 AM
markj added inline comments.
sys/kern/vfs_syscalls.c
4992

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

5002–5003

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.

5006

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

5012

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.

5026

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

kib added inline comments.Sun, Mar 23, 10:59 AM
sys/kern/vfs_syscalls.c
4992

But then it depends on the fact that foffset cannot return -1?

5002–5003

Ok

5026

I think yes it would be useful too.

markj updated this revision to Diff 152553.Sun, Mar 23, 11:44 AM
markj marked 5 inline comments as done.

Use a dedicated flag variable to track foffset locking.

Harbormaster completed remote builds in B63065: Diff 152553.Sun, Mar 23, 11:46 AM
kib accepted this revision.Sun, Mar 23, 11:50 AM
This revision is now accepted and ready to land.Sun, Mar 23, 11:50 AM
rmacklem accepted this revision.Sun, Mar 23, 1:13 PM

Looks fine to me.

mjg added a subscriber: mjg.Sun, Mar 23, 1:34 PM

can you instead sort the files and require the ordering that smaller is addr is locked first? a dedicated routine could do it

this would also avoid failures to lock to begin with

markj added a comment.Sun, Mar 23, 1:47 PM
In D49440#1127936, @mjg wrote:

can you instead sort the files and require the ordering that smaller is addr is locked first? a dedicated routine could do it

this would also avoid failures to lock to begin with

Yes that should work too and is a bit simpler. I will implement it in a bit.

markj updated this revision to Diff 152614.Mon, Mar 24, 12:57 PM

Instead of using a trylock operation, introduce foffset_lock_pair()
and use that instead.

This revision now requires review to proceed.Mon, Mar 24, 12:57 PM
Harbormaster completed remote builds in B63094: Diff 152614.Mon, Mar 24, 12:57 PM
rmacklem accepted this revision.Mon, Mar 24, 1:21 PM

This looks fine to me and does simplify things, I think?

This revision is now accepted and ready to land.Mon, Mar 24, 1:21 PM
kib accepted this revision.Mon, Mar 24, 1:32 PM
Closed by commit rG12ecb0fe0afd: file: Add foffset_lock_pair() (authored by markj). · Explain WhyMon, Mar 31, 9:07 AM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rG12ecb0fe0afd: file: Add foffset_lock_pair().
markj added a commit: rG197997a4c36d: file: Fix offset handling in kern_copy_file_range().

Revision Contents
Changeset List

PathSize
sys/
kern/
73 lines
20 lines
sys/
2 lines

Diff 152614

View Options

sys/kern/vfs_syscalls.c

Show First 20 Lines • Show All 230 Lines • ▼ Show 20 Lines
/* /*
* Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC * Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC
*/ */
struct pfsyncreq { struct pfsyncreq {
char pfsyncr_syncdev[IFNAMSIZ]; char pfsyncr_syncdev[IFNAMSIZ];
struct in_addr pfsyncr_syncpeer; struct in_addr pfsyncr_syncpeer;
int pfsyncr_maxupdates; int pfsyncr_maxupdates;
#define PFSYNCF_OK 0x00000001
#define PFSYNCF_DEFER 0x00000002
int pfsyncr_defer; int pfsyncr_defer;
}; };
#define SIOCSETPFSYNC _IOW('i', 247, struct ifreq) #define SIOCSETPFSYNC _IOW('i', 247, struct ifreq)
#define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq) #define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq)
#ifdef _KERNEL #ifdef _KERNEL
Show All 20 Lines
View Options

sys/kern/vfs_vnops.c

Show First 20 Lines • Show All 230 Lines • ▼ Show 20 Lines
/* /*
* Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC * Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC
*/ */
struct pfsyncreq { struct pfsyncreq {
char pfsyncr_syncdev[IFNAMSIZ]; char pfsyncr_syncdev[IFNAMSIZ];
struct in_addr pfsyncr_syncpeer; struct in_addr pfsyncr_syncpeer;
int pfsyncr_maxupdates; int pfsyncr_maxupdates;
#define PFSYNCF_OK 0x00000001
#define PFSYNCF_DEFER 0x00000002
int pfsyncr_defer; int pfsyncr_defer;
}; };
#define SIOCSETPFSYNC _IOW('i', 247, struct ifreq) #define SIOCSETPFSYNC _IOW('i', 247, struct ifreq)
#define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq) #define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq)
#ifdef _KERNEL #ifdef _KERNEL
Show All 20 Lines
View Options

sys/sys/file.h

Show First 20 Lines • Show All 230 Lines • ▼ Show 20 Lines
/* /*
* Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC * Configuration structure for SIOCSETPFSYNC SIOCGETPFSYNC
*/ */
struct pfsyncreq { struct pfsyncreq {
char pfsyncr_syncdev[IFNAMSIZ]; char pfsyncr_syncdev[IFNAMSIZ];
struct in_addr pfsyncr_syncpeer; struct in_addr pfsyncr_syncpeer;
int pfsyncr_maxupdates; int pfsyncr_maxupdates;
#define PFSYNCF_OK 0x00000001
#define PFSYNCF_DEFER 0x00000002
int pfsyncr_defer; int pfsyncr_defer;
}; };
#define SIOCSETPFSYNC _IOW('i', 247, struct ifreq) #define SIOCSETPFSYNC _IOW('i', 247, struct ifreq)
#define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq) #define SIOCGETPFSYNC _IOWR('i', 248, struct ifreq)
#ifdef _KERNEL #ifdef _KERNEL
Show All 20 Lines