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

[Dave Chinner]

On Tue, May 21, 2013 at 10:56:05PM -0400, Dave Jones wrote:
> On Wed, May 22, 2013 at 10:16:03AM +1000, Dave Chinner wrote:
> 
> Seems like I can trigger this from paths other than truncate too.. (eg, sys_open)

O_TRUNC?

> The mask is always 0xa068 though if that helps.

A bit - it confirms what I thought - a truncate killing SUID bits.
i.e. mask = ATTR_FORCE|ATTR_KILL_SUID|ATTR_CTIME|ATTR_SIZE.

But then here's the next question - where the hell is the ATTR_MODE
bit?  In do_truncate(), the should_remove_suid() return is or'd into
the mask along with ATTR_FORCE. The ATTR_KILL_SUID is returned after
this check:

        umode_t mode = dentry->d_inode->i_mode;
        int kill = 0;

        /* suid always must be killed */
        if (unlikely(mode & S_ISUID))
                kill = ATTR_KILL_SUID;

However, notify_change() takes this mask and the inode and does this
when the ATTR_KILL_SUID flag is set:

	umode_t mode = inode->i_mode;
...
        if (ia_valid & ATTR_KILL_SUID) {
                if (mode & S_ISUID) {
                        ia_valid = attr->ia_valid |= ATTR_MODE;
                        attr->ia_mode = (inode->i_mode & ~S_ISUID);
                }
        }


So, we know that (mode & S_ISUID) is true, because that's how
ATTR_KILL_SUID gets set, but that same check here doesn't result
in ATTR_MODE being set....

That doesn't make a whole lot of sense to me. What am I missing?
Are you seeing this fire at all from notify_change()?

	WARN_ON_ONCE(!mutex_is_locked(&inode->i_mutex));

<Light Bulb>

What's wrong with this code in do_truncate()?

        /* Remove suid/sgid on truncate too */
        ret = should_remove_suid(dentry);
        if (ret)
                newattrs.ia_valid |= ret | ATTR_FORCE;

        mutex_lock(&dentry->d_inode->i_mutex);
        ret = notify_change(dentry, &newattrs);
        mutex_unlock(&dentry->d_inode->i_mutex);

Patch below to fix this.

However, it probably doesn't fix the fact that truncate can change
the size and kill suid/sgid bits at the same time and XFS doesn't
appear to handle that sanely right now. Can you run the patch below
just so when it fails we can see that the mask is actually sane?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

vfs: do_truncate() needs to serialise should_remove_suid

From: Dave Chinner <dchinner@xxxxxxxxxx>

Otherwise someone else can come in an remove the suid bit before the
truncate locks the inode and calls notify_change(). This results in
XFS throwing asserts because it's getting a strange attr mask being
passed down to it that it doesn't know how to handle correctly.

Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

---
 fs/open.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index 8c74100..b8d4899 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -52,11 +52,11 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	}
 
 	/* Remove suid/sgid on truncate too */
+	mutex_lock(&dentry->d_inode->i_mutex);
 	ret = should_remove_suid(dentry);
 	if (ret)
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
-	mutex_lock(&dentry->d_inode->i_mutex);
 	ret = notify_change(dentry, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return ret;
--
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/