[PATCH] fs: annotate lockless file position updates in sendfile

[Nguyen Ngoc Thang]

sendfile(2) does not take f_pos_lock, so with a NULL offset it reads
and writes file->f_pos of both files locklessly. Two tasks sharing a
file, or one running lseek(2), race on it. The splice read paths
likewise update in->f_ra.prev_pos, a readahead hint, without
serialization. KCSAN reports all of these:

  BUG: KCSAN: data-race in do_sendfile / do_sendfile
  BUG: KCSAN: data-race in do_sendfile / generic_file_llseek_size
  BUG: KCSAN: data-race in shmem_file_splice_read / shmem_file_splice_read

The races are benign. The worst outcome of concurrent sendfile() on a
shared file position is an unpredictable offset, which is already the
case for any caller that shares an fd without synchronizing. prev_pos
only steers readahead.

Mark the accesses with data_race(). Locking f_pos in sendfile would
change behavior for no gain.

Reported-by: syzbot+399510418a0b0cf1fc29@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=399510418a0b0cf1fc29
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
---
Reproduced in QEMU with the syzbot C repro on a KCSAN build of
cfec639c844f: 42 and 43 reports over two boots, covering all three
pairs above. With this patch there are none for those pairs over four
boots. The remaining reports are lseek/lseek between the repro's own
threads (a different pair) and an unrelated ps2 race.

I used data_race() rather than taking f_pos_lock, which would change
sendfile behavior. Happy to respin if you prefer locking.

No Fixes tag: the race predates the history I can bisect.
 fs/read_write.c | 11 +++++++----
 mm/filemap.c    |  3 ++-
 mm/shmem.c      |  3 ++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index e8c14e2760b2..ca2916472e7d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1319,7 +1319,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 	if (!(fd_file(in)->f_mode & FMODE_READ))
 		return -EBADF;
 	if (!ppos) {
-		pos = fd_file(in)->f_pos;
+		/* f_pos is not locked here; racing updates are benign */
+		pos = data_race(fd_file(in)->f_pos);
 	} else {
 		pos = *ppos;
 		if (!(fd_file(in)->f_mode & FMODE_PREAD))
@@ -1341,7 +1342,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		return -EBADF;
 	in_inode = file_inode(fd_file(in));
 	out_inode = file_inode(fd_file(out));
-	out_pos = fd_file(out)->f_pos;
+	/* see above */
+	out_pos = data_race(fd_file(out)->f_pos);
 
 	if (!max)
 		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
@@ -1382,11 +1384,12 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		add_wchar(current, retval);
 		fsnotify_access(fd_file(in));
 		fsnotify_modify(fd_file(out));
-		fd_file(out)->f_pos = out_pos;
+		/* see above */
+		data_race(fd_file(out)->f_pos = out_pos);
 		if (ppos)
 			*ppos = pos;
 		else
-			fd_file(in)->f_pos = pos;
+			data_race(fd_file(in)->f_pos = pos); /* see above */
 	}
 
 	inc_syscr(current);
diff --git a/mm/filemap.c b/mm/filemap.c
index 00fd89cf6f55..d5a4a5ead4e2 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3146,7 +3146,8 @@ ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
 			len -= n;
 			total_spliced += n;
 			*ppos += n;
-			in->f_ra.prev_pos = *ppos;
+			/* readahead hint, racy updates are benign */
+			data_race(in->f_ra.prev_pos = *ppos);
 			if (pipe_is_full(pipe))
 				goto out;
 		}
diff --git a/mm/shmem.c b/mm/shmem.c
index 897fa2b61346..56fa2bc520ed 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3586,7 +3586,8 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
 		len -= n;
 		total_spliced += n;
 		*ppos += n;
-		in->f_ra.prev_pos = *ppos;
+		/* readahead hint, racy updates are benign */
+		data_race(in->f_ra.prev_pos = *ppos);
 		if (pipe_is_full(pipe))
 			break;
 
-- 
2.43.0