[PATCH v1.1 0/5] IMA: making i_readcount a first class inode citizen

[PATCH v1.1 0/5] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

Based on the previous posting discussion, i_readcount is now defined as
atomic.

This patchset separates the incrementing/decrementing of the i_readcount,
in the VFS layer, from other IMA functionality, by replacing the current
ima_counts_get() call with iget_readcount(). Its unclear whether this
call to increment i_readcount should be made earlier, like i_writecount.

The patch ordering is a bit redundant in order to leave removing the ifdef
around i_readcount until the last patch. The first four patches: redefines 
i_readcount as atomic, defines iget/iput_readcount(), moves the IMA
functionality in ima_counts_get() to ima_file_check(), and removes the IMA
imbalance code, simplifying IMA. The last patch moves iput_readcount()
to the fs directory and removes the ifdef around i_readcount, making
i_readcount into a "first class inode citizen".

The generic_setlease code could then take advantage of i_readcount.
 
Mimi

Mimi Zohar (5):
  IMA: convert i_readcount to atomic
  IMA: define readcount functions
  IMA: maintain i_readcount in the VFS layer
  IMA: remove IMA imbalance checking
  IMA: making i_readcount a first class inode citizen

 fs/file_table.c                   |   16 ++++-
 fs/inode.c                        |    3 +
 fs/open.c                         |    3 +-
 include/linux/fs.h                |    9 ++-
 include/linux/ima.h               |    6 --
 security/integrity/ima/ima_iint.c |    5 --
 security/integrity/ima/ima_main.c |  131 +++++--------------------------------
 7 files changed, 43 insertions(+), 130 deletions(-)

-- 
1.7.2.2

[PATCH v1.1 1/5] IMA: convert i_readcount to atomic

[Mimi Zohar]

Convert the inode's i_readcount from an unsigned int to atomic.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 include/linux/fs.h                |    2 +-
 security/integrity/ima/ima_iint.c |    7 ++++---
 security/integrity/ima/ima_main.c |   11 ++++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1eb2939..18d677c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -788,7 +788,7 @@ struct inode {
 
 #ifdef CONFIG_IMA
 	/* protected by i_lock */
-	unsigned int		i_readcount; /* struct files open RO */
+	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c442e47..f005355 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,11 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (inode->i_readcount)
-		printk(KERN_INFO "%s: readcount: %u\n", __func__, inode->i_readcount);
+	if (atomic_read(&inode->i_readcount))
+		printk(KERN_INFO "%s: readcount: %u\n", __func__,
+		       atomic_read(&inode->i_readcount));
 
-	inode->i_readcount = 0;
+	atomic_set(&inode->i_readcount, 0);
 
 	if (!IS_IMA(inode))
 		return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 203de97..a189197 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -113,7 +113,7 @@ void ima_counts_get(struct file *file)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount && IS_IMA(inode))
+		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
@@ -127,7 +127,7 @@ void ima_counts_get(struct file *file)
 out:
 	/* remember the vfs deals with i_writecount */
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		inode->i_readcount++;
+		atomic_inc(&inode->i_readcount);
 
 	spin_unlock(&inode->i_lock);
 
@@ -149,15 +149,16 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(inode->i_readcount == 0)) {
+	    	if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
 			if (!ima_limit_imbalance(file)) {
 				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__, inode->i_readcount);
+				       __func__,
+				       atomic_read(&inode->i_readcount));
 				dump_stack();
 			}
 			return;
 		}
-		inode->i_readcount--;
+		atomic_dec(&inode->i_readcount);
 	}
 }
 
-- 
1.7.2.2

[PATCH v1.1 2/5] IMA: define readcount functions

[Mimi Zohar]

