ima: prevent dead lock when a file is opened for direct io (take II)

ima: prevent dead lock when a file is opened for direct io (take II)

[Mimi Zohar]

Files are measured or appraised based on the IMA policy.  When a file,
in policy, is opened with the O_DIRECT flag, a deadlock occurs.

ima_process_measurement() takes the i_mutex, before calculating the
hash, appraising the hash against a 'good' known value stored as an
extended attribute, and caching the result, before releasing the
i_mutex.  Holding the i_mutex results in a lockdep with
do_blockdev_direct_IO(), which takes the i_mutex and releases it
for each read.

The first attempt at resolving this lockdep, by Dmitry Kasatkin,
temporarily removed the O_DIRECT flag and restored it, after
calculating the hash.  This resolved the lockdep, but the benefits
of using O_DIRECT, in terms of page cache, were lost.  Al Viro
objected to this patch, because it made the "existing locking rule
more convoluted".  (The existing (undocumented) IMA locking
rule prohibits ->read() of regular files from taking the i_mutex.
The change would have prohibited ->read() of regular files from
taking the i_mutex, unless they were opened with O_DIRECT.)

The IMA specific 'iint->mutex' was removed, because it caused dead
locking due to different locking orders in different cases (eg.
regular vs. setxattr).

This patch defines a new flag, O_DIRECT_HAVELOCK for lack of a better
name, to indicate the i_mutex has already been taken.  Based on this
flag, the i_mutex is not taken, again, in do_blockdev_direct_IO().

Similar changes for filesystems, that define their own .aio_read
instead of using the generic_file_aio_read(), will probably need
to be made.  Before going down this path, I'd appreciate any
comments/suggestions.

thanks,

Mimi

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 fs/direct-io.c                    | 12 +++++++++---
 include/uapi/asm-generic/fcntl.h  |  1 +
 security/integrity/ima/ima_main.c |  5 +++++
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 0e04142..d6444b9 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -1126,6 +1126,7 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 	size_t bytes;
 	struct buffer_head map_bh = { 0, };
 	struct blk_plug plug;
+	int skip_lock = 0;
 
 	if (rw & WRITE)
 		rw = WRITE_ODIRECT;
@@ -1179,14 +1180,19 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 		if (rw == READ) {
 			struct address_space *mapping =
 					iocb->ki_filp->f_mapping;
+			struct file *file = iocb->ki_filp;
 
 			/* will be released by direct_io_worker */
-			mutex_lock(&inode->i_mutex);
+			if (file->f_flags & O_DIRECT_HAVELOCK)
+				skip_lock = 1;
+			if (!skip_lock)
+				mutex_lock(&inode->i_mutex);
 
 			retval = filemap_write_and_wait_range(mapping, offset,
 							      end - 1);
 			if (retval) {
-				mutex_unlock(&inode->i_mutex);
+				if (!skip_lock)
+					mutex_unlock(&inode->i_mutex);
 				kmem_cache_free(dio_cache, dio);
 				goto out;
 			}
@@ -1331,7 +1337,7 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 	 * we can let i_mutex go now that its achieved its purpose
 	 * of protecting us from looking up uninitialized blocks.
 	 */
-	if (rw == READ && (dio->flags & DIO_LOCKING))
+	if (rw == READ && (dio->flags & DIO_LOCKING) && (!skip_lock))
 		mutex_unlock(&dio->inode->i_mutex);
 
 	/*
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 95e46c8..02c6996 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -45,6 +45,7 @@
 #endif
 #ifndef O_DIRECT
 #define O_DIRECT	00040000	/* direct disk access hint */
+#define O_DIRECT_HAVELOCK 04000000	/* direct disk access hint */
 #endif
 #ifndef O_LARGEFILE
 #define O_LARGEFILE	00100000
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 149ee11..0d2c93d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -191,6 +191,9 @@ static int process_measurement(struct file *file, const char *filename,
 
 	mutex_lock(&inode->i_mutex);
 
+	if (file->f_flags & O_DIRECT)
+		file->f_flags |= O_DIRECT_HAVELOCK;
+
 	iint = integrity_inode_get(inode);
 	if (!iint)
 		goto out;
@@ -237,6 +240,8 @@ out_digsig:
 	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
 		rc = -EACCES;
 out:
+	if (file->f_flags & O_DIRECT)
+		file->f_flags &= ~O_DIRECT_HAVELOCK;
 	mutex_unlock(&inode->i_mutex);
 	kfree(xattr_value);
 	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
-- 
1.8.1.4