[PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Eric Paris]

The IMA code needs to store the number of tasks which have an open fd
granting permission to write a file even when IMA is not in use.  It needs
this information in order to be enabled at a later point in time without
losing it's integrity garantees.  At the moment that means we store a
little bit of data about every inode in a cache.  We use a radix tree key'd
on the inode's memory address.  Dave Chinner pointed out that a radix tree
is a terrible data structure for such a sparse key space.  This patch
switches to using an rbtree which should be more efficient.

Bug report from Dave:

 I just noticed that slabtop
was reportingi an awfully high usage of radix tree nodes:

 OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
4200331 2778082  66%    0.55K 144839       29   2317424K radix_tree_node
2321500 2060290  88%    1.00K  72581       32   2322592K xfs_inode
2235648 2069791  92%    0.12K  69864       32    279456K iint_cache

That is, 2.7M radix tree nodes are allocated, and the cache itself
is consuming 2.3GB of RAM. I know that the XFS inodei caches are
indexed by radix tree node, but for 2 million cached inodes that
would mean a density of 1 inode per radix tree node, which for a
system with 16M inodes in the filsystems is an impossibly low
density. The worst I've seen in a production system like kernel.org
is about 20-25% density, which would mean about 150−200k radix tree
nodes for that many inodes. So it's not the inode cache.

So I looked up what the iint_cache was. It appears to used for storing
per-inode IMA information, and uses a radix tree for indexing.
It uses the *address* of the struct inode as the indexing key. That
means the key space is extremely sparse - for XFS the struct inode
addresses are approximately 1000 bytes apart, which means the
closest the radix tree index keys get is ~1000. Which means
that there is a single entry per radix tree leaf node, so the radix
tree is using roughly 550 bytes for every 120byte structure being
cached. For the above example, it's probably wasting close to 1GB of
RAM....

Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    6 +-
 security/integrity/ima/ima_iint.c |  105 +++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 36 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3fbcd1d..7557791 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -100,6 +100,8 @@ static inline unsigned long ima_hash_key(u8 *digest)
 
 /* integrity data associated with an inode */
 struct ima_iint_cache {
+	struct rb_node rb_node; /* rooted in ima_iint_tree */
+	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
 	unsigned long flags;
 	u8 digest[IMA_DIGEST_SIZE];
@@ -108,7 +110,6 @@ struct ima_iint_cache {
 	long writecount;	/* measured files writecount */
 	long opencount;		/* opens reference count */
 	struct kref refcount;	/* ima_iint_cache reference count */
-	struct rcu_head rcu;
 };
 
 /* LIM API function definitions */
@@ -122,13 +123,12 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
 void ima_template_show(struct seq_file *m, void *e,
 		       enum ima_show_type show);
 
-/* radix tree calls to lookup, insert, delete
+/* rbtree tree calls to lookup, insert, delete
  * integrity data associated with an inode.
  */
 struct ima_iint_cache *ima_iint_insert(struct inode *inode);
 struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
 void iint_free(struct kref *kref);
-void iint_rcu_free(struct rcu_head *rcu);
 
 /* IMA policy related functions */
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index afba4ae..8395f0f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -12,21 +12,48 @@
  * File: ima_iint.c
  * 	- implements the IMA hooks: ima_inode_alloc, ima_inode_free
  *	- cache integrity information associated with an inode
- *	  using a radix tree.
+ *	  using a rbtree tree.
  */
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
-#include <linux/radix-tree.h>
+#include <linux/rbtree.h>
 #include "ima.h"
 
-RADIX_TREE(ima_iint_store, GFP_ATOMIC);
-DEFINE_SPINLOCK(ima_iint_lock);
+static struct rb_root ima_iint_tree = RB_ROOT;
+static DEFINE_SPINLOCK(ima_iint_lock);
 static struct kmem_cache *iint_cache __read_mostly;
 
 int iint_initialized = 0;
 
-/* ima_iint_find_get - return the iint associated with an inode
+/*
+ * __ima_iint_find - return the iint associated with an inode
+ */
+static struct ima_iint_cache *__ima_iint_find(struct inode *inode)
+{
+	struct ima_iint_cache *iint;
+	struct rb_node *n = ima_iint_tree.rb_node;
+
+	assert_spin_locked(&ima_iint_lock);
+
+	while (n) {
+		iint = rb_entry(n, struct ima_iint_cache, rb_node);
+
+		if (inode < iint->inode)
+			n = n->rb_left;
+		else if (inode > iint->inode)
+			n = n->rb_right;
+		else
+			break;
+	}
+	if (!n)
+		return NULL;
+
+	return iint;
+}
+
+/*
+ * ima_iint_find_get - return the iint associated with an inode
  *
  * ima_iint_find_get gets a reference to the iint. Caller must
  * remember to put the iint reference.
@@ -35,13 +62,12 @@ struct ima_iint_cache *ima_iint_find_get(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	rcu_read_lock();
-	iint = radix_tree_lookup(&ima_iint_store, (unsigned long)inode);
-	if (!iint)
-		goto out;
-	kref_get(&iint->refcount);
-out:
-	rcu_read_unlock();
+	spin_lock(&ima_iint_lock);
+	iint = __ima_iint_find(inode);
+	if (iint)
+		kref_get(&iint->refcount);
+	spin_unlock(&ima_iint_lock);
+
 	return iint;
 }
 
@@ -51,25 +77,43 @@ out:
  */
 int ima_inode_alloc(struct inode *inode)
 {
-	struct ima_iint_cache *iint = NULL;
-	int rc = 0;
+	struct rb_node **p;
+	struct rb_node *new_node, *parent = NULL;
+	struct ima_iint_cache *new_iint, *test_iint;
+	int rc;
 
-	iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
-	if (!iint)
+	new_iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
+	if (!new_iint)
 		return -ENOMEM;
 
-	rc = radix_tree_preload(GFP_NOFS);
-	if (rc < 0)
-		goto out;
+	new_iint->inode = inode;
+	new_node = &new_iint->rb_node;
 
 	spin_lock(&ima_iint_lock);
-	rc = radix_tree_insert(&ima_iint_store, (unsigned long)inode, iint);
+
+	p = &ima_iint_tree.rb_node;
+	while (*p) {
+		parent = *p;
+		test_iint = rb_entry(parent, struct ima_iint_cache, rb_node);
+
+		rc = -EEXIST;
+		if (inode < test_iint->inode)
+			p = &(*p)->rb_left;
+		else if (inode > test_iint->inode)
+			p = &(*p)->rb_right;
+		else
+			goto out_err;
+	}
+
+	rb_link_node(new_node, parent, p);
+	rb_insert_color(new_node, &ima_iint_tree);
+
 	spin_unlock(&ima_iint_lock);
-	radix_tree_preload_end();
-out:
-	if (rc < 0)
-		kmem_cache_free(iint_cache, iint);
 
+	return 0;
+out_err:
+	spin_unlock(&ima_iint_lock);
+	kref_put(&new_iint->refcount, iint_free);
 	return rc;
 }
 
@@ -99,13 +143,6 @@ void iint_free(struct kref *kref)
 	kmem_cache_free(iint_cache, iint);
 }
 
-void iint_rcu_free(struct rcu_head *rcu_head)
-{
-	struct ima_iint_cache *iint = container_of(rcu_head,
-						   struct ima_iint_cache, rcu);
-	kref_put(&iint->refcount, iint_free);
-}
-
 /**
  * ima_inode_free - called on security_inode_free
  * @inode: pointer to the inode
@@ -117,10 +154,12 @@ void ima_inode_free(struct inode *inode)
 	struct ima_iint_cache *iint;
 
 	spin_lock(&ima_iint_lock);
-	iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
+	iint = __ima_iint_find(inode);
+	if (iint)
+		rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
 	if (iint)
-		call_rcu(&iint->rcu, iint_rcu_free);
+		kref_put(&iint->refcount, iint_free);
 }
 
 static void init_once(void *foo)

[PATCH 02/11] IMA: drop the inode opencount since it isn't needed for operation

[Eric Paris]

The opencount was used to help debugging to make sure that everything which
created a struct file also correctly made the IMA calls.  Since we moved
all of that into the VFS this isn't as necessary.  We should be able to get
the same amount of debugging out of just the reader and write count.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    1 -
 security/integrity/ima/ima_iint.c |    6 ------
 security/integrity/ima/ima_main.c |   10 +++-------
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7557791..3d70108 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -108,7 +108,6 @@ struct ima_iint_cache {
 	struct mutex mutex;	/* protects: version, flags, digest */
 	long readcount;		/* measured files readcount */
 	long writecount;	/* measured files writecount */
-	long opencount;		/* opens reference count */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 8395f0f..8e64313 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -134,11 +134,6 @@ void iint_free(struct kref *kref)
 		       iint->writecount);
 		iint->writecount = 0;
 	}
-	if (iint->opencount != 0) {
-		printk(KERN_INFO "%s: opencount: %ld\n", __func__,
-		       iint->opencount);
-		iint->opencount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -172,7 +167,6 @@ static void init_once(void *foo)
 	mutex_init(&iint->mutex);
 	iint->readcount = 0;
 	iint->writecount = 0;
-	iint->opencount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e662b89..995bd1b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -122,7 +122,6 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 {
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	iint->opencount++;
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
 	if (mode & FMODE_WRITE)
@@ -181,7 +180,6 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	mode_t mode = file->f_mode;
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	iint->opencount--;
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount--;
 	if (mode & FMODE_WRITE) {
@@ -192,13 +190,11 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		}
 	}
 
-	if (((iint->opencount < 0) ||
-	     (iint->readcount < 0) ||
+	if (((iint->readcount < 0) ||
 	     (iint->writecount < 0)) &&
 	    !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld o:%ld)\n",
-		       __func__, iint->readcount, iint->writecount,
-		       iint->opencount);
+		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld)\n",
+		       __func__, iint->readcount, iint->writecount);
 		dump_stack();
 	}
 }

[PATCH 03/11] IMA: use unsigned int instead of long for counters

[Eric Paris]

