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

--

[ message continues ]

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 ...
[ message continues ]

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,


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
--

[ message continues ]

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

thanks,

Mimi

--

[ message continues ]

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 ...
[ message continues ]

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 ...
[ message continues ]

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 = ...
[ message continues ]

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)) == ...
[ message continues ]

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....

--

[ message continues ]

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



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

thanks,

Mimi

--

[ message continues ]