Define iget/iput_readcount() functions to be called from the VFS layer.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 include/linux/fs.h                     |   16 ++++++++++++++++
 security/integrity/ima/Makefile        |    2 +-
 security/integrity/ima/ima_readcount.c |   25 +++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletions(-)
 create mode 100644 security/integrity/ima/ima_readcount.c

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 18d677c..7f5939d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2178,6 +2178,22 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
+#ifdef CONFIG_IMA
+extern void iput_readcount(struct inode *inode);
+static inline void iget_readcount(struct inode *inode)
+{
+	atomic_inc(&inode->i_readcount);
+}
+#else
+static inline void iput_readcount(struct inode *inode)
+{
+	return;
+}
+static inline void iget_readcount(struct inode *inode)
+{
+	return;
+}
+#endif
 extern int do_pipe_flags(int *, int);
 extern struct file *create_read_pipe(struct file *f, int flags);
 extern struct file *create_write_pipe(int flags);
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 787c4cb..131eb1f 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -6,4 +6,4 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_iint.o ima_audit.o
+	 ima_policy.o ima_iint.o ima_audit.o ima_readcount.o
diff --git a/security/integrity/ima/ima_readcount.c b/security/integrity/ima/ima_readcount.c
new file mode 100644
index 0000000..d139e2a9
--- /dev/null
+++ b/security/integrity/ima/ima_readcount.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2010 IBM Corporation
+ *
+ * Authors:
+ * Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ */
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/fs.h>
+
+void iput_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	if (unlikely((atomic_read(&inode->i_readcount) == 0)))
+		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
+		       inode->i_ino);
+	else
+		atomic_dec(&inode->i_readcount);
+	spin_unlock(&inode->i_lock);
+}
-- 
1.7.2.2

[PATCH v1.1 3/5] IMA: maintain i_readcount in the VFS layer

[Mimi Zohar]

ima_counts_get() updated the readcount and invalidated the PCR,
as necessary. Only update the i_readcount in the VFS layer.
Move the PCR invalidation checks to ima_file_check(), where it
belongs.

Maintaining the i_readcount in the VFS layer, will allow other
subsystems to use i_readcount.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 fs/file_table.c                   |    5 ++++-
 fs/inode.c                        |    3 +++
 fs/open.c                         |    3 ++-
 include/linux/ima.h               |    6 ------
 security/integrity/ima/ima_iint.c |    2 --
 security/integrity/ima/ima_main.c |   25 ++++++++-----------------
 6 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index c3dee38..e575e78 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -190,7 +190,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 		file_take_write(file);
 		WARN_ON(mnt_clone_write(path->mnt));
 	}
-	ima_counts_get(file);
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		iget_readcount(path->dentry->d_inode);
 	return file;
 }
 EXPORT_SYMBOL(alloc_file);
@@ -251,6 +252,8 @@ static void __fput(struct file *file)
 	fops_put(file->f_op);
 	put_pid(file->f_owner.pid);
 	file_sb_list_del(file);
+	if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		iput_readcount(inode);
 	if (file->f_mode & FMODE_WRITE)
 		drop_file_write_access(file);
 	file->f_path.dentry = NULL;
diff --git a/fs/inode.c b/fs/inode.c
index ae2727a..4676b98 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -170,6 +170,9 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	inode->i_nlink = 1;
 	inode->i_uid = 0;
 	inode->i_gid = 0;
+#ifdef CONFIG_IMA
+	atomic_set(&inode->i_readcount, 0);
+#endif
 	atomic_set(&inode->i_writecount, 0);
 	inode->i_size = 0;
 	inode->i_blocks = 0;
diff --git a/fs/open.c b/fs/open.c
index 4197b9e..1f6686f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -688,7 +688,8 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 		if (error)
 			goto cleanup_all;
 	}
-	ima_counts_get(f);
+	if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		iget_readcount(inode);
 
 	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 975837e..09e6e62 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,6 @@ extern void ima_inode_free(struct inode *inode);
 extern int ima_file_check(struct file *file, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern void ima_counts_get(struct file *file);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -53,10 +52,5 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
 	return 0;
 }
 
-static inline void ima_counts_get(struct file *file)
-{
-	return;
-}
-
 #endif /* CONFIG_IMA_H */
 #endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index f005355..68efe3b 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -141,8 +141,6 @@ void ima_inode_free(struct inode *inode)
 		printk(KERN_INFO "%s: readcount: %u\n", __func__,
 		       atomic_read(&inode->i_readcount));
 
-	atomic_set(&inode->i_readcount, 0);
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a189197..a0626bc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -86,17 +86,16 @@ out:
 }
 
 /*
- * ima_counts_get - increment file counts
+ * ima_rdwr_violation_check
  *
- * Maintain read/write counters for all files, but only
- * invalidate the PCR for measured files:
+ * Only invalidate the PCR for measured files:
  * 	- Opening a file for write when already open for read,
  *	  results in a time of measure, time of use (ToMToU) error.
  *	- Opening a file for read when already open for write,
  * 	  could result in a file measurement error.
  *
  */
