linux-kernel - Re: fs/attr.c:notify_change locking warning.

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:	Tue, 15 Oct 2013 13:19:05 -0700
From:	Christoph Hellwig <hch@...radead.org>
To:	Dave Chinner <david@...morbit.com>
Cc:	Dave Jones <davej@...hat.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Al Viro <viro@...iv.linux.org.uk>, xfs@....sgi.com
Subject: Re: fs/attr.c:notify_change locking warning.

On Sat, Oct 05, 2013 at 01:19:18PM +1000, Dave Chinner wrote:
> Yup, we don't hold the i_mutex *at all* through the fast path for
> direct IO writes. Having to grab the i_mutex on every IO just for
> the extremely unlikely case we need to remove a suid bit on the file
> would add a significant serialisation point into the direct Io model
> that XFS uses, and is the difference between 50,000 and 2+ million
> direct IO IOPS to a single file.
> 
> I'm unwilling to sacrifice the concurrency of direct IO writes just
> to shut up ths warning, especially as the actual modifications that
> are made to remove SUID bits are correctly serialised within XFS
> once notify_change() calls ->setattr(). If it really matters, I'll
> just open code file_remove_suid() into XFS like ocfs2 does just so
> we don't get that warning being emitted by trinity.

But the i_lock doesn't synchronize against the VFS modifying various
struct inode fields.  The right fix is to take i_mutex just in case
we actually need to remove the suid bit.  The patch below should fix it,
although I need to write a testcase that actually exercises it first.

Dave (J.): if you have time to try the patch below please go ahead,
if not I'll make sure to write an isolated test ASAP to verify it and
will then submit the change.

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 4c749ab..e879f96 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -590,8 +590,22 @@ restart:
 	 * If we're writing the file then make sure to clear the setuid and
 	 * setgid bits if the process is not being run by root.  This keeps
 	 * people from modifying setuid and setgid binaries.
+	 *
+	 * Note that file_remove_suid must be called with the i_mutex held,
+	 * so we have to go through some hoops here to make sure we hold it.
 	 */
-	return file_remove_suid(file);
+	if (!IS_NOSEC(inode) && should_remove_suid(file->f_path.dentry)) {
+		if (*iolock == XFS_IOLOCK_SHARED) {
+			mutex_lock(&inode->i_mutex);
+			error = file_remove_suid(file);
+			mutex_unlock(&inode->i_mutex);
+		} else {
+			error = file_remove_suid(file);
+		}
+
+	}
+
+	return error;
 }
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/