Currently IMA uses 2 longs in struct inode.  To save space (and as it seems
impossible to overflow 32 bits) we switch these to unsigned int.  The
switch to unsigned does require slightly different checks for underflow,
but it isn't complex.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    4 ++--
 security/integrity/ima/ima_iint.c |    4 ++--
 security/integrity/ima/ima_main.c |   15 ++++++++++-----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3d70108..000d13a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -106,8 +106,8 @@ struct ima_iint_cache {
 	unsigned long flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	long readcount;		/* measured files readcount */
-	long writecount;	/* measured files writecount */
+	unsigned int readcount;	/* measured files readcount */
+	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 8e64313..db71a13 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -125,12 +125,12 @@ void iint_free(struct kref *kref)
 	iint->version = 0;
 	iint->flags = 0UL;
 	if (iint->readcount != 0) {
-		printk(KERN_INFO "%s: readcount: %ld\n", __func__,
+		printk(KERN_INFO "%s: readcount: %u\n", __func__,
 		       iint->readcount);
 		iint->readcount = 0;
 	}
 	if (iint->writecount != 0) {
-		printk(KERN_INFO "%s: writecount: %ld\n", __func__,
+		printk(KERN_INFO "%s: writecount: %u\n", __func__,
 		       iint->writecount);
 		iint->writecount = 0;
 	}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 995bd1b..5a1bf3d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -178,11 +178,18 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 			   struct file *file)
 {
 	mode_t mode = file->f_mode;
+	bool dump = false;
+
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
+		if (unlikely(iint->readcount == 0))
+			dump = true;
 		iint->readcount--;
+	}
 	if (mode & FMODE_WRITE) {
+		if (unlikely(iint->writecount == 0))
+			dump = true;
 		iint->writecount--;
 		if (iint->writecount == 0) {
 			if (iint->version != inode->i_version)
@@ -190,10 +197,8 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		}
 	}
 
-	if (((iint->readcount < 0) ||
-	     (iint->writecount < 0)) &&
-	    !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld)\n",
+	if (dump && !ima_limit_imbalance(file)) {
+		printk(KERN_INFO "%s: open/free imbalance (r:%u w:%u)\n",
 		       __func__, iint->readcount, iint->writecount);
 		dump_stack();
 	}

[PATCH 04/11] IMA: convert internal flags from long to char

[Eric Paris]

The IMA flags is an unsigned long but there is only 1 flag defined.  Lets
save a little space and make it a char.  This packs nicely next to the array
of u8's.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 000d13a..f7af011 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -96,14 +96,14 @@ static inline unsigned long ima_hash_key(u8 *digest)
 }
 
 /* iint cache flags */
-#define IMA_MEASURED		1
+#define IMA_MEASURED		0x01
 
 /* integrity data associated with an inode */
 struct ima_iint_cache {
 	struct rb_node rb_node; /* rooted in ima_iint_tree */
 	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
-	unsigned long flags;
+	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
 	unsigned int readcount;	/* measured files readcount */

[PATCH 05/11] IMA: use inode->i_lock to protect read and write counters

[Eric Paris]

Currently IMA used the iint->mutex to protect the i_readcount and
i_writecount.  This patch uses the inode->i_lock since we are going to
start using in inode objects and that is the most appropriate lock.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    1 +
 security/integrity/ima/ima_main.c |   57 +++++++++++++++----------------------
 2 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f7af011..80aca3d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -106,6 +106,7 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
+	/* protected by inode->i_lock */
 	unsigned int readcount;	/* measured files readcount */
 	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5a1bf3d..2f9b5d5 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -85,42 +85,12 @@ out:
 	return found;
 }
 
-/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
- *
- * When opening a file for read, if the file is already open for write,
- * the file could change, resulting in a file measurement error.
- *
- * Opening a file for write, if the file is already open for read, results
- * in a time of measure, time of use (ToMToU) error.
- *
- * In either case invalidate the PCR.
- */
-enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
-static void ima_read_write_check(enum iint_pcr_error error,
-				 struct ima_iint_cache *iint,
-				 struct inode *inode,
-				 const unsigned char *filename)
-{
-	switch (error) {
-	case TOMTOU:
-		if (iint->readcount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "ToMToU");
-		break;
-	case OPEN_WRITERS:
-		if (iint->writecount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "open_writers");
-		break;
-	}
-}
-
 /*
  * Update the counts given an fmode_t
  */
 static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 {
-	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&iint->inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
@@ -146,6 +116,7 @@ void ima_counts_get(struct file *file)
 	fmode_t mode = file->f_mode;
 	struct ima_iint_cache *iint;
 	int rc;
+	bool send_tomtou = false, send_writers = false;
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
@@ -153,22 +124,35 @@ void ima_counts_get(struct file *file)
 	if (!iint)
 		return;
 	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
 	if (!ima_initialized)
 		goto out;
+
 	rc = ima_must_measure(iint, inode, MAY_READ, FILE_CHECK);
 	if (rc < 0)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		ima_read_write_check(TOMTOU, iint, inode, dentry->d_name.name);
+		if (iint->readcount)
+			send_tomtou = true;
 		goto out;
 	}
-	ima_read_write_check(OPEN_WRITERS, iint, inode, dentry->d_name.name);
+
+	if (atomic_read(&inode->i_writecount) > 0)
+		send_writers = true;
 out:
 	ima_inc_counts(iint, file->f_mode);
+	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
-
 	kref_put(&iint->refcount, iint_free);
+
+	if (send_tomtou)
+		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
+				  "ToMToU");
+	if (send_writers)
+		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
+				  "open_writers");
 }
 
 /*
@@ -181,6 +165,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	bool dump = false;
 
 	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
 		if (unlikely(iint->readcount == 0))
@@ -223,7 +208,11 @@ void ima_file_free(struct file *file)
 		return;
 
 	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
 	ima_dec_counts(iint, inode, file);
+
+	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
 	kref_put(&iint->refcount, iint_free);
 }

[PATCH 06/11] IMA: use i_writecount rather than a private counter

[Eric Paris]

IMA tracks the number of struct files which are holding a given inode
readonly and the number which are holding the inode write or r/w.  It needs
this information so when a new reader or writer comes in it can tell if
this new file will be able to invalidate results it already made about
existing files.

aka if a task is holding a struct file open RO, IMA measured the file and
recorded those measurements and then a task opens the file RW IMA needs to
note in the logs that the old measurement may not be correct.  It's called
a "Time of Measure Time of Use" (ToMToU) issue.  The same is true is a RO
file is opened to an inode which has an open writer.  We cannot, with any
validity, measure the file in question since it could be changing.

This patch attempts to use the i_writecount field to track writers.  The
i_writecount field actually embeds more information in it's value than IMA
needs but it should work for our purposes and allow us to shrink the struct
inode even more.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    1 -
 security/integrity/ima/ima_iint.c |    6 ------
 security/integrity/ima/ima_main.c |   16 ++++++----------
 3 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 80aca3d..b546b90 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -108,7 +108,6 @@ struct ima_iint_cache {
 	struct mutex mutex;	/* protects: version, flags, digest */
 	/* protected by inode->i_lock */
 	unsigned int readcount;	/* measured files readcount */
-	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index db71a13..e68891f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -129,11 +129,6 @@ void iint_free(struct kref *kref)
 		       iint->readcount);
 		iint->readcount = 0;
 	}
-	if (iint->writecount != 0) {
-		printk(KERN_INFO "%s: writecount: %u\n", __func__,
-		       iint->writecount);
-		iint->writecount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -166,7 +161,6 @@ static void init_once(void *foo)
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
 	iint->readcount = 0;
-	iint->writecount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2f9b5d5..24660bf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -94,8 +94,6 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
-	if (mode & FMODE_WRITE)
-		iint->writecount++;
 }
 
 /*
@@ -173,18 +171,16 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		iint->readcount--;
 	}
 	if (mode & FMODE_WRITE) {
-		if (unlikely(iint->writecount == 0))
+		if (atomic_read(&inode->i_writecount) <= 0)
 			dump = true;
-		iint->writecount--;
-		if (iint->writecount == 0) {
-			if (iint->version != inode->i_version)
-				iint->flags &= ~IMA_MEASURED;
-		}
+		if (atomic_read(&inode->i_writecount) == 1 &&
+		    iint->version != inode->i_version)
+			iint->flags &= ~IMA_MEASURED;
 	}
 
 	if (dump && !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%u w:%u)\n",
-		       __func__, iint->readcount, iint->writecount);
+		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
+		       __func__, iint->readcount);
 		dump_stack();
 	}
 }

[PATCH 07/11] IMA: move read counter into struct inode

[Eric Paris]

IMA currently allocated an inode integrity structure for every inode in
core.  This stucture is about 120 bytes long.  Most files however
(especially on a system which doesn't make use of IMA) will never need any
of this space.  The problem is that if IMA is enabled we need to know
information about the number of readers and the number of writers for every
inode on the box.  At the moment we collect that information in the per
inode iint structure and waste the rest of the space.  This patch moves those
counters into the struct inode so we can eventually stop allocating an IMA
integrity structure except when absolutely needed.

This patch does the minimum needed to move the location of the data.  Further
cleanups, especially the location of counter updates, may still be possible.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 fs/inode.c                        |    1 +
 include/linux/fs.h                |    4 ++++
 security/integrity/ima/ima.h      |    3 +--
 security/integrity/ima/ima_api.c  |    2 +-
 security/integrity/ima/ima_iint.c |   11 +++++------
 security/integrity/ima/ima_main.c |   35 ++++++++++-------------------------
 6 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 8646433..56d909d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -24,6 +24,7 @@
 #include <linux/mount.h>
 #include <linux/async.h>
 #include <linux/posix_acl.h>
+#include <linux/ima.h>
 
 /*
  * This is needed for the following functions:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5fb4dfd..cb9dbd0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -772,6 +772,10 @@ struct inode {
 
 	unsigned int		i_flags;
 
+#if CONFIG_IMA
+	/* protected by i_lock */
+	unsigned int		i_readcount; /* struct files open RO */
+#endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
 	void			*i_security;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b546b90..27849e1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -70,6 +70,7 @@ int ima_init(void);
 void ima_cleanup(void);
 int ima_fs_init(void);
 void ima_fs_cleanup(void);
+int ima_inode_alloc(struct inode *inode);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode);
 int ima_calc_hash(struct file *file, char *digest);
