Re: [PATCH] mm: add missing mutex lock arround notify_change

[Al Viro]

On Fri, Dec 16, 2011 at 12:55:56PM -0800, Andrew Morton wrote:

> >  static int __remove_suid(struct dentry *dentry, int kill)
> >  {
> > +	int ret;
> >  	struct iattr newattrs;
> >  
> >  	newattrs.ia_valid = ATTR_FORCE | kill;
> > -	return notify_change(dentry, &newattrs);
> > +
> > +	mutex_lock(&dentry->d_inode->i_mutex);
> > +	ret = notify_change(dentry, &newattrs);
> > +	mutex_unlock(&dentry->d_inode->i_mutex);
> > +
> > +	return ret;
> >  }

Consider this:
generic_file_aio_write():
        mutex_lock(&inode->i_mutex);
...
        ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);

and from there we have
        err = file_remove_suid(file);
which calls __remove_suid()

Deadlock.  OK, let's look at the callers:

__remove_suid() <- file_remove_suid()

file_remove_suid() <-
	xip_file_write()			! we grab i_mutex there
	__generic_file_aio_write() <-
		generic_file_aio_write()	! we grab i_mutex there
		pohmelfs_write()		! we grab i_mutex there
		blkdev_aio_write()
	generic_file_splice_write()		! we grab i_mutex there
	xfs_file_aio_write_checks()
	ntfs_file_aio_write_nolock() <-
		ntfs_file_aio_write()		! we grab i_mutex there
	fuse_file_aio_write()			! we grab i_mutex there
	btrfs_file_aio_write()			! we grab i_mutex there
	ext4_ioctl(), EXT4_IOC_MOVE_EXT case

We have a shitload of deadlocks on very common paths with that patch.  What
of the paths that do lead to file_remove_suid() without i_mutex?
*	xfs_file_aio_write_checks(): we drop i_mutex (via xfs_rw_iunlock())
just before calling file_remove_suid().  Racy, the fix is obvious - move
file_remove_suid() call before unlocking.
*	ext4_ioctl(): doesn't bother with i_mutex at all, very likely to be
racy.  BTW, that file_remove_suid() belongs *before* mnt_drop_write(), for
obvious reasons.
*	blkdev_aio_write(): file_remove_suid() will be called, but it won't
reach __remove_suid() - should_remove_suid() returns 0 unless we are dealing
with regular file.  And for blkdev_aio_write() that file will be a block
device.

IOW, this patch is bogus and would have deadlocked the box as soon as one
would try to do write(2) on suid file.  Testing Is A Good Thing(tm).

xfs and ext4_ioctl() need to be fixed; XFS fix follows, ext4 I'd rather left
to ext4 folks - I don't know how wide an area needs i_mutex there

xfs: call file_remove_suid() before dropping i_mutex

Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 753ed9b..33705b1 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -750,17 +750,16 @@ restart:
 		*new_sizep = new_size;
 	}
 
-	xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
-	if (error)
-		return error;
-
 	/*
 	 * 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.
 	 */
-	return file_remove_suid(file);
+	if (!error)
+		error = file_remove_suid(file);
 
+	xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
 }
 
 /*
--
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/