-void ima_counts_get(struct file *file)
+static void ima_rdwr_violation_check(struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
@@ -104,13 +103,10 @@ void ima_counts_get(struct file *file)
 	int rc;
 	bool send_tomtou = false, send_writers = false;
 
-	if (!S_ISREG(inode->i_mode))
+	if (!S_ISREG(inode->i_mode) || !ima_initialized)
 		return;
 
-	spin_lock(&inode->i_lock);
-
-	if (!ima_initialized)
-		goto out;
+	mutex_lock(&inode->i_mutex);	/* file metadata: permissions, xattr */
 
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
@@ -125,11 +121,7 @@ void ima_counts_get(struct file *file)
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
-	/* remember the vfs deals with i_writecount */
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		atomic_inc(&inode->i_readcount);
-
-	spin_unlock(&inode->i_lock);
+	mutex_unlock(&inode->i_mutex);
 
 	if (send_tomtou)
 		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -158,7 +150,6 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 			}
 			return;
 		}
-		atomic_dec(&inode->i_readcount);
 	}
 }
 
@@ -203,8 +194,7 @@ static void ima_file_free_noiint(struct inode *inode, struct file *file)
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
  *
- * Flag files that changed, based on i_version;
- * and decrement the i_readcount.
+ * Flag files that changed, based on i_version
  */
 void ima_file_free(struct file *file)
 {
@@ -318,6 +308,7 @@ int ima_file_check(struct file *file, int mask)
 {
 	int rc;
 
+	ima_rdwr_violation_check(file);
 	rc = process_measurement(file, file->f_dentry->d_name.name,
 				 mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
 				 FILE_CHECK);
-- 
1.7.2.2

[PATCH v1.1 4/5] IMA: remove IMA imbalance checking

[Mimi Zohar]

Now that i_readcount is maintained by the VFS layer, remove the
imbalance checking in IMA. Cleans up the IMA code nicely.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 security/integrity/ima/ima_iint.c |    4 --
 security/integrity/ima/ima_main.c |  105 +++----------------------------------
 2 files changed, 8 insertions(+), 101 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 68efe3b..4ae7304 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,6 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (atomic_read(&inode->i_readcount))
-		printk(KERN_INFO "%s: readcount: %u\n", __func__,
-		       atomic_read(&inode->i_readcount));
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a0626bc..978a113 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -36,55 +36,6 @@ static int __init hash_setup(char *str)
 }
 __setup("ima_hash=", hash_setup);
 
-struct ima_imbalance {
-	struct hlist_node node;
-	unsigned long fsmagic;
-};
-
-/*
- * ima_limit_imbalance - emit one imbalance message per filesystem type
- *
- * Maintain list of filesystem types that do not measure files properly.
- * Return false if unknown, true if known.
- */
-static bool ima_limit_imbalance(struct file *file)
-{
-	static DEFINE_SPINLOCK(ima_imbalance_lock);
-	static HLIST_HEAD(ima_imbalance_list);
-
-	struct super_block *sb = file->f_dentry->d_sb;
-	struct ima_imbalance *entry;
-	struct hlist_node *node;
-	bool found = false;
-
-	rcu_read_lock();
-	hlist_for_each_entry_rcu(entry, node, &ima_imbalance_list, node) {
-		if (entry->fsmagic == sb->s_magic) {
-			found = true;
-			break;
-		}
-	}
-	rcu_read_unlock();
-	if (found)
-		goto out;
-
-	entry = kmalloc(sizeof(*entry), GFP_NOFS);
-	if (!entry)
-		goto out;
-	entry->fsmagic = sb->s_magic;
-	spin_lock(&ima_imbalance_lock);
-	/*
-	 * we could have raced and something else might have added this fs
-	 * to the list, but we don't really care
-	 */
-	hlist_add_head_rcu(&entry->node, &ima_imbalance_list);
-	spin_unlock(&ima_imbalance_lock);
-	printk(KERN_INFO "IMA: unmeasured files on fsmagic: %lX\n",
-	       entry->fsmagic);
-out:
-	return found;
-}
-
 /*
  * ima_rdwr_violation_check
  *
@@ -131,28 +82,6 @@ out:
 				  "open_writers");
 }
 
-/*
- * Decrement ima counts
- */
-static void ima_dec_counts(struct inode *inode, struct file *file)
-{
-	mode_t mode = file->f_mode;
-
-	assert_spin_locked(&inode->i_lock);
-
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-	    	if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
-			if (!ima_limit_imbalance(file)) {
-				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__,
-				       atomic_read(&inode->i_readcount));
-				dump_stack();
-			}
-			return;
-		}
-	}
-}
-
 static void ima_check_last_writer(struct ima_iint_cache *iint,
 				  struct inode *inode,
 				  struct file *file)