@@ -106,8 +107,6 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	/* protected by inode->i_lock */
-	unsigned int readcount;	/* measured files readcount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 52015d0..d3963de 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -116,7 +116,7 @@ int ima_must_measure(struct ima_iint_cache *iint, struct inode *inode,
 {
 	int must_measure;
 
-	if (iint->flags & IMA_MEASURED)
+	if (iint && iint->flags & IMA_MEASURED)
 		return 1;
 
 	must_measure = ima_match_policy(inode, function, mask);
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index e68891f..0936a71 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -124,11 +124,6 @@ void iint_free(struct kref *kref)
 						   refcount);
 	iint->version = 0;
 	iint->flags = 0UL;
-	if (iint->readcount != 0) {
-		printk(KERN_INFO "%s: readcount: %u\n", __func__,
-		       iint->readcount);
-		iint->readcount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -143,6 +138,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);
+
+	inode->i_readcount = 0;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
 	if (iint)
@@ -160,7 +160,6 @@ static void init_once(void *foo)
 	iint->version = 0;
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
-	iint->readcount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 24660bf..2a77b14 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -86,17 +86,6 @@ out:
 }
 
 /*
- * Update the counts given an fmode_t
- */
-static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
-{
-	assert_spin_locked(&iint->inode->i_lock);
-
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		iint->readcount++;
-}
-
-/*
  * ima_counts_get - increment file counts
  *
  * Maintain read/write counters for all files, but only
@@ -112,27 +101,23 @@ void ima_counts_get(struct file *file)
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
 	fmode_t mode = file->f_mode;
-	struct ima_iint_cache *iint;
 	int rc;
 	bool send_tomtou = false, send_writers = false;
 
-	if (!iint_initialized || !S_ISREG(inode->i_mode))
+	if (!S_ISREG(inode->i_mode))
 		return;
-	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return;
-	mutex_lock(&iint->mutex);
+
 	spin_lock(&inode->i_lock);
 
 	if (!ima_initialized)
 		goto out;
 
-	rc = ima_must_measure(iint, inode, MAY_READ, FILE_CHECK);
+	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
 	if (rc < 0)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		if (iint->readcount)
+		if (inode->i_readcount)
 			send_tomtou = true;
 		goto out;
 	}
@@ -140,10 +125,10 @@ void ima_counts_get(struct file *file)
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
-	ima_inc_counts(iint, file->f_mode);
+	/* remember the vfs deals with i_writecount */
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		inode->i_readcount++;
 	spin_unlock(&inode->i_lock);
-	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 
 	if (send_tomtou)
 		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -166,9 +151,9 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(iint->readcount == 0))
+		if (unlikely(inode->i_readcount == 0))
 			dump = true;
-		iint->readcount--;
+		inode->i_readcount--;
 	}
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_writecount) <= 0)
@@ -180,7 +165,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 
 	if (dump && !ima_limit_imbalance(file)) {
 		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-		       __func__, iint->readcount);
+		       __func__, inode->i_readcount);
 		dump_stack();
 	}
 }

[PATCH 08/11] IMA: only allocate iint when needed

[Eric Paris]

IMA always allocates an integrity structure to hold information about every
inode, but only needed this structure to tract the number of readers and
writers currently accessing a given inode.  Since that information was moved
into struct inode instead of the integrity struct this patch stops allocating
the integrity stucture until it is needed.  Thus greatly reducing memory usage.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima_main.c |   94 +++++++++++++++++++++++++------------
 security/security.c               |   10 ----
 2 files changed, 65 insertions(+), 39 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2a77b14..5e3229c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -141,33 +141,62 @@ out:
 /*
  * Decrement ima counts
  */
-static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
-			   struct file *file)
+static void ima_dec_counts(struct inode *inode, struct file *file)
 {
 	mode_t mode = file->f_mode;
-	bool dump = false;
 
-	BUG_ON(!mutex_is_locked(&iint->mutex));
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(inode->i_readcount == 0))
-			dump = true;
+		if (unlikely(inode->i_readcount == 0)) {
+			if (!ima_limit_imbalance(file)) {
+				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
+				       __func__, inode->i_readcount);
+				dump_stack();
+			}
+			return;
+		}
 		inode->i_readcount--;
 	}
-	if (mode & FMODE_WRITE) {
-		if (atomic_read(&inode->i_writecount) <= 0)
-			dump = true;
-		if (atomic_read(&inode->i_writecount) == 1 &&
-		    iint->version != inode->i_version)
-			iint->flags &= ~IMA_MEASURED;
-	}
+}
 
