Re: XFS assertion from truncate. (3.10-rc2)

[Dave Chinner]

On Thu, May 23, 2013 at 11:03:00PM -0400, Dave Jones wrote:
> On Thu, May 23, 2013 at 09:52:19PM -0400, Dave Jones wrote:
>  > On Fri, May 24, 2013 at 11:26:25AM +1000, Dave Chinner wrote:
>  >  
>  >  > You want to print the debug output if the masked value != 0.
>  > 
>  > XFS (sda2): xfs_setattr_size: mask 0xaa69, masked 0x801 ii 0xffff8802129e98c0, d 0xffff88021757d970 path /davej/src/trinity/tmp/tmp.5/
> 
> ah, sneaky. There's some unprintable characters there that didn't
> show up in my terminal when I cut-n-pasted.  They made it to the
> logs though..
> 
> XFS (sda2): xfs_setattr_size: mask 0xaa69, masked 0x801 ii 0xffff8802129e98c0, d 0xffff88021757d970 path /davej/src/trinity/tmp/tmp.5/



So the mask is:

	ATTR_OPEN | ATTR_FILE | ATTR_KILL_SUID | ATTR_FORCE | \
	ATTR_CTIME | ATTR_MTIME | ATTR_MODE

And so the mask is tripping the assert is (ATTR_KILL_SUID |
ATTR_MODE).

Ok, that now makes sense - that's consistent with open(O_TRUNCATE)
on a suid file. How the hell has this gone unnoticed?

> Hexdump: of that part.. 706d 352e 012f 0a01 
> So filename is 0x01 0x01
> 
> Don't know if that's relevant to the bug or not..

No, it's not. It seems like the XFS code is simply broken, and has
been for a couple of years. This is the commit that changed the code
into the split we currently have now:

commit c4ed4243c40f97ed5b7b121777bbbc6aeaa722f0
Author: Christoph Hellwig <hch@xxxxxx>
Date:   Fri Jul 8 14:34:23 2011 +0200

    xfs: split xfs_setattr
    
    Split up xfs_setattr into two functions, one for the complex truncate
    handling, and one for the trivial attribute updates.  Also move both
    new routines to xfs_iops.c as they are fairly Linux-specific.
    
    Signed-off-by: Christoph Hellwig <hch@xxxxxx>
    Reviewed-by: Alex Elder <aelder@xxxxxxx>
    Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

Clearly there's a simple test that xfstests is missing, not to
mention every other test suite out there that is run regularly on
linux....

Awww, hell.

xfstest generic/193 is *supposed* to test behaviour of suid/sgid bits
and clearing them is various situations.

You know what I'm about to say, don't you? The test doesn't test
what it thinks it is testing. it puts the destination file in root
directory of the xfstests harness, not in the filesystems being
tested.

So, on all my machines, it runs on ext3 filesystems, never on the
ext4, btrfs, xfs, etc filesystems that I'm actually testing.

That's beside the point, because it doesn't test truncate behaviour.
But at least I know now why my attempts to reproduce the problem
didn't work...

Right, patch below should fix the problem.

What a frustrating bug. Now, where's my bottle of scotch?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

xfs: kill suid/sgid through the truncate path.

From: Dave Chinner <dchinner@xxxxxxxxxx>

XFS has failed to kill suid/sgid bits correctly when truncating
files of non-zero size since commit c4ed4243 ("xfs: split
xfs_setattr") introduced in the 3.1 kernel. Fix it.

Fix it.

Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
---
 fs/xfs/xfs_iops.c |   43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index d82efaa..c255382 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -455,6 +455,24 @@ xfs_vn_getattr(
 	return 0;
 }
 
+static void
+xfs_setattr_mode(
+	struct inode	*inode,
+	struct iattr	*iattr)
+{
+	struct xfs_inode *ip = XFS_I(inode);
+	umode_t		mode = iattr->ia_mode;
+
+	if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+		mode &= ~S_ISGID;
+
+	ip->i_d.di_mode &= S_IFMT;
+	ip->i_d.di_mode |= mode & ~S_IFMT;
+
+	inode->i_mode &= S_IFMT;
+	inode->i_mode |= mode & ~S_IFMT;
+}
+
 int
 xfs_setattr_nonsize(
 	struct xfs_inode	*ip,
@@ -606,18 +624,8 @@ xfs_setattr_nonsize(
 	/*
 	 * Change file access modes.
 	 */
-	if (mask & ATTR_MODE) {
-		umode_t mode = iattr->ia_mode;
-
-		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-			mode &= ~S_ISGID;
-
-		ip->i_d.di_mode &= S_IFMT;
-		ip->i_d.di_mode |= mode & ~S_IFMT;
-
-		inode->i_mode &= S_IFMT;
-		inode->i_mode |= mode & ~S_IFMT;
-	}
+	if (mask & ATTR_MODE)
+		xfs_setattr_mode(inode, iattr);
 
 	/*
 	 * Change file access or modified times.
@@ -714,9 +722,8 @@ xfs_setattr_size(
 		return XFS_ERROR(error);
 
 	ASSERT(S_ISREG(ip->i_d.di_mode));
-	ASSERT((mask & (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-			ATTR_MTIME_SET|ATTR_KILL_SUID|ATTR_KILL_SGID|
-			ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
+	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
+			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
 
 	if (!(flags & XFS_ATTR_NOLOCK)) {
 		lock_flags |= XFS_IOLOCK_EXCL;
@@ -860,6 +867,12 @@ xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
+	/*
+	 * Change file access modes.
+	 */
+	if (mask & ATTR_MODE)
+		xfs_setattr_mode(inode, iattr);
+
 	if (mask & ATTR_CTIME) {
 		inode->i_ctime = iattr->ia_ctime;
 		ip->i_d.di_ctime.t_sec = iattr->ia_ctime.tv_sec;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/