linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache
@ 2010-10-25 18:41 Eric Paris
  2010-10-25 18:41 ` [PATCH 02/11] IMA: drop the inode opencount since it isn't needed for operation Eric Paris
                   ` (11 more replies)
  0 siblings, 12 replies; 34+ messages in thread
From: Eric Paris @ 2010-10-25 18:41 UTC (permalink / raw)
  To: linux-kernel, linux-security-module, linux-fsdevel
  Cc: hch, zohar, warthog9, david, jmorris, kyle, hpa, akpm, torvalds,
	mingo, eparis, viro

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)


^ permalink raw reply related	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2010-10-27 13:37 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-25 18:41 [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache Eric Paris
2010-10-25 18:41 ` [PATCH 02/11] IMA: drop the inode opencount since it isn't needed for operation Eric Paris
2010-10-25 18:41 ` [PATCH 03/11] IMA: use unsigned int instead of long for counters Eric Paris
2010-10-25 18:41 ` [PATCH 04/11] IMA: convert internal flags from long to char Eric Paris
2010-10-25 18:41 ` [PATCH 05/11] IMA: use inode->i_lock to protect read and write counters Eric Paris
2010-10-25 18:41 ` [PATCH 06/11] IMA: use i_writecount rather than a private counter Eric Paris
2010-10-25 19:27   ` John Stoffel
2010-10-25 21:52     ` Eric Paris
2010-10-25 22:25       ` H. Peter Anvin
2010-10-25 22:29         ` Eric Paris
2010-10-26 13:57           ` John Stoffel
2010-10-26 13:53       ` John Stoffel
2010-10-26 22:08         ` H. Peter Anvin
2010-10-25 18:41 ` [PATCH 07/11] IMA: move read counter into struct inode Eric Paris
2010-10-25 18:42 ` [PATCH 08/11] IMA: only allocate iint when needed Eric Paris
2010-10-25 18:42 ` [PATCH 09/11] IMA: drop refcnt from ima_iint_cache since it isn't needed Eric Paris
2010-10-25 18:42 ` [PATCH 10/11] IMA: explicit IMA i_flag to remove global lock on inode_delete Eric Paris
2010-10-25 18:42 ` [PATCH 11/11] IMA: fix the ToMToU logic Eric Paris
2010-10-25 19:21 ` [PATCH 01/11] IMA: use rbtree instead of radix tree for inode information cache John Stoffel
2010-10-25 19:38   ` J.H.
2010-10-25 20:55     ` Linus Torvalds
2010-10-25 20:57       ` Christoph Hellwig
2010-10-25 21:11         ` Linus Torvalds
2010-10-26 14:01           ` John Stoffel
2010-10-26 15:22             ` Linus Torvalds
2010-10-26 15:30               ` Eric Paris
2010-10-26 15:53               ` John Stoffel
2010-10-26 18:13               ` Al Viro
2010-10-27 13:35                 ` James Morris
2010-10-26 14:07       ` John Stoffel
2010-10-25 21:34   ` Eric Paris
2010-10-26 13:45     ` John Stoffel
2010-10-25 23:22 ` Dave Chinner
2010-10-26  0:12   ` Eric Paris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).