-	if (dump && !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-		       __func__, inode->i_readcount);
-		dump_stack();
-	}
+static void ima_check_last_writer(struct ima_iint_cache *iint,
+				  struct inode *inode,
+				  struct file *file)
+{
+	mode_t mode = file->f_mode;
+
+	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&inode->i_lock);
+
+	if (mode & FMODE_WRITE &&
+	    atomic_read(&inode->i_writecount) == 1 &&
+	    iint->version != inode->i_version)
+		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);
+
+	kref_put(&iint->refcount, iint_free);
+}
+
+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);
 }
 
 /**
@@ -175,7 +204,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
  * @file: pointer to file structure being freed
  *
  * Flag files that changed, based on i_version;
- * and decrement the iint readcount/writecount.
+ * and decrement the i_readcount.
  */
 void ima_file_free(struct file *file)
 {
@@ -185,17 +214,12 @@ void ima_file_free(struct file *file)
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
 	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return;
 
-	mutex_lock(&iint->mutex);
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(iint, inode, file);
+	if (iint)
+		ima_file_free_iint(iint, inode, file);
+	else
+		ima_file_free_noiint(inode, file);
 
-	spin_unlock(&inode->i_lock);
-	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
@@ -207,11 +231,21 @@ static int process_measurement(struct file *file, const unsigned char *filename,
 
 	if (!ima_initialized || !S_ISREG(inode->i_mode))
 		return 0;
+
+	rc = ima_must_measure(NULL, inode, mask, function);
+	if (rc != 0)
+		return rc;
+retry:
 	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return -ENOMEM;
+	if (!iint) {
+		rc = ima_inode_alloc(inode);
+		if (!rc || rc == -EEXIST)
+			goto retry;
+		return rc;
+	}
 
 	mutex_lock(&iint->mutex);
+
 	rc = ima_must_measure(iint, inode, mask, function);
 	if (rc != 0)
 		goto out;
diff --git a/security/security.c b/security/security.c
index ad5ca29..259d3ad 100644
--- a/security/security.c
+++ b/security/security.c
@@ -325,16 +325,8 @@ EXPORT_SYMBOL(security_sb_parse_opts_str);
 
 int security_inode_alloc(struct inode *inode)
 {
-	int ret;
-
 	inode->i_security = NULL;
-	ret =  security_ops->inode_alloc_security(inode);
-	if (ret)
-		return ret;
-	ret = ima_inode_alloc(inode);
-	if (ret)
-		security_inode_free(inode);
-	return ret;
+	return security_ops->inode_alloc_security(inode);
 }
 
 void security_inode_free(struct inode *inode)

[PATCH 09/11] IMA: drop refcnt from ima_iint_cache since it isn't needed

[Eric Paris]

Since finding a struct ima_iint_cache requires a valid struct inode, and the
struct ima_iint_cache is supposed to have the same lifetime as a struct inode
(technically they die together but don't need to be created at the same time)
we don't have to worry about the ima_iint_cache outliving or dieing before the
inode.  So the refcnt isn't useful.  Just get rid of it and free the structure
when the inode is freed.

Signed-off-by: Eric Paris <eapris@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima.h      |    4 +---
 security/integrity/ima/ima_iint.c |   38 ++++++++++++++++---------------------
 security/integrity/ima/ima_main.c |    7 ++-----
 3 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 27849e1..ac79032 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -107,7 +107,6 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
 /* LIM API function definitions */
@@ -125,8 +124,7 @@ void ima_template_show(struct seq_file *m, void *e,
  * integrity data associated with an inode.
  */
 struct ima_iint_cache *ima_iint_insert(struct inode *inode);
-struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
-void iint_free(struct kref *kref);
+struct ima_iint_cache *ima_iint_find(struct inode *inode);
 
 /* IMA policy related functions */
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 0936a71..969a1c1 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -53,24 +53,26 @@ static struct ima_iint_cache *__ima_iint_find(struct inode *inode)
 }
 
 /*
- * ima_iint_find_get - return the iint associated with an inode
- *
- * ima_iint_find_get gets a reference to the iint. Caller must
- * remember to put the iint reference.
+ * ima_iint_find - return the iint associated with an inode
  */
-struct ima_iint_cache *ima_iint_find_get(struct inode *inode)
+struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
-	if (iint)
-		kref_get(&iint->refcount);
 	spin_unlock(&ima_iint_lock);
 
 	return iint;
 }
 
+static void iint_free(struct ima_iint_cache *iint)
+{
+	iint->version = 0;
+	iint->flags = 0UL;
+	kmem_cache_free(iint_cache, iint);
+}
+
 /**
  * ima_inode_alloc - allocate an iint associated with an inode
  * @inode: pointer to the inode
@@ -113,19 +115,9 @@ int ima_inode_alloc(struct inode *inode)
 	return 0;
 out_err:
 	spin_unlock(&ima_iint_lock);
-	kref_put(&new_iint->refcount, iint_free);
-	return rc;
-}
+	iint_free(new_iint);
 
-/* iint_free - called when the iint refcount goes to zero */
-void iint_free(struct kref *kref)
-{
-	struct ima_iint_cache *iint = container_of(kref, struct ima_iint_cache,
-						   refcount);
-	iint->version = 0;
-	iint->flags = 0UL;
-	kref_init(&iint->refcount);
-	kmem_cache_free(iint_cache, iint);
+	return rc;
 }
 
 /**
@@ -148,8 +140,11 @@ void ima_inode_free(struct inode *inode)
 	if (iint)
 		rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
-	if (iint)
-		kref_put(&iint->refcount, iint_free);
+
+	if (!iint)
+		return;
+
+	iint_free(iint);
 }
 
 static void init_once(void *foo)
@@ -160,7 +155,6 @@ static void init_once(void *foo)
 	iint->version = 0;
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
-	kref_init(&iint->refcount);
 }
 
 static int __init ima_iintcache_init(void)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5e3229c..1dccafe 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -186,8 +186,6 @@ static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
 
 	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
-
-	kref_put(&iint->refcount, iint_free);
 }
 
 static void ima_file_free_noiint(struct inode *inode, struct file *file)
@@ -213,7 +211,7 @@ void ima_file_free(struct file *file)
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
-	iint = ima_iint_find_get(inode);
+	iint = ima_iint_find(inode);
 
 	if (iint)
 		ima_file_free_iint(iint, inode, file);
@@ -236,7 +234,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,
 	if (rc != 0)
 		return rc;
 retry:
-	iint = ima_iint_find_get(inode);
+	iint = ima_iint_find(inode);
 	if (!iint) {
 		rc = ima_inode_alloc(inode);
 		if (!rc || rc == -EEXIST)
@@ -255,7 +253,6 @@ retry:
 		ima_store_measurement(iint, file, filename);
 out:
 	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 	return rc;
 }
 

[PATCH 10/11] IMA: explicit IMA i_flag to remove global lock on inode_delete

[Eric Paris]

Currently for every removed inode IMA must take a global lock and search
the IMA rbtree looking for an associated integrity structure.  Instead we
explicitly mark an inode when we add an integrity structure so we only have
to take the global lock and do the removal if it exists.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 include/linux/fs.h                |    2 ++
 security/integrity/ima/ima_iint.c |   16 +++++++++++-----
 security/integrity/ima/ima_main.c |    1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index cb9dbd0..b228b3a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -231,6 +231,7 @@ struct inodes_stat_t {
 #define S_NOCMTIME	128	/* Do not update file c/mtime */
 #define S_SWAPFILE	256	/* Do not truncate: swapon got its bmaps */
 #define S_PRIVATE	512	/* Inode is fs-internal */
+#define S_IMA		1024	/* Inode has an associated IMA struct */
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system
@@ -265,6 +266,7 @@ struct inodes_stat_t {
 #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
 #define IS_SWAPFILE(inode)	((inode)->i_flags & S_SWAPFILE)
 #define IS_PRIVATE(inode)	((inode)->i_flags & S_PRIVATE)
+#define IS_IMA(inode)		((inode)->i_flags & S_IMA)
 
 /* the read-only stuff doesn't really belong here, but any other place is
    probably as bad and I don't want to create yet another include file. */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 969a1c1..c442e47 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -59,6 +59,9 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
+	if (!IS_IMA(inode))
+		return NULL;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
 	spin_unlock(&ima_iint_lock);
@@ -91,6 +94,7 @@ int ima_inode_alloc(struct inode *inode)
 	new_iint->inode = inode;
 	new_node = &new_iint->rb_node;
 
+	mutex_lock(&inode->i_mutex); /* i_flags */
 	spin_lock(&ima_iint_lock);
 
 	p = &ima_iint_tree.rb_node;
@@ -107,14 +111,17 @@ int ima_inode_alloc(struct inode *inode)
 			goto out_err;
 	}
 
+	inode->i_flags |= S_IMA;
 	rb_link_node(new_node, parent, p);
 	rb_insert_color(new_node, &ima_iint_tree);
 
 	spin_unlock(&ima_iint_lock);
+	mutex_unlock(&inode->i_mutex); /* i_flags */
 
 	return 0;
 out_err:
 	spin_unlock(&ima_iint_lock);
+	mutex_unlock(&inode->i_mutex); /* i_flags */
 	iint_free(new_iint);
 
 	return rc;
@@ -135,15 +142,14 @@ void ima_inode_free(struct inode *inode)
 
 	inode->i_readcount = 0;
 
+	if (!IS_IMA(inode))
+		return;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
-	if (iint)
-		rb_erase(&iint->rb_node, &ima_iint_tree);
+	rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
 
-	if (!iint)
-		return;
-
 	iint_free(iint);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1dccafe..60dd615 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -211,6 +211,7 @@ void ima_file_free(struct file *file)
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
+
 	iint = ima_iint_find(inode);
 
 	if (iint)

[PATCH 11/11] IMA: fix the ToMToU logic

[Eric Paris]

Current logic looks like this:

        rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
        if (rc < 0)
                goto out;

        if (mode & FMODE_WRITE) {
                if (inode->i_readcount)
                        send_tomtou = true;
                goto out;
        }

        if (atomic_read(&inode->i_writecount) > 0)
                send_writers = true;

Lets assume we have a policy which states that all files opened for read by
root must be measured.

Lets assume the file has permissions 777.

Lets assume that root has the given file open for read.

Lets assume that a non-root process opens the file write.

The non-root process will get to ima_counts_get() and will check the
ima_must_measure().  Since it is not supposed to measure it will goto out.

We should check the i_readcount no matter what since we might be causing a
ToMToU voilation!

This is close to correct , but still not quite perfect.  The situation could have
been that root, which was interested in the mesurement opened and closed the
file and another process which is not interested in the measurement is the one
holding the i_readcount ATM.  This is just overly strict on ToMToU violations,
which is better than not strict enough...

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/integrity/ima/ima_main.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 60dd615..203de97 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -112,22 +112,23 @@ void ima_counts_get(struct file *file)
 	if (!ima_initialized)
 		goto out;
 
-	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
-	if (rc < 0)
-		goto out;
-
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount)
+		if (inode->i_readcount && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
 
+	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
+	if (rc < 0)
+		goto out;
+
 	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)
 		inode->i_readcount++;
+
 	spin_unlock(&inode->i_lock);
 
 	if (send_tomtou)

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[John Stoffel]

>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

Eric> The IMA code needs to store the number of tasks which have an
Eric> open fd granting permission to write a file even when IMA is not
Eric> in use.  It needs this information in order to be enabled at a
Eric> later point in time without losing it's integrity garantees.

This sounds completely wrong to me.  If I disable IMA (but have the
sucker compiled in due to a vendor...) I don't want *any* overhead,
and this is speaking using my SysAdmin hat for people who do EDA
design work, and having fast systems is key for us.  IMA is NOT.  We
disable SELINUX too, because of the overhead and the maintenance
nightmare.

If IMA isn't enabled right from the get-go, how can you ensure
integrity?  And how can you ensure integrity if root is compromised?
If root can disable IMA, screw around with a file, then turn on IMA
again without the change being guarrentteed to be noticed (and not
because the attacked didn't do the attack perfectly!) what's the use?

How does this help any?  

To quote from:
http://domino.research.ibm.com/comm/research_people.nsf/pages/sailer.ima.html

    What IMA is not

    IMA is not controlling your system. IMA is non-intrusive and is best
    described as an independent observer collecting integrity information
    of loaded code or sensitive application files on demand. Consequently,
    IMA does not prevent a system from illegal behavior that might
    compromise the system including the integrity measurement architecture
    itself. Recognizing the danger of being by-passed, IMA simply
    invalidates its own measurement list by invalidating the TPM integrity
    aggregate and thereby rendering the evidence useless (non-verifiable)
    until it is reset during the next system reboot. For example, if
    applications write directly to a device (/dev/hda, /dev/sda) or kernel
    memory (/dev/kmem), then IMA invalidates the TPM aggregate.

    IMA is not a Digital Rights Management tool either. IMA collects
    evidence on the local system, which can be used for many purposes but
    whose release is fully controlled by the local system. It is getting
    harder to lie about what you are running when you use IMA; no doubt
    about it. However, if system security is going to be reality, systems
    that lie at will seem not a convincing alternative in a distributed
    environment where the weakest link determines the security of the
    distributed service. The price to pay is that properties must be
    established securely and that the balance between use and abuse of
    knowledge about such properties as well as the validity of requiring
    such evidence (e.g., before connecting a system to a video-on-demand
    service) must be controlled by rules that are enforced the same way
    they are in other areas of our daily life (most likely by laws).


So basically, IMA is super duper tripwire with the ability to allow
some remote system to come in and ask for your Hashes.  And if you
screw around with it, it immediately disables itself.  

Which seems to fly in the face of your claim that it needs to be able
to re-enable itself by tracking open inodes even when disabled.

Eric>   At the moment that means we store a little bit of data about
Eric> every inode in a cache.  We use a radix tree key'd on the
Eric> inode's memory address.  Dave Chinner pointed out that a radix
Eric> tree is a terrible data structure for such a sparse key space.
Eric> This patch switches to using an rbtree which should be more
Eric> efficient.

As the number of inodes goes up (say during a backup which reads
them...) won't the size of this cache go up as well, even when IMA is
disabled?  Why is this overhead even needed?  

This should all just be ripped out.

John

Eric> Bug report from Dave:

Eric>  I just noticed that slabtop
Eric> was reportingi an awfully high usage of radix tree nodes:

Eric>  OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
Eric> 4200331 2778082  66%    0.55K 144839       29   2317424K radix_tree_node
Eric> 2321500 2060290  88%    1.00K  72581       32   2322592K xfs_inode
Eric> 2235648 2069791  92%    0.12K  69864       32    279456K iint_cache

Eric> That is, 2.7M radix tree nodes are allocated, and the cache itself
Eric> is consuming 2.3GB of RAM. I know that the XFS inodei caches are
Eric> indexed by radix tree node, but for 2 million cached inodes that
Eric> would mean a density of 1 inode per radix tree node, which for a
Eric> system with 16M inodes in the filsystems is an impossibly low
Eric> density. The worst I've seen in a production system like kernel.org
Eric> is about 20-25% density, which would mean about 150−200k radix tree
Eric> nodes for that many inodes. So it's not the inode cache.

Eric> So I looked up what the iint_cache was. It appears to used for storing
Eric> per-inode IMA information, and uses a radix tree for indexing.
Eric> It uses the *address* of the struct inode as the indexing key. That
Eric> means the key space is extremely sparse - for XFS the struct inode
Eric> addresses are approximately 1000 bytes apart, which means the
Eric> closest the radix tree index keys get is ~1000. Which means
Eric> that there is a single entry per radix tree leaf node, so the radix
Eric> tree is using roughly 550 bytes for every 120byte structure being
Eric> cached. For the above example, it's probably wasting close to 1GB of
Eric> RAM....

Eric> Reported-by: Dave Chinner <david@fromorbit.com>
Eric> Signed-off-by: Eric Paris <eparis@redhat.com>
Eric> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Eric> ---

Eric>  security/integrity/ima/ima.h      |    6 +-
Eric>  security/integrity/ima/ima_iint.c |  105 +++++++++++++++++++++++++------------
Eric>  2 files changed, 75 insertions(+), 36 deletions(-)

Eric> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
Eric> index 3fbcd1d..7557791 100644
Eric> --- a/security/integrity/ima/ima.h
Eric> +++ b/security/integrity/ima/ima.h
Eric> @@ -100,6 +100,8 @@ static inline unsigned long ima_hash_key(u8 *digest)
 
Eric>  /* integrity data associated with an inode */
Eric>  struct ima_iint_cache {
Eric> +	struct rb_node rb_node; /* rooted in ima_iint_tree */
Eric> +	struct inode *inode;	/* back pointer to inode in question */
Eric>  	u64 version;		/* track inode changes */
Eric>  	unsigned long flags;
Eric>  	u8 digest[IMA_DIGEST_SIZE];
Eric> @@ -108,7 +110,6 @@ struct ima_iint_cache {
Eric>  	long writecount;	/* measured files writecount */
Eric>  	long opencount;		/* opens reference count */
Eric>  	struct kref refcount;	/* ima_iint_cache reference count */
Eric> -	struct rcu_head rcu;
Eric>  };
 
Eric>  /* LIM API function definitions */
Eric> @@ -122,13 +123,12 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
Eric>  void ima_template_show(struct seq_file *m, void *e,
Eric>  		       enum ima_show_type show);
 
Eric> -/* radix tree calls to lookup, insert, delete
Eric> +/* rbtree tree calls to lookup, insert, delete
Eric>   * integrity data associated with an inode.
Eric>   */
Eric>  struct ima_iint_cache *ima_iint_insert(struct inode *inode);
Eric>  struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
Eric>  void iint_free(struct kref *kref);
Eric> -void iint_rcu_free(struct rcu_head *rcu);
 
Eric>  /* IMA policy related functions */
Eric>  enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
Eric> diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
Eric> index afba4ae..8395f0f 100644
Eric> --- a/security/integrity/ima/ima_iint.c
Eric> +++ b/security/integrity/ima/ima_iint.c
Eric> @@ -12,21 +12,48 @@
Eric>   * File: ima_iint.c
Eric>   * 	- implements the IMA hooks: ima_inode_alloc, ima_inode_free
Eric>   *	- cache integrity information associated with an inode
Eric> - *	  using a radix tree.
Eric> + *	  using a rbtree tree.
Eric>   */
Eric>  #include <linux/slab.h>
Eric>  #include <linux/module.h>
Eric>  #include <linux/spinlock.h>
Eric> -#include <linux/radix-tree.h>
Eric> +#include <linux/rbtree.h>
Eric>  #include "ima.h"
 
Eric> -RADIX_TREE(ima_iint_store, GFP_ATOMIC);
Eric> -DEFINE_SPINLOCK(ima_iint_lock);
Eric> +static struct rb_root ima_iint_tree = RB_ROOT;
Eric> +static DEFINE_SPINLOCK(ima_iint_lock);
Eric>  static struct kmem_cache *iint_cache __read_mostly;
 
Eric>  int iint_initialized = 0;
 
Eric> -/* ima_iint_find_get - return the iint associated with an inode
Eric> +/*
Eric> + * __ima_iint_find - return the iint associated with an inode
Eric> + */
Eric> +static struct ima_iint_cache *__ima_iint_find(struct inode *inode)
Eric> +{
Eric> +	struct ima_iint_cache *iint;
Eric> +	struct rb_node *n = ima_iint_tree.rb_node;
Eric> +
Eric> +	assert_spin_locked(&ima_iint_lock);
Eric> +
Eric> +	while (n) {
Eric> +		iint = rb_entry(n, struct ima_iint_cache, rb_node);
Eric> +
Eric> +		if (inode < iint->inode)
Eric> +			n = n->rb_left;
Eric> +		else if (inode > iint->inode)
Eric> +			n = n->rb_right;
Eric> +		else
Eric> +			break;
Eric> +	}
Eric> +	if (!n)
Eric> +		return NULL;
Eric> +
Eric> +	return iint;
Eric> +}
Eric> +
Eric> +/*
Eric> + * ima_iint_find_get - return the iint associated with an inode
Eric>   *
Eric>   * ima_iint_find_get gets a reference to the iint. Caller must
Eric>   * remember to put the iint reference.
Eric> @@ -35,13 +62,12 @@ struct ima_iint_cache *ima_iint_find_get(struct inode *inode)
Eric>  {
Eric>  	struct ima_iint_cache *iint;
 
Eric> -	rcu_read_lock();
Eric> -	iint = radix_tree_lookup(&ima_iint_store, (unsigned long)inode);
Eric> -	if (!iint)
Eric> -		goto out;
Eric> -	kref_get(&iint->refcount);
Eric> -out:
Eric> -	rcu_read_unlock();
Eric> +	spin_lock(&ima_iint_lock);
Eric> +	iint = __ima_iint_find(inode);
Eric> +	if (iint)
Eric> +		kref_get(&iint->refcount);
Eric> +	spin_unlock(&ima_iint_lock);
Eric> +
Eric>  	return iint;
Eric>  }
 
Eric> @@ -51,25 +77,43 @@ out:
Eric>   */
Eric>  int ima_inode_alloc(struct inode *inode)
Eric>  {
Eric> -	struct ima_iint_cache *iint = NULL;
Eric> -	int rc = 0;
Eric> +	struct rb_node **p;
Eric> +	struct rb_node *new_node, *parent = NULL;
Eric> +	struct ima_iint_cache *new_iint, *test_iint;
Eric> +	int rc;
 
Eric> -	iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
Eric> -	if (!iint)
Eric> +	new_iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
Eric> +	if (!new_iint)
Eric>  		return -ENOMEM;
 
Eric> -	rc = radix_tree_preload(GFP_NOFS);
Eric> -	if (rc < 0)
Eric> -		goto out;
Eric> +	new_iint->inode = inode;
Eric> +	new_node = &new_iint->rb_node;
 
Eric>  	spin_lock(&ima_iint_lock);
Eric> -	rc = radix_tree_insert(&ima_iint_store, (unsigned long)inode, iint);
Eric> +
Eric> +	p = &ima_iint_tree.rb_node;
Eric> +	while (*p) {
Eric> +		parent = *p;
Eric> +		test_iint = rb_entry(parent, struct ima_iint_cache, rb_node);
Eric> +
Eric> +		rc = -EEXIST;
Eric> +		if (inode < test_iint->inode)
Eric> +			p = &(*p)->rb_left;
Eric> +		else if (inode > test_iint->inode)
Eric> +			p = &(*p)->rb_right;
Eric> +		else
Eric> +			goto out_err;
Eric> +	}
Eric> +
Eric> +	rb_link_node(new_node, parent, p);
Eric> +	rb_insert_color(new_node, &ima_iint_tree);
Eric> +
Eric>  	spin_unlock(&ima_iint_lock);
Eric> -	radix_tree_preload_end();
Eric> -out:
Eric> -	if (rc < 0)
Eric> -		kmem_cache_free(iint_cache, iint);
 
Eric> +	return 0;
Eric> +out_err:
Eric> +	spin_unlock(&ima_iint_lock);
Eric> +	kref_put(&new_iint->refcount, iint_free);
Eric>  	return rc;
Eric>  }
 
Eric> @@ -99,13 +143,6 @@ void iint_free(struct kref *kref)
Eric>  	kmem_cache_free(iint_cache, iint);
Eric>  }
 
Eric> -void iint_rcu_free(struct rcu_head *rcu_head)
Eric> -{
Eric> -	struct ima_iint_cache *iint = container_of(rcu_head,
Eric> -						   struct ima_iint_cache, rcu);
Eric> -	kref_put(&iint->refcount, iint_free);
Eric> -}
Eric> -
Eric>  /**
Eric>   * ima_inode_free - called on security_inode_free
Eric>   * @inode: pointer to the inode
Eric> @@ -117,10 +154,12 @@ void ima_inode_free(struct inode *inode)
Eric>  	struct ima_iint_cache *iint;
 
Eric>  	spin_lock(&ima_iint_lock);
Eric> -	iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
Eric> +	iint = __ima_iint_find(inode);
Eric> +	if (iint)
Eric> +		rb_erase(&iint->rb_node, &ima_iint_tree);
Eric>  	spin_unlock(&ima_iint_lock);
Eric>  	if (iint)
Eric> -		call_rcu(&iint->rcu, iint_rcu_free);
Eric> +		kref_put(&iint->refcount, iint_free);
Eric>  }
 
Eric>  static void init_once(void *foo)

Eric> --
Eric> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Eric> the body of a message to majordomo@vger.kernel.org
Eric> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric> Please read the FAQ at  http://www.tux.org/lkml/

-- 

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[John Stoffel]

>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

Eric> IMA tracks the number of struct files which are holding a given
Eric> inode readonly and the number which are holding the inode write
Eric> or r/w.  It needs this information so when a new reader or
Eric> writer comes in it can tell if this new file will be able to
Eric> invalidate results it already made about existing files.

Eric> aka if a task is holding a struct file open RO, IMA measured the
Eric> file and recorded those measurements and then a task opens the
Eric> file RW IMA needs to note in the logs that the old measurement
Eric> may not be correct.  It's called a "Time of Measure Time of Use"
Eric> (ToMToU) issue.  The same is true is a RO file is opened to an
Eric> inode which has an open writer.  We cannot, with any validity,
Eric> measure the file in question since it could be changing.

So if ToMToU is blown away by opening a file RW, how do you handle a
denial of attack which just opens and closes a bunch of files under
IMA control as fast as it can?  

What happens then?  Does IMA try to re-calculate it's integrity
measurement each and every time this happens?  What does this do to
system performance?  

If I open a file RO, read 512 bytes in a random block, then writes
those same 512 bytes back to the exact same spot, what happens?  

While I'm really concerned about the overhead when it's disabled, I'm
not sure you've explained the overhead when it *IS* enabled, esp on
systems with lots of disks and lots of files.  

The problems with kernel.org is a perfect exmaple of how an annocuous
feature like this, can kill a system's performance.

John


Eric> This patch attempts to use the i_writecount field to track writers.  The
Eric> i_writecount field actually embeds more information in it's value than IMA
Eric> needs but it should work for our purposes and allow us to shrink the struct
Eric> inode even more.

Eric> Signed-off-by: Eric Paris <eparis@redhat.com>
Eric> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Eric> ---

Eric>  security/integrity/ima/ima.h      |    1 -
Eric>  security/integrity/ima/ima_iint.c |    6 ------
Eric>  security/integrity/ima/ima_main.c |   16 ++++++----------
Eric>  3 files changed, 6 insertions(+), 17 deletions(-)

Eric> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
Eric> index 80aca3d..b546b90 100644
Eric> --- a/security/integrity/ima/ima.h
Eric> +++ b/security/integrity/ima/ima.h
Eric> @@ -108,7 +108,6 @@ struct ima_iint_cache {
Eric>  	struct mutex mutex;	/* protects: version, flags, digest */
Eric>  	/* protected by inode->i_lock */
Eric>  	unsigned int readcount;	/* measured files readcount */
Eric> -	unsigned int writecount;/* measured files writecount */
Eric>  	struct kref refcount;	/* ima_iint_cache reference count */
Eric>  };
 
Eric> diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
Eric> index db71a13..e68891f 100644
Eric> --- a/security/integrity/ima/ima_iint.c
Eric> +++ b/security/integrity/ima/ima_iint.c
Eric> @@ -129,11 +129,6 @@ void iint_free(struct kref *kref)
iint-> readcount);
iint-> readcount = 0;
Eric>  	}
Eric> -	if (iint->writecount != 0) {
Eric> -		printk(KERN_INFO "%s: writecount: %u\n", __func__,
Eric> -		       iint->writecount);
Eric> -		iint->writecount = 0;
Eric> -	}
Eric>  	kref_init(&iint->refcount);
Eric>  	kmem_cache_free(iint_cache, iint);
Eric>  }
Eric> @@ -166,7 +161,6 @@ static void init_once(void *foo)
iint-> flags = 0UL;
Eric>  	mutex_init(&iint->mutex);
iint-> readcount = 0;
Eric> -	iint->writecount = 0;
Eric>  	kref_init(&iint->refcount);
Eric>  }
 