@@ -168,28 +97,6 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
 		iint->flags &= ~IMA_MEASURED;
 }
 
-static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
-			       struct file *file)
-{
-	mutex_lock(&iint->mutex);
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(inode, file);
-	ima_check_last_writer(iint, inode, file);
-
-	spin_unlock(&inode->i_lock);
-	mutex_unlock(&iint->mutex);
-}
-
-static void ima_file_free_noiint(struct inode *inode, struct file *file)
-{
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(inode, file);
-
-	spin_unlock(&inode->i_lock);
-}
-
 /**
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
@@ -205,12 +112,16 @@ void ima_file_free(struct file *file)
 		return;
 
 	iint = ima_iint_find(inode);
+	if (!iint)
+		return;
 
-	if (iint)
-		ima_file_free_iint(iint, inode, file);
-	else
-		ima_file_free_noiint(inode, file);
+	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
+	ima_check_last_writer(iint, inode, file);
 
+	spin_unlock(&inode->i_lock);
+	mutex_unlock(&iint->mutex);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
-- 
1.7.2.2

[PATCH v1.1 5/5] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

Finally, remove the ifdef's around i_readcount, making it a full
inode citizen so that other subsystems, such as leases, could use it.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 fs/file_table.c                        |   11 +++++++++++
 include/linux/fs.h                     |   13 -------------
 security/integrity/ima/Makefile        |    2 +-
 security/integrity/ima/ima_readcount.c |   25 -------------------------
 4 files changed, 12 insertions(+), 39 deletions(-)
 delete mode 100644 security/integrity/ima/ima_readcount.c

diff --git a/fs/file_table.c b/fs/file_table.c
index e575e78..a658adb 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -92,6 +92,17 @@ int proc_nr_files(ctl_table *table, int write,
 }
 #endif
 
+void iput_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	if (unlikely((atomic_read(&inode->i_readcount) == 0)))
+		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
+		       inode->i_ino);
+	else
+		atomic_dec(&inode->i_readcount);
+	spin_unlock(&inode->i_lock);
+}
+
 /* Find an unused file structure and return a pointer to it.
  * Returns NULL, if there are no more free file structures or
  * we run out of memory.
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7f5939d..9e296b3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -786,10 +786,8 @@ struct inode {
 
 	unsigned int		i_flags;
 
-#ifdef CONFIG_IMA
 	/* protected by i_lock */
 	atomic_t		i_readcount; /* struct files open RO */
-#endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
 	void			*i_security;
@@ -2178,22 +2176,11 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
-#ifdef CONFIG_IMA
 extern void iput_readcount(struct inode *inode);
 static inline void iget_readcount(struct inode *inode)
 {
 	atomic_inc(&inode->i_readcount);
 }
-#else
-static inline void iput_readcount(struct inode *inode)
-{
-	return;
-}
-static inline void iget_readcount(struct inode *inode)
-{
-	return;
-}
-#endif
 extern int do_pipe_flags(int *, int);
 extern struct file *create_read_pipe(struct file *f, int flags);
 extern struct file *create_write_pipe(int flags);
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 131eb1f..787c4cb 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -6,4 +6,4 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_iint.o ima_audit.o ima_readcount.o
+	 ima_policy.o ima_iint.o ima_audit.o
diff --git a/security/integrity/ima/ima_readcount.c b/security/integrity/ima/ima_readcount.c
deleted file mode 100644
index d139e2a9..0000000
--- a/security/integrity/ima/ima_readcount.c
+++ /dev/null
@@ -1,25 +0,0 @@
-/*
- * Copyright (C) 2010 IBM Corporation
- *
- * Authors:
- * Mimi Zohar <zohar@us.ibm.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation, version 2 of the
- * License.
- */
-#include <linux/module.h>
-#include <linux/spinlock.h>
-#include <linux/fs.h>
-
-void iput_readcount(struct inode *inode)
-{
-	spin_lock(&inode->i_lock);
-	if (unlikely((atomic_read(&inode->i_readcount) == 0)))
-		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
-		       inode->i_ino);
-	else
-		atomic_dec(&inode->i_readcount);
-	spin_unlock(&inode->i_lock);
-}
-- 
1.7.2.2

