linux-ext4 - [PATCH v6 8/9] vfs: update times after copying data in __generic_file_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Sep 2022 07:18:39 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     tytso@....edu, adilger.kernel@...ger.ca, djwong@...nel.org,
        david@...morbit.com, trondmy@...merspace.com, neilb@...e.de,
        viro@...iv.linux.org.uk, zohar@...ux.ibm.com, xiubli@...hat.com,
        chuck.lever@...cle.com, lczerner@...hat.com, jack@...e.cz,
        bfields@...ldses.org, brauner@...nel.org, fweimer@...hat.com
Cc:     linux-btrfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, ceph-devel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-xfs@...r.kernel.org
Subject: [PATCH v6 8/9] vfs: update times after copying data in __generic_file_write_iter

The c/mtime and i_version currently get updated before the data is
copied (or a DIO write is issued), which is problematic for NFS.

READ+GETATTR can race with a write (even a local one) in such a way as
to make the client associate the state of the file with the wrong change
attribute. That association can persist indefinitely if the file sees no
further changes.

Move the setting of times to the bottom of the function in
__generic_file_write_iter and only update it if something was
successfully written.

If the time update fails, log a warning once, but don't fail the write.
All of the existing callers use update_time functions that don't fail,
so we should never trip this.

Signed-off-by: Jeff Layton <jlayton@...nel.org>
---
 mm/filemap.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 15800334147b..72c0ceb75176 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3812,10 +3812,6 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (err)
 		goto out;
 
-	err = file_update_time(file);
-	if (err)
-		goto out;
-
 	if (iocb->ki_flags & IOCB_DIRECT) {
 		loff_t pos, endbyte;
 
@@ -3868,6 +3864,19 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			iocb->ki_pos += written;
 	}
 out:
+	if (written > 0) {
+		err = file_update_time(file);
+		/*
+		 * There isn't much we can do at this point if updating the
+		 * times fails after a successful write. The times and i_version
+		 * should still be updated in the inode, and it should still be
+		 * marked dirty, so hopefully the next inode update will catch it.
+		 * Log a warning once so we have a record that something untoward
+		 * has occurred.
+		 */
+		WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
+	}
+
 	current->backing_dev_info = NULL;
 	return written ? written : err;
 }
-- 
2.37.3