Eric> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
Eric> index 2f9b5d5..24660bf 100644
Eric> --- a/security/integrity/ima/ima_main.c
Eric> +++ b/security/integrity/ima/ima_main.c
Eric> @@ -94,8 +94,6 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 
Eric>  	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
iint-> readcount++;
Eric> -	if (mode & FMODE_WRITE)
Eric> -		iint->writecount++;
Eric>  }
 
Eric>  /*
Eric> @@ -173,18 +171,16 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
iint-> readcount--;
Eric>  	}
Eric>  	if (mode & FMODE_WRITE) {
Eric> -		if (unlikely(iint->writecount == 0))
Eric> +		if (atomic_read(&inode->i_writecount) <= 0)
Eric>  			dump = true;
Eric> -		iint->writecount--;
Eric> -		if (iint->writecount == 0) {
Eric> -			if (iint->version != inode->i_version)
Eric> -				iint->flags &= ~IMA_MEASURED;
Eric> -		}
Eric> +		if (atomic_read(&inode->i_writecount) == 1 &&
Eric> +		    iint->version != inode->i_version)
Eric> +			iint->flags &= ~IMA_MEASURED;
Eric>  	}
 
Eric>  	if (dump && !ima_limit_imbalance(file)) {
Eric> -		printk(KERN_INFO "%s: open/free imbalance (r:%u w:%u)\n",
Eric> -		       __func__, iint->readcount, iint->writecount);
Eric> +		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
Eric> +		       __func__, iint->readcount);
Eric>  		dump_stack();
Eric>  	}
Eric>  }

Eric> --
Eric> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Eric> the body of a message to majordomo@vger.kernel.org
Eric> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric> Please read the FAQ at  http://www.tux.org/lkml/

-- 

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[J.H.]

On 10/25/2010 12:21 PM, John Stoffel wrote:
>>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:
> 
> Eric> The IMA code needs to store the number of tasks which have an
> Eric> open fd granting permission to write a file even when IMA is not
> Eric> in use.  It needs this information in order to be enabled at a
> Eric> later point in time without losing it's integrity garantees.
> 
> This sounds completely wrong to me.  If I disable IMA (but have the
> sucker compiled in due to a vendor...) I don't want *any* overhead,
> and this is speaking using my SysAdmin hat for people who do EDA
> design work, and having fast systems is key for us.  IMA is NOT.  We
> disable SELINUX too, because of the overhead and the maintenance
> nightmare.

I'll second both points.  If IMA is disabled, but compiled in, it *HAS*
to consume 0 resources.  It's defective by design if something that is
only useful to 1% of the userbase (guesstimating) consumes even 1% of
the resources on any and all machines that are deployed with it.  It's
just plain wasteful of power, cooling, ram, compute resources, etc.

Keep in mind distros are going to err on the side of compiling things in
(either into the kernel or as modules) because there is a customer
*SOMEWHERE* that wants it.  Penalizing the rest of us, and forcing us
(effectively) to buy more ram, is not a good option.

> If IMA isn't enabled right from the get-go, how can you ensure
> integrity?  And how can you ensure integrity if root is compromised?
> If root can disable IMA, screw around with a file, then turn on IMA
> again without the change being guarrentteed to be noticed (and not
> because the attacked didn't do the attack perfectly!) what's the use?
> 
> How does this help any?  
> 
> To quote from:
> http://domino.research.ibm.com/comm/research_people.nsf/pages/sailer.ima.html
> 
>     What IMA is not
> 
>     IMA is not controlling your system. IMA is non-intrusive and is best
>     described as an independent observer collecting integrity information
>     of loaded code or sensitive application files on demand. Consequently,
>     IMA does not prevent a system from illegal behavior that might
>     compromise the system including the integrity measurement architecture
>     itself. Recognizing the danger of being by-passed, IMA simply
>     invalidates its own measurement list by invalidating the TPM integrity
>     aggregate and thereby rendering the evidence useless (non-verifiable)
>     until it is reset during the next system reboot. For example, if
>     applications write directly to a device (/dev/hda, /dev/sda) or kernel
>     memory (/dev/kmem), then IMA invalidates the TPM aggregate.
> 
>     IMA is not a Digital Rights Management tool either. IMA collects
>     evidence on the local system, which can be used for many purposes but
>     whose release is fully controlled by the local system. It is getting
>     harder to lie about what you are running when you use IMA; no doubt
>     about it. However, if system security is going to be reality, systems
>     that lie at will seem not a convincing alternative in a distributed
>     environment where the weakest link determines the security of the
>     distributed service. The price to pay is that properties must be
>     established securely and that the balance between use and abuse of
>     knowledge about such properties as well as the validity of requiring
>     such evidence (e.g., before connecting a system to a video-on-demand
>     service) must be controlled by rules that are enforced the same way
>     they are in other areas of our daily life (most likely by laws).
> 
> 
> So basically, IMA is super duper tripwire with the ability to allow
> some remote system to come in and ask for your Hashes.  And if you
> screw around with it, it immediately disables itself.  
> 
> Which seems to fly in the face of your claim that it needs to be able
> to re-enable itself by tracking open inodes even when disabled.

Wouldn't an enterprising root-kit snag root, muck with IMA and then muck
with other files?  I mean I'm happy that it will trip-wire the
incompetent or old root kits, but if root can enable/disable this
outside of boot it's completely pointless.

> 
> Eric>   At the moment that means we store a little bit of data about
> Eric> every inode in a cache.  We use a radix tree key'd on the
> Eric> inode's memory address.  Dave Chinner pointed out that a radix
> Eric> tree is a terrible data structure for such a sparse key space.
> Eric> This patch switches to using an rbtree which should be more
> Eric> efficient.
> 
> As the number of inodes goes up (say during a backup which reads
> them...) won't the size of this cache go up as well, even when IMA is
> disabled?  Why is this overhead even needed?  

Ignore backups, file servers and large webservers (read: basically
kernel.org's *ENTIRE* infrastructure) basically trips over this case.
To throw fire on the whole thing, if there are 100 mirrors in a
mirroring infrastructure (not uncommon for some distros) and they all
have IMA disabled but compiled in, and they are all rsyncing data (say a
distro release is ongoing) you are talking about a huge amount of waste
across all those machines.

I'd argue disabled = takes up no resources, that's what a sysadmin is
going to generally expect particularly for something that not many of
them are going to want to take advantage of.

Just my $0.02usd anyway.

- John 'Warthog9' Hawley

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Linus Torvalds]

On Mon, Oct 25, 2010 at 12:38 PM, J.H. <warthog9@kernel.org> wrote:
>
> I'll second both points.  If IMA is disabled, but compiled in, it *HAS*
> to consume 0 resources.

I disagree. First off, this isn't actually true. Look at things like
quota support: it eats more memory in the inode than IMA does after
this patch-series (two pointers), and most people don't use that
either. So the "it must use zero extra memory" is bogus - it's a
balance between simplicity of the code and memory use.

Secondly, right now we're in the situation that IMA just sucks. Sucks
with all capital letters, in fact. This patch-series may not be
perfect, but it's _so_ much better than the current situation that I
don't really see why people are so adamantly negative about it.

Please do feel free to be constructive about it, and I'm sure there
are ways to improve even more, but right now "constructive" is not
what the objections seem to be.

                            Linus

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Christoph Hellwig]

On Mon, Oct 25, 2010 at 01:55:09PM -0700, Linus Torvalds wrote:
> Please do feel free to be constructive about it, and I'm sure there
> are ways to improve even more, but right now "constructive" is not
> what the objections seem to be.

Kyle sent a very useful patch to simply disable the ima tracking unless
you enable it on the command line.

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Linus Torvalds]

On Mon, Oct 25, 2010 at 1:57 PM, Christoph Hellwig <hch@infradead.org> wrote:
>
> Kyle sent a very useful patch to simply disable the ima tracking unless
> you enable it on the command line.

And exactly how does that invalidate _any_ of the patches in the IMA
series in question? All of them are basically still equally valid.

And the "four bytes in 'struct inode' is a total no-no" crowd clearly
haven't looked at struct inode. As mentioned, we've got things like
quota stuff there too.

And quite frankly, it sounds like the right thing to do for Fedora &co
is to simply _disable_ CONFIG_IMA. If there is no support for it on a
distro level, then you shouldn't enable it.

                           Linus

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Eric Paris]

On Mon, 2010-10-25 at 15:21 -0400, John Stoffel wrote:
> >>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

> Which seems to fly in the face of your claim that it needs to be able
> to re-enable itself by tracking open inodes even when disabled.

You're confusing multiple completely unrelated things.  Your confusing
loading an IMA measurement policy vs IMA indicating that it's
measurements may be unreliable.

> As the number of inodes goes up (say during a backup which reads
> them...) won't the size of this cache go up as well, even when IMA is
> disabled?  Why is this overhead even needed?

At the end of this patch the number of integrity structures still has a
1-1 mapping with the number of inodes.  If you look at the entire series
you will see that is not the case.

This patch by itself will cut the memory usage per inode by almost 600
bytes.

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[Eric Paris]

On Mon, 2010-10-25 at 15:27 -0400, John Stoffel wrote:

> The problems with kernel.org is a perfect exmaple of how an annocuous
> feature like this, can kill a system's performance.

You admit that you don't know what you are talking about and then state
that this kills systems performance.  Interesting conclusion.

I'm not going to try to refute you point by point but will instead paint
a broad picture.  I see 3 possible states:
1) Configured out - 0 overhead.  period.
2) Configured in but default disabled
3) Configured in and enabled by admin intervention

I have (I think) pretty clearly discussed the overhead and the changes
made in case #2.  We expand struct inode by 4 bytes, we increment and
decrement those 4 bytes on open/close() and we use a new inode->i_flags.

In you e-mail you seemed to be asking about case #3 where you explicitly
chose to load a measurement policy (either custom or using the imb_tcb=1
boot option).  There are additional overheads in that case if the inode
in question matches the measurement policy.  I don't see the need to go
into the details of that overhead since you have 0 intention of using
this feature no matter what and don't seem to be interested in helping
to change those overheads for users of the subsystem.  Please correct me
if I'm wrong.  I do readily admit there is overhead, and that overhead
will be higher if inodes which have been deemed integrity relevant by
the measurement policy you chose to load are changed in certain
patterns.

-Eric

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[H. Peter Anvin]

On 10/25/2010 02:52 PM, Eric Paris wrote:
> On Mon, 2010-10-25 at 15:27 -0400, John Stoffel wrote:
> 
>> The problems with kernel.org is a perfect exmaple of how an annocuous
>> feature like this, can kill a system's performance.
> 
> You admit that you don't know what you are talking about and then state
> that this kills systems performance.  Interesting conclusion.
> 
> I'm not going to try to refute you point by point but will instead paint
> a broad picture.  I see 3 possible states:
> 1) Configured out - 0 overhead.  period.
> 2) Configured in but default disabled
> 3) Configured in and enabled by admin intervention
> 
> I have (I think) pretty clearly discussed the overhead and the changes
> made in case #2.  We expand struct inode by 4 bytes, we increment and
> decrement those 4 bytes on open/close() and we use a new inode->i_flags.
> 

Case #2 is the bad one, as long as distros are likely to compile it in.

	-hpa

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[Eric Paris]

On Mon, 2010-10-25 at 15:25 -0700, H. Peter Anvin wrote:
> On 10/25/2010 02:52 PM, Eric Paris wrote:
> > On Mon, 2010-10-25 at 15:27 -0400, John Stoffel wrote:
> > 
> >> The problems with kernel.org is a perfect exmaple of how an annocuous
> >> feature like this, can kill a system's performance.
> > 
> > You admit that you don't know what you are talking about and then state
> > that this kills systems performance.  Interesting conclusion.
> > 
> > I'm not going to try to refute you point by point but will instead paint
> > a broad picture.  I see 3 possible states:
> > 1) Configured out - 0 overhead.  period.
> > 2) Configured in but default disabled
> > 3) Configured in and enabled by admin intervention
> > 
> > I have (I think) pretty clearly discussed the overhead and the changes
> > made in case #2.  We expand struct inode by 4 bytes, we increment and
> > decrement those 4 bytes on open/close() and we use a new inode->i_flags.
> > 
> 
> Case #2 is the bad one, as long as distros are likely to compile it in.

Agreed.  And that's the case this whole patch series is addressing.  It
makes it (literally not figuratively) hundreds of times better than it
is today  :)

-Eric

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Dave Chinner]

On Mon, Oct 25, 2010 at 02:41:18PM -0400, Eric Paris wrote:
> The IMA code needs to store the number of tasks which have an open fd
> granting permission to write a file even when IMA is not in use.  It needs
> this information in order to be enabled at a later point in time without
> losing it's integrity garantees.  At the moment that means we store a
> little bit of data about every inode in a cache.  We use a radix tree key'd
> on the inode's memory address.  Dave Chinner pointed out that a radix tree
> is a terrible data structure for such a sparse key space.  This patch
> switches to using an rbtree which should be more efficient.

I'm not sure this is the right fix, though.

Realistically, there is a 1:1 relationship between the inode and the
IMA information. I fail to see why an external index is needed here
at all - just use a separate structure to store the IMA information
that the inode points to. That makes the need for a new global index
and global lock go away completely.

You're already adding 8 bytes to the inode, so why not make it a
pointer. We've got 4 conditions:

1. not configured - no overhead
2. configured, boot time disabled - 8 bytes per inode
3. configured, boot time enabled, runtime disabled - 8 bytes per
inode + small IMA structure
4. configured, boot time enabled, runtime enabled - 8 bytes per
inode + large IMA structure

Anyone who wants the option of runtime enablement can take the extra
allocation overhead, but otherwise nobody is affected apart from 8
bytes of additional memory per inode. I doubt that will change
anything unless it increases the size of the inode enough to push it
over slab boundaries. And if LSM stacking is introduced, then that 8
bytes per inode overhead will go away, anyway.

This approach doesn't introduce new global lock and lookup overhead
into the main VFS paths, allows you to remove a bunch of code and
has a path forward for removing the 8 byte per inode overhead as
well. Seems like the best compromise to me....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Eric Paris]

On Tue, 2010-10-26 at 10:22 +1100, Dave Chinner wrote:
> On Mon, Oct 25, 2010 at 02:41:18PM -0400, Eric Paris wrote:
> > The IMA code needs to store the number of tasks which have an open fd
> > granting permission to write a file even when IMA is not in use.  It needs
> > this information in order to be enabled at a later point in time without
> > losing it's integrity garantees.  At the moment that means we store a
> > little bit of data about every inode in a cache.  We use a radix tree key'd
> > on the inode's memory address.  Dave Chinner pointed out that a radix tree
> > is a terrible data structure for such a sparse key space.  This patch
> > switches to using an rbtree which should be more efficient.
> 
> I'm not sure this is the right fix, though.
> 
> Realistically, there is a 1:1 relationship between the inode and the
> IMA information. I fail to see why an external index is needed here
> at all - just use a separate structure to store the IMA information
> that the inode points to. That makes the need for a new global index
> and global lock go away completely.

I guess I did a bad job explaining my 1:1 relationship comments.  I only
need the i_readcount in a 1:1 manor.  (I'm also using the already
existing i_writecount)  So IMA needs some information in a 1:1
relationship, but everything else in the IMA structure is only needed
when 'a measurement policy is loaded.'

I believe that IBM is going to look into making i_readcount a first
class citizen which can be used by both IMA and generic_setlease().
Then people could say IMA had 0 per inode overhead   :)

> You're already adding 8 bytes to the inode, so why not make it a
> pointer.

4 + 4 padding.  Yes.

> We've got 4 conditions:

You're suggesting we go to 4 conditions?  Today we have 3.

> 1. not configured - no overhead
> 2. configured, boot time disabled - 8 bytes per inode
> 3. configured, boot time enabled, runtime disabled - 8 bytes per
> inode + small IMA structure

2 and 3 are the same today, and both are 4+4.  I believe your suggestion
would be for #3 would be 8 bytes in inode pointing to a 4+4 byte
structure.  I don't really know if that gets us anything.

> 4. configured, boot time enabled, runtime enabled - 8 bytes per
> inode + large IMA structure

> Anyone who wants the option of runtime enablement can take the extra
> allocation overhead, but otherwise nobody is affected apart from 8
> bytes of additional memory per inode. I doubt that will change
> anything unless it increases the size of the inode enough to push it
> over slab boundaries. And if LSM stacking is introduced, then that 8
> bytes per inode overhead will go away, anyway.

At least it gets shifted so you don't see it.  Can't say it goes
away....

> This approach doesn't introduce new global lock and lookup overhead
> into the main VFS paths, allows you to remove a bunch of code and
> has a path forward for removing the 8 byte per inode overhead as
> well. Seems like the best compromise to me....

End of my patch series there are no global locks in main VFS paths
(unless you load an ima measurement policy).  I realize that this patch
switches an rcu_readlock() to a spin_lock() and maybe that's what you
means, but you'll find that I drop ALL locking on core paths when you
don't load a measurement policy in 10/11

http://marc.info/?l=linux-kernel&m=128803236419823&w=2

-Eric

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[John Stoffel]

>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

Eric> On Mon, 2010-10-25 at 15:21 -0400, John Stoffel wrote:
>> >>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

>> Which seems to fly in the face of your claim that it needs to be able
>> to re-enable itself by tracking open inodes even when disabled.

Eric> You're confusing multiple completely unrelated things.  Your
Eric> confusing loading an IMA measurement policy vs IMA indicating
Eric> that it's measurements may be unreliable.

So educate me (and the rest of LKML) about these differences and what
they mean.  Can you please add in a patch to this series which puts in
some documentation on how and why to use this stuff?

And can you also change the Kconfig to default to 'N' for this feature
too!  The help text says to say 'N' by default, so that should be the
default, right?

>> As the number of inodes goes up (say during a backup which reads
>> them...) won't the size of this cache go up as well, even when IMA is
>> disabled?  Why is this overhead even needed?

Eric> At the end of this patch the number of integrity structures
Eric> still has a 1-1 mapping with the number of inodes.  If you look
Eric> at the entire series you will see that is not the case.

Eric> This patch by itself will cut the memory usage per inode by
Eric> almost 600 bytes.

That's a good thing then.  

John

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[John Stoffel]

>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

Eric> On Mon, 2010-10-25 at 15:27 -0400, John Stoffel wrote:
>> The problems with kernel.org is a perfect exmaple of how an annocuous
>> feature like this, can kill a system's performance.

Eric> You admit that you don't know what you are talking about and
Eric> then state that this kills systems performance.  Interesting
Eric> conclusion.

I'm basing my observations on the data reported by John "Warthog9"
who's the kernel.org sysadmin, and the *fact* that IMA was chewing up
gigs of memory when it wasn't even enabled, but just compiled into the
system.

It certainly does NOT help system performance to suck up RAM with data
that is NEVER used by that system.  

Eric> I'm not going to try to refute you point by point but will instead paint
Eric> a broad picture.  I see 3 possible states:

Eric> 1) Configured out - 0 overhead.  period.

Excellent, this should be the default and the Kconfig should be
updated to default to this.  

Eric> 2) Configured in but default disabled

And what is the overhead in this case?  That's what I'm concerned
about.  

Eric> 3) Configured in and enabled by admin intervention

I can't complain about this aspect, though I can still push for the
lowest possible impact on system performance when *any* of these
last two states are in force.  

Eric> I have (I think) pretty clearly discussed the overhead and the
Eric> changes made in case #2.  We expand struct inode by 4 bytes, we
Eric> increment and decrement those 4 bytes on open/close() and we use
Eric> a new inode->i_flags.

Then this is a huge improvement!  Don't get me wrong, I'm negative
about IMA in general, but I'm very happy at how well you've responded
to the firestorm of criticism (even from me, a non-kernel programmer)
about this subsystem. 

Eric> In you e-mail you seemed to be asking about case #3 where you
Eric> explicitly chose to load a measurement policy (either custom or
Eric> using the imb_tcb=1 boot option).  There are additional
Eric> overheads in that case if the inode in question matches the
Eric> measurement policy.  I don't see the need to go into the details
Eric> of that overhead since you have 0 intention of using this
Eric> feature no matter what and don't seem to be interested in
Eric> helping to change those overheads for users of the subsystem.
Eric> Please correct me if I'm wrong.  I do readily admit there is
Eric> overhead, and that overhead will be higher if inodes which have
Eric> been deemed integrity relevant by the measurement policy you
Eric> chose to load are changed in certain patterns.

No.  What I was trying to get at, and probably poorly, was the comment
you made about having to keep the IMA data structures around, even if
IMA has been disabled, so that you could continue to claim integrity
if IMA was re-enabled.

So my question is really about the following situation:

1.  System boots up, IMA is enabled.
2.  SysAdmin notices and turns it off.
    - does the IMA overhead (not the per-inode 4 bytes) go away?
    - do the various in memory data structures get freed?
    - does the pointer in the inode get null'ed?

Thanks,
John

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[John Stoffel]

>>>>> "Eric" == Eric Paris <eparis@redhat.com> writes:

Eric> On Mon, 2010-10-25 at 15:25 -0700, H. Peter Anvin wrote:
>> On 10/25/2010 02:52 PM, Eric Paris wrote:
>> > On Mon, 2010-10-25 at 15:27 -0400, John Stoffel wrote:
>> > 
>> >> The problems with kernel.org is a perfect exmaple of how an annocuous
>> >> feature like this, can kill a system's performance.
>> > 
>> > You admit that you don't know what you are talking about and then state
>> > that this kills systems performance.  Interesting conclusion.
>> > 
>> > I'm not going to try to refute you point by point but will instead paint
>> > a broad picture.  I see 3 possible states:
>> > 1) Configured out - 0 overhead.  period.
>> > 2) Configured in but default disabled
>> > 3) Configured in and enabled by admin intervention
>> > 
>> > I have (I think) pretty clearly discussed the overhead and the changes
>> > made in case #2.  We expand struct inode by 4 bytes, we increment and
>> > decrement those 4 bytes on open/close() and we use a new inode->i_flags.
>> > 
>> 
>> Case #2 is the bad one, as long as distros are likely to compile it in.

Eric> Agreed.  And that's the case this whole patch series is addressing.  It
Eric> makes it (literally not figuratively) hundreds of times better than it
Eric> is today  :)

And just to chime in, I really appreciate your hard work on this
cleanup!

John

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[John Stoffel]

>>>>> "Linus" == Linus Torvalds <torvalds@linux-foundation.org> writes:

Linus> On Mon, Oct 25, 2010 at 1:57 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> 
>> Kyle sent a very useful patch to simply disable the ima tracking unless
>> you enable it on the command line.

Linus> And exactly how does that invalidate _any_ of the patches in
Linus> the IMA series in question? All of them are basically still
Linus> equally valid.

Well, if we're going to keep IMA as an option, then this cleanup is
certainly worthwhile.   And keeping it's impact down as much as
possible is even better.  

Linus> And the "four bytes in 'struct inode' is a total no-no" crowd
Linus> clearly haven't looked at struct inode. As mentioned, we've got
Linus> things like quota stuff there too.

Quota is arguably much more useful than IMA, and to a much larger
audience.  There's a reason it's in there.  As a SysAdmin, one of my
major gripes is how hard it is to manage disk space usage by my users
and track it in useful ways.

Quotas allow me to do a quicker, more targeted response when disk
space fills up and I need to find the biggest users.  Would I like
better quota reporting?  Sure!  Do I want more overhead, not so much.
It's a balancing act.

Linus> And quite frankly, it sounds like the right thing to do for
Linus> Fedora &co is to simply _disable_ CONFIG_IMA. If there is no
Linus> support for it on a distro level, then you shouldn't enable it.

So the Kconfig should have 'default N' for IMA then?

John

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[John Stoffel]

>>>>> "Linus" == Linus Torvalds <torvalds@linux-foundation.org> writes:

Linus> On Mon, Oct 25, 2010 at 12:38 PM, J.H. <warthog9@kernel.org> wrote:
>> 
>> I'll second both points.  If IMA is disabled, but compiled in, it *HAS*
>> to consume 0 resources.

Linus> I disagree. First off, this isn't actually true. Look at things
Linus> like quota support: it eats more memory in the inode than IMA
Linus> does after this patch-series (two pointers), and most people
Linus> don't use that either. So the "it must use zero extra memory"
Linus> is bogus - it's a balance between simplicity of the code and
Linus> memory use.

Quotas are useful in a much more general sense for managing a limited
resource (disk space) and for a larger audience as well.  And hey,
let's target quotas next!  *grin*

Linus> Secondly, right now we're in the situation that IMA just
Linus> sucks. Sucks with all capital letters, in fact. This
Linus> patch-series may not be perfect, but it's _so_ much better than
Linus> the current situation that I don't really see why people are so
Linus> adamantly negative about it.

I'm negative about it because I forsee very limited applicability to
normal day to day use of Linux in my work.  Quotas I use every day.  

Linus> Please do feel free to be constructive about it, and I'm sure
Linus> there are ways to improve even more, but right now
Linus> "constructive" is not what the objections seem to be.

Sorry, will certainly try to be more positive about my objections to
this system.  

Mostly I'd really like to just see:

       - documentation
       - Kconfig updated to default to N.

John

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Linus Torvalds]

On Tue, Oct 26, 2010 at 7:01 AM, John Stoffel <john@stoffel.org> wrote:
>
> So the Kconfig should have 'default N' for IMA then?

ALL new features should have "default n" for them. And if you had
actually looked at it, you would see that it already has that ("n" is
the default if no default is listed) _and_ it says

  "If unsure, say N"

in the comments.

So why the hell are people complaining about a patch-series that
_clearly_ improves on the current situation?

And yes, Fedora should never have enabled it. If the distro doesn't
use a feature, it shouldn't be enabled, because it's inevitably just a
source of problems. In this case, I think we should be happy that it
was enabled just because it made people notice the problem, but at the
same time the fact that Fedora enabled it is _not_ justification for
then saying "well, if you enable it and don't use it, it must be
zero-overhead".

If you want zero overhead and you think nobody uses it (and that seems
to be the _only_ logic the people complaining about it keep drumming
on), then DON'T ENABLE IT, FOR CHRISSAKE!

This thread has been a total waste of everybody's time.

Did I miss any actual _constructive_ criticism of the patches? Is
there any reason I shouldn't actually apply them? If there is, I've
lost it in the roar of pointlessness.

                      Linus

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Eric Paris]

On Tue, 2010-10-26 at 08:22 -0700, Linus Torvalds wrote:

> Did I miss any actual _constructive_ criticism of the patches? Is
> there any reason I shouldn't actually apply them? If there is, I've
> lost it in the roar of pointlessness.

I think dchinner had some comments on what he might like to see in the
long run to further improve the situation, but I don't think anything he
requested or commented on precludes this series from being applied...

-Eric

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[John Stoffel]

>>>>> "Linus" == Linus Torvalds <torvalds@linux-foundation.org> writes:

Linus> On Tue, Oct 26, 2010 at 7:01 AM, John Stoffel <john@stoffel.org> wrote:
>> 
>> So the Kconfig should have 'default N' for IMA then?

Linus> ALL new features should have "default n" for them. And if you
Linus> had actually looked at it, you would see that it already has
Linus> that ("n" is the default if no default is listed) _and_ it says

Linus>   "If unsure, say N"

Linus> in the comments.

Yes, I did look at it.  I just didn't see a default listed for the
overall IMA feature, while sub-feature do have a 'default N' option
set.  And so my confusion.  My fault for not understanding Kconfig
defaults better.

John

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[Al Viro]

On Tue, Oct 26, 2010 at 08:22:31AM -0700, Linus Torvalds wrote:

> If you want zero overhead and you think nobody uses it (and that seems
> to be the _only_ logic the people complaining about it keep drumming
> on), then DON'T ENABLE IT, FOR CHRISSAKE!
> 
> This thread has been a total waste of everybody's time.
> 
> Did I miss any actual _constructive_ criticism of the patches? Is
> there any reason I shouldn't actually apply them? If there is, I've
> lost it in the roar of pointlessness.

FWIW, I'm OK with that.  My general opinion about the quality and
usefulness of security/* is a separate story, but if we are stuck
with that shit in the tree, let's at least trim down what can be
trimmed down.

Re: [PATCH 06/11] IMA: use i_writecount rather than a private counter

[H. Peter Anvin]

On 10/26/2010 06:53 AM, John Stoffel wrote:
> 
> No.  What I was trying to get at, and probably poorly, was the comment
> you made about having to keep the IMA data structures around, even if
> IMA has been disabled, so that you could continue to claim integrity
> if IMA was re-enabled.
> 
> So my question is really about the following situation:
> 
> 1.  System boots up, IMA is enabled.
> 2.  SysAdmin notices and turns it off.
>     - does the IMA overhead (not the per-inode 4 bytes) go away?
>     - do the various in memory data structures get freed?
>     - does the pointer in the inode get null'ed?
> 

I think it's reasonable to require a reboot in this case.

	-hpa

Re: [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache

[James Morris]

On Tue, 26 Oct 2010, Al Viro wrote:

> FWIW, I'm OK with that.  My general opinion about the quality and
> usefulness of security/* is a separate story, but if we are stuck
> with that shit in the tree, let's at least trim down what can be
> trimmed down.

If you have any specific concerns about code in security/ then please do 
explain them (in a different thread), so we can address them.

Note that if code is not being used, and/or has serious technical issues, 
it can be removed.  This already happened once with the BSD secure levels, 
which was basically unused, unmaintained, and had unfixable security 
issues -- it was taken out and shot.

Unfortunately, we have to deal with the fact that Unix was not designed 
with security primarily in mind, and that the security it does have is 
inherently flawed (all according to Ritchie c. 1970s).  We have to 
maintain decades of backward compatibility, address underlying flaws, and 
also address new forms of security threats which could not have even been 
imagined at the time.  We have to do all this without maybe using more 
than a few extra bytes in any major core data structure, and with nobody 
really agreeing on how to do it.

Expecting perfection out of this process is probably unreasonable, alas.



- James
-- 
James Morris
<jmorris@namei.org>