Re: [PATCH v1.1 2/5] IMA: define readcount functions

[Dave Chinner]

On Mon, Nov 01, 2010 at 03:45:36PM -0400, Mimi Zohar wrote:
> Define iget/iput_readcount() functions to be called from the VFS layer.

Can't say I like the function names. i_readcount_{inc,dec} seem more
appropriate, especially so they don't get confused with inode
reference counting...

Cheers,

Dave.

> +void iput_readcount(struct inode *inode)
> +{
> +	spin_lock(&inode->i_lock);
> +	if (unlikely((atomic_read(&inode->i_readcount) == 0)))
> +		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
> +		       inode->i_ino);
> +	else
> +		atomic_dec(&inode->i_readcount);
> +	spin_unlock(&inode->i_lock);
> +}

No need for the lock just to indicate an imbalance. You could just
use:

	if (atomic_dec_return(&inode->i_readcount) < 0) {
		.....
	}

Given this is an integrity subsystem, I suspect the correct thing to
do here is BUG(), not just issue an informational message that
something is wrong with the integrity tracking....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1.1 0/5] IMA: making i_readcount a first class inode citizen

[Eric Paris]

On Mon, 2010-11-01 at 15:45 -0400, Mimi Zohar wrote:
> Based on the previous posting discussion, i_readcount is now defined as
> atomic.
> 
> This patchset separates the incrementing/decrementing of the i_readcount,
> in the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with iget_readcount(). Its unclear whether this
> call to increment i_readcount should be made earlier, like i_writecount.
> 
> The patch ordering is a bit redundant in order to leave removing the ifdef
> around i_readcount until the last patch. The first four patches: redefines 
> i_readcount as atomic, defines iget/iput_readcount(), moves the IMA
> functionality in ima_counts_get() to ima_file_check(), and removes the IMA
> imbalance code, simplifying IMA. The last patch moves iput_readcount()
> to the fs directory and removes the ifdef around i_readcount, making
> i_readcount into a "first class inode citizen".
> 
> The generic_setlease code could then take advantage of i_readcount.

Hey Mimi, 

couple of comment and questions, can you help me understand what you
believe the three locks in question are currently protecting?  And
remember I already said I don't think they are quite right before you
started so try not to use that as your example   :)

inode->i_lock
inode->i_mutex
iint->mutex

I also question you finishing in patch 5/5 with:

+void iput_readcount(struct inode *inode)
+{
+       spin_lock(&inode->i_lock);
+       if (unlikely((atomic_read(&inode->i_readcount) == 0)))
+               printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
+                      inode->i_ino);
+       else
+               atomic_dec(&inode->i_readcount);
+       spin_unlock(&inode->i_lock);
+}

obviously I wonder what the locking is for, but really I question why we
need this as a conditional at all.  If we are really worried it should
be a WARN_ON() or BUG() but personally I wonder if we need it at all.
The VFS is by supposed to get stuff right.  All of the interesting
checks around IMA were mostly needed because IMA was an object that hung
off the side of the VFS and you couldn't be certain that all filesystems
were adhering to the calling conventions you thought were correct.
Since we've pretty much moved all of this into the VFS its about time we
stop wasting time wondering if our assumptions are correct.  These are
pretty hot paths and I'm all for cutting down the IMA overhead on them.
If we do that this function becomes:

BUG_ON(!atomic_read(&inode->i_readcount))
atomic_dec(&inode->i_readcont);

it also means that we don't need to set the i_readcount to 0 in
inode_init_always() from patch 3....

Re: [PATCH v1.1 0/5] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

On Mon, 2010-11-01 at 21:22 -0400, Eric Paris wrote:
> On Mon, 2010-11-01 at 15:45 -0400, Mimi Zohar wrote:
> > Based on the previous posting discussion, i_readcount is now defined as
> > atomic.
> > 
> > This patchset separates the incrementing/decrementing of the i_readcount,
> > in the VFS layer, from other IMA functionality, by replacing the current
> > ima_counts_get() call with iget_readcount(). Its unclear whether this
> > call to increment i_readcount should be made earlier, like i_writecount.
> > 
> > The patch ordering is a bit redundant in order to leave removing the ifdef
> > around i_readcount until the last patch. The first four patches: redefines 
> > i_readcount as atomic, defines iget/iput_readcount(), moves the IMA
> > functionality in ima_counts_get() to ima_file_check(), and removes the IMA
> > imbalance code, simplifying IMA. The last patch moves iput_readcount()
> > to the fs directory and removes the ifdef around i_readcount, making
> > i_readcount into a "first class inode citizen".
> > 
> > The generic_setlease code could then take advantage of i_readcount.
> 
> Hey Mimi, 
> 
> couple of comment and questions, can you help me understand what you
> believe the three locks in question are currently protecting?  And
> remember I already said I don't think they are quite right before you
> started so try not to use that as your example   :)
> 
> inode->i_lock

It shouldn't be necessary. As you originally said, 

> > My thought was that the IMA read/write checks should happen AFTER
> > the i_writecount and i_readcount counters were updated.  Thus even
> > if we raced with another task we can rest assured that the other
> > task would catch the situation we missed....

> inode->i_mutex

As the measurement policy is based on file metdata (eg, permissions,
xattrs), the mutex is taken to prevent the file metadata from changing,
while making the measurement decision.

> iint->mutex

Taken when accessing/modifying the iint structure.

> I also question you finishing in patch 5/5 with:
> 
> +void iput_readcount(struct inode *inode)
> +{
> +       spin_lock(&inode->i_lock);
> +       if (unlikely((atomic_read(&inode->i_readcount) == 0)))
> +               printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
> +                      inode->i_ino);
> +       else
> +               atomic_dec(&inode->i_readcount);
> +       spin_unlock(&inode->i_lock);
> +}
> 
> obviously I wonder what the locking is for, but really I question why we
> need this as a conditional at all.  If we are really worried it should
> be a WARN_ON() or BUG() but personally I wonder if we need it at all.
> The VFS is by supposed to get stuff right.  All of the interesting
> checks around IMA were mostly needed because IMA was an object that hung
> off the side of the VFS and you couldn't be certain that all filesystems
> were adhering to the calling conventions you thought were correct.
> Since we've pretty much moved all of this into the VFS its about time we
> stop wasting time wondering if our assumptions are correct.  These are
> pretty hot paths and I'm all for cutting down the IMA overhead on them.
> If we do that this function becomes:
> 
> BUG_ON(!atomic_read(&inode->i_readcount))
> atomic_dec(&inode->i_readcont);

yes, nice.

> it also means that we don't need to set the i_readcount to 0 in
> inode_init_always() from patch 3....

True, and init_once() sets the inode structure to 0.

thanks,

Mimi

Re: [PATCH v1.1 2/5] IMA: define readcount functions

[Mimi Zohar]

On Tue, 2010-11-02 at 11:45 +1100, Dave Chinner wrote:
> On Mon, Nov 01, 2010 at 03:45:36PM -0400, Mimi Zohar wrote:
> > Define iget/iput_readcount() functions to be called from the VFS layer.
> 
> Can't say I like the function names. i_readcount_{inc,dec} seem more
> appropriate, especially so they don't get confused with inode
> reference counting...
> 
> Cheers,
> 
> Dave.

Definitely better naming. thanks!

> > +void iput_readcount(struct inode *inode)
> > +{
> > +	spin_lock(&inode->i_lock);
> > +	if (unlikely((atomic_read(&inode->i_readcount) == 0)))
> > +		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
> > +		       inode->i_ino);
> > +	else
> > +		atomic_dec(&inode->i_readcount);
> > +	spin_unlock(&inode->i_lock);
> > +}
> 
> No need for the lock just to indicate an imbalance. You could just
> use:
> 
> 	if (atomic_dec_return(&inode->i_readcount) < 0) {
> 		.....
> 	}
> 
> Given this is an integrity subsystem, I suspect the correct thing to
> do here is BUG(), not just issue an informational message that
> something is wrong with the integrity tracking....
> 
> Cheers,
> 
> Dave.

Yes, as Eric explained, the testing is a remnant from IMA, when it
wasn't fully integrated in the kernel.

thanks,

Mimi