On Sat, Oct 16, 2010 at 02:10:29PM -0700, H. Peter Anvin wrote:
I'm confused ... what makes you think we are? This might have been an unintentional misconfiguration...
I didn't mean to imply you enabled it intentionally. In fact it looks like the inode tracking in IMA is always on once it's compiled in, which totally defeats the purpose of doing it's on iternal inode tracking instead of bloating the inode what they originally proposed. IMA really needs a kernel parameter to only enabled this crap when people actually use it.
And whoever turned it on in Fedora needs some serious wahcking.
On Sat, Oct 16, 2010 at 08:49:45PM -0400, Christoph Hellwig wrote:
On Sat, Oct 16, 2010 at 02:10:29PM -0700, H. Peter Anvin wrote:
I'm confused ... what makes you think we are? This might have been an unintentional misconfiguration...
I didn't mean to imply you enabled it intentionally. In fact it looks like the inode tracking in IMA is always on once it's compiled in, which totally defeats the purpose of doing it's on iternal inode tracking instead of bloating the inode what they originally proposed. IMA really needs a kernel parameter to only enabled this crap when people actually use it.
And whoever turned it on in Fedora needs some serious wahcking.
Hey Eric, sorry to throw you under the bus, but...
1.316 (eparis 11-Aug-09): CONFIG_IMA=y
Perhaps we should stop merging crap into the kernel if it's not supposed to be enabled. (Granted, IMA is better than 99% of the random junk merged in a release, it actually has somewhat useful Kconfig help text, unlike the thousands of crap drivers that get merged which provide no hint to what obscure medieval ARM board found only on three silicon revisions two of which were never released they were for.)
The Kconfig description sounds generally useful to me. If it's bloated crap, how did it get merged?
Sigh.
--Kyle
On Sat, Oct 16, 2010 at 09:09:17PM -0400, Kyle McMartin wrote:
The Kconfig description sounds generally useful to me. If it's bloated crap, how did it get merged?
Just like all the other crap. It's been made pretty clear that "no" generally is not an answer for a merge request. Just bikeshedding over whitespace or maintainer files entries for a couple month of delay is fine.
* Christoph Hellwig hch@infradead.org wrote:
On Sat, Oct 16, 2010 at 09:09:17PM -0400, Kyle McMartin wrote:
The Kconfig description sounds generally useful to me. If it's bloated crap, how did it get merged?
Just like all the other crap. It's been made pretty clear that "no" generally is not an answer for a merge request. Just bikeshedding over whitespace or maintainer files entries for a couple month of delay is fine.
IMO insensitive, tester alienating insults like the one you have just launched against the wrong person are far more destructive to the efficiency of the kernel quality process than the same-old influx of crap.
You'd be right to flame the heck out of the lkml hardened person(s) who allowed that crap, but not the person who was in the chain of testing that helped us find it, for chrissakes!
We keep losing really good people due to social imbeciles like you.
Thanks,
Ingo
* Christoph Hellwig hch@infradead.org wrote:
On Sat, Oct 16, 2010 at 02:10:29PM -0700, H. Peter Anvin wrote:
"Christoph Hellwig" hch@infradead.org wrote:
Besides the algorithmic problems with ima, why is kernel.org using IMA to start with? Except for IBM looking for a reason to jusity why TPM isn't a completely waster of ressources it's pointless. And it was only merged under the premise that it would not affect innocent normal users.
I'm confused ... what makes you think we are? This might have been an unintentional misconfiguration...
I didn't mean to imply you enabled it intentionally. In fact it looks like the inode tracking in IMA is always on once it's compiled in, which totally defeats the purpose of doing it's on iternal inode tracking instead of bloating the inode what they originally proposed. IMA really needs a kernel parameter to only enabled this crap when people actually use it.
That is true.
And whoever turned it on in Fedora needs some serious wahcking.
And that is false.
This security feature was merged upstream last year, it's not in drivers/staging/ and the Kconfig help text does not contain any warning that this is 'crap', so how were the Fedora people supposed to know?
If you are suggesting that distribution kernel maintainers should not trust upstream kernel feature decisions and are expected to do a line by line review of the ~40,000 commits that go upstream every year, to make sure there's no hidden 'crap' in them (and failing that be labeled incompetent idiots), then you are out of your mind.
It's just not possible to do that nor is it reasonable or efficient: crap should be caught via hierarchical filtering: when the developer posts the first patches to lkml, or when it merged into a maintainer tree, or when it goes upstream or when it is upstream and then, as the very last (and most expensive) line of defense, it will be caught when it gets exposure in distributions. Which seems to be precisely what happened here.
Fact is that Kyle did Linux a _favor_ by enabling the feature in Fedora, as it allowed the bug/inefficiency/crap to be found by Dave. Linux got richer as a result as we learned about a bug that affects many people. Your gratuitous insults against him are highly misguided.
Thanks,
Ingo
On Sun, Oct 17, 2010 at 07:40:08AM +0200, Ingo Molnar wrote:
This security feature was merged upstream last year, it's not in drivers/staging/ and the Kconfig help text does not contain any warning that this is 'crap', so how were the Fedora people supposed to know?
By looking at what they turn on? What happened to the good old idea of actually auditing what you turn on? It might be a bit too much for every little driver, aven if that was helpful, but for security/ code with intricate hooks all over the kernel I think it is in order.
Especially as our merge requirements for security/ are a lot lower than for the rest of the kernel given that James is very afraid of getting whacked by Linux for not mering things.
Fact is that Kyle did Linux a _favor_ by enabling the feature in Fedora, as it allowed the bug/inefficiency/crap to be found by Dave. Linux got richer as a result as we learned about a bug that affects many people. Your gratuitous insults against him are highly misguided.
I think you need to tune down your insult filter a bit :)
On Sun, 17 Oct 2010, Christoph Hellwig wrote:
Especially as our merge requirements for security/ are a lot lower than for the rest of the kernel given that James is very afraid of getting whacked by Linux for not mering things.
I think historically you'll see that I'm not afraid of getting whacked by Linus.
A procedure for merging security features has been adopted by consensus, based on suggestions from Arjan, with the aim of preventing the literally endless arguments which arise from security feature discussions. It's sometimes referred to as the Arjan protocol, essentially:
If the feature correctly implements a well-defined security goal, meets user needs without incurring unreasonable overheads, passes technical review, and is supported by competent developers, then it is likely to be merged.
If you disagree with a specific feature, you need to step up while it's being reviewed and make a case against it according to the above criteria.
If you disagree with the protocol, then you need to come up with a better one, and probably implement it yourself, to the satisfaction of all parties.
- James
If someone gives me a good reason why Fedora actually needs this enabled, I'm going to apply the following patch to our kernel so that IMA is globally an opt-in feature... Otherwise I'm inclined to just disable it.
(But the more I look at this, the more it looks like a completely niche option that has little use outside of three-letter agencies.)
I regret not looking at this more closely when it was enabled, (although, in my defence, when the option first showed up, I left it off...)
It's probably way more heavyweight than necessary, as I think in most cases the iint_initalized test will cover the call into IMA. But better safe than sorry or something and there's a bunch of other cases where -ENOMEM will leak back up from failing kmem_cache_alloc, iirc.
regards, Kyle
--- From: Kyle McMartin kyle@mcmartin.ca Date: Mon, 18 Oct 2010 02:08:35 -0400 Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Allow IMA to be entirely disabled, don't even bother calling into the provided hooks, and avoid initializing caches.
(A lot of the hooks will test iint_initialized, and so this doubly disables them, since the iint cache won't be enabled. But hey, we avoid a pointless branch...)
Signed-off-by: Kyle McMartin kyle@redhat.com --- include/linux/ima.h | 66 +++++++++++++++++++++++++++++++++---- security/integrity/ima/ima_iint.c | 13 +++++-- security/integrity/ima/ima_main.c | 34 +++++++++++++------ 3 files changed, 91 insertions(+), 22 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h index 975837e..2fa456d 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -14,13 +14,65 @@ struct linux_binprm;
#ifdef CONFIG_IMA -extern int ima_bprm_check(struct linux_binprm *bprm); -extern int ima_inode_alloc(struct inode *inode); -extern void ima_inode_free(struct inode *inode); -extern int ima_file_check(struct file *file, int mask); -extern void ima_file_free(struct file *file); -extern int ima_file_mmap(struct file *file, unsigned long prot); -extern void ima_counts_get(struct file *file); + +extern int ima_enabled; + +extern int __ima_bprm_check(struct linux_binprm *bprm); +extern int __ima_inode_alloc(struct inode *inode); +extern void __ima_inode_free(struct inode *inode); +extern int __ima_file_check(struct file *file, int mask); +extern void __ima_file_free(struct file *file); +extern int __ima_file_mmap(struct file *file, unsigned long prot); +extern void __ima_counts_get(struct file *file); + +static inline int ima_bprm_check(struct linux_binprm *bprm) +{ + if (ima_enabled) + return __ima_bprm_check(bprm); + return 0; +} + +static inline int ima_inode_alloc(struct inode *inode) +{ + if (ima_enabled) + return __ima_inode_alloc(inode); + return 0; +} + +static inline void ima_inode_free(struct inode *inode) +{ + if (ima_enabled) + __ima_inode_free(inode); + return; +} + +static inline int ima_file_check(struct file *file, int mask) +{ + if (ima_enabled) + return __ima_file_check(file, mask); + return 0; +} + +static inline void ima_file_free(struct file *file) +{ + if (ima_enabled) + __ima_file_free(file); + return; +} + +static inline int ima_file_mmap(struct file *file, unsigned long prot) +{ + if (ima_enabled) + return __ima_file_mmap(file, prot); + return 0; +} + +static inline void ima_counts_get(struct file *file) +{ + if (ima_enabled) + return __ima_counts_get(file); + return; +}
#else static inline int ima_bprm_check(struct linux_binprm *bprm) diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index afba4ae..767f026 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -46,10 +46,10 @@ out: }
/** - * ima_inode_alloc - allocate an iint associated with an inode + * __ima_inode_alloc - allocate an iint associated with an inode * @inode: pointer to the inode */ -int ima_inode_alloc(struct inode *inode) +int __ima_inode_alloc(struct inode *inode) { struct ima_iint_cache *iint = NULL; int rc = 0; @@ -107,12 +107,12 @@ void iint_rcu_free(struct rcu_head *rcu_head) }
/** - * ima_inode_free - called on security_inode_free + * __ima_inode_free - called on security_inode_free * @inode: pointer to the inode * * Free the integrity information(iint) associated with an inode. */ -void ima_inode_free(struct inode *inode) +void __ima_inode_free(struct inode *inode) { struct ima_iint_cache *iint;
@@ -139,6 +139,11 @@ static void init_once(void *foo)
static int __init ima_iintcache_init(void) { + extern int ima_enabled; + + if (!ima_enabled) + return 0; + iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index e662b89..92e084c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -26,6 +26,7 @@ #include "ima.h"
int ima_initialized; +int ima_enabled = 0;
char *ima_hash = "sha1"; static int __init hash_setup(char *str) @@ -36,6 +37,14 @@ static int __init hash_setup(char *str) } __setup("ima_hash=", hash_setup);
+static int __init ima_enable(char *str) +{ + if (strncmp(str, "on", 2) == 0) + ima_enabled = 1; + return 1; +} +__setup("ima=", ima_enable); + struct ima_imbalance { struct hlist_node node; unsigned long fsmagic; @@ -130,7 +139,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode) }
/* - * ima_counts_get - increment file counts + * __ima_counts_get - increment file counts * * Maintain read/write counters for all files, but only * invalidate the PCR for measured files: @@ -140,7 +149,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode) * could result in a file measurement error. * */ -void ima_counts_get(struct file *file) +void __ima_counts_get(struct file *file) { struct dentry *dentry = file->f_path.dentry; struct inode *inode = dentry->d_inode; @@ -204,13 +213,13 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode, }
/** - * ima_file_free - called on __fput() + * __ima_file_free - called on __fput() * @file: pointer to file structure being freed * * Flag files that changed, based on i_version; * and decrement the iint readcount/writecount. */ -void ima_file_free(struct file *file) +void __ima_file_free(struct file *file) { struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint; @@ -255,7 +264,7 @@ out: }
/** - * ima_file_mmap - based on policy, collect/store measurement. + * __ima_file_mmap - based on policy, collect/store measurement. * @file: pointer to the file to be measured (May be NULL) * @prot: contains the protection that will be applied by the kernel. * @@ -265,7 +274,7 @@ out: * Return 0 on success, an error code on failure. * (Based on the results of appraise_measurement().) */ -int ima_file_mmap(struct file *file, unsigned long prot) +int __ima_file_mmap(struct file *file, unsigned long prot) { int rc;
@@ -278,7 +287,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) }
/** - * ima_bprm_check - based on policy, collect/store measurement. + * __ima_bprm_check - based on policy, collect/store measurement. * @bprm: contains the linux_binprm structure * * The OS protects against an executable file, already open for write, @@ -290,7 +299,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) * Return 0 on success, an error code on failure. * (Based on the results of appraise_measurement().) */ -int ima_bprm_check(struct linux_binprm *bprm) +int __ima_bprm_check(struct linux_binprm *bprm) { int rc;
@@ -300,7 +309,7 @@ int ima_bprm_check(struct linux_binprm *bprm) }
/** - * ima_path_check - based on policy, collect/store measurement. + * __ima_path_check - based on policy, collect/store measurement. * @file: pointer to the file to be measured * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE * @@ -309,7 +318,7 @@ int ima_bprm_check(struct linux_binprm *bprm) * Always return 0 and audit dentry_open failures. * (Return code will be based upon measurement appraisal.) */ -int ima_file_check(struct file *file, int mask) +int __ima_file_check(struct file *file, int mask) { int rc;
@@ -318,12 +327,15 @@ int ima_file_check(struct file *file, int mask) FILE_CHECK); return 0; } -EXPORT_SYMBOL_GPL(ima_file_check); +EXPORT_SYMBOL_GPL(__ima_file_check);
static int __init init_ima(void) { int error;
+ if (!ima_enabled) + return 0; + error = ima_init(); ima_initialized = 1; return error;
On Mon, 18 Oct 2010 02:25:30 -0400 Kyle McMartin kyle@mcmartin.ca wrote:
Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Good enough as a short-term thing I guess.
But the memory consumption which David described is plain nuttiness and needs to be fixed, presumably by using a more appropriate data structure for lookups.
However unless I missed it, nobody has yet stated that they intend to fix this?
On Sun, Oct 17, 2010 at 11:36:20PM -0700, Andrew Morton wrote:
On Mon, 18 Oct 2010 02:25:30 -0400 Kyle McMartin kyle@mcmartin.ca wrote:
Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Good enough as a short-term thing I guess.
But the memory consumption which David described is plain nuttiness and needs to be fixed, presumably by using a more appropriate data structure for lookups.
However unless I missed it, nobody has yet stated that they intend to fix this?
I think Eric said he is going to look at it.
Cheers,
Dave.
On Mon, 2010-10-18 at 20:29 +1100, Dave Chinner wrote:
On Sun, Oct 17, 2010 at 11:36:20PM -0700, Andrew Morton wrote:
On Mon, 18 Oct 2010 02:25:30 -0400 Kyle McMartin kyle@mcmartin.ca wrote:
Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Good enough as a short-term thing I guess.
But the memory consumption which David described is plain nuttiness and needs to be fixed, presumably by using a more appropriate data structure for lookups.
However unless I missed it, nobody has yet stated that they intend to fix this?
I think Eric said he is going to look at it.
Cheers,
Dave.
Am looking at it as well.
Mimi
On 10/18/10 6:31 AM, "Mimi Zohar" zohar@linux.vnet.ibm.com wrote:
On Mon, 2010-10-18 at 20:29 +1100, Dave Chinner wrote:
On Sun, Oct 17, 2010 at 11:36:20PM -0700, Andrew Morton wrote:
On Mon, 18 Oct 2010 02:25:30 -0400 Kyle McMartin kyle@mcmartin.ca
wrote:
Subject: [PATCH] ima: allow it to be completely disabled (and
default to off)
Good enough as a short-term thing I guess.
But the memory consumption which David described is plain nuttiness
and
needs to be fixed, presumably by using a more appropriate data
structure
for lookups.
However unless I missed it, nobody has yet stated that they intend to fix this?
I think Eric said he is going to look at it.
Cheers,
Dave.
Am looking at it as well.
Mimi
I'll help look into this aspect too.
To be clear (since my first post seemed to go to the moderator), we will be using this starting with MeeGo 1.2. Yes, this rather pigish memory usage needs to be addressed. Let's focus on fixing that. I'd say there are a number of options we can explore here to do that.
It's already upstream and in the appropriate place as far as I'm concerned. Let's not chuck it out because on second blush there were faults that no one expected.
Ryan
Hi!
Good enough as a short-term thing I guess.
But the memory consumption which David described is plain nuttiness
and
needs to be fixed, presumably by using a more appropriate data
structure
for lookups.
However unless I missed it, nobody has yet stated that they intend to fix this?
I think Eric said he is going to look at it.
Cheers,
Dave.
Am looking at it as well.
Mimi
I'll help look into this aspect too.
To be clear (since my first post seemed to go to the moderator), we will be using this starting with MeeGo 1.2. Yes, this rather pigish memory usage needs to be addressed. Let's focus on fixing that. I'd say there are a number of options we can explore here to do that.
Curious... I see how NSA might want to use it, but MeeGo? Pavel
On Mon, 2010-10-18 at 02:25 -0400, Kyle McMartin wrote:
If someone gives me a good reason why Fedora actually needs this enabled, I'm going to apply the following patch to our kernel so that IMA is globally an opt-in feature... Otherwise I'm inclined to just disable it.
Am hoping others will chime in.
(But the more I look at this, the more it looks like a completely niche option that has little use outside of three-letter agencies.)
I regret not looking at this more closely when it was enabled, (although, in my defence, when the option first showed up, I left it off...)
It's probably way more heavyweight than necessary, as I think in most cases the iint_initalized test will cover the call into IMA. But better safe than sorry or something and there's a bunch of other cases where -ENOMEM will leak back up from failing kmem_cache_alloc, iirc.
regards, Kyle
Thanks, will compile/test patch.
Mimi
From: Kyle McMartin kyle@mcmartin.ca Date: Mon, 18 Oct 2010 02:08:35 -0400 Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Allow IMA to be entirely disabled, don't even bother calling into the provided hooks, and avoid initializing caches.
(A lot of the hooks will test iint_initialized, and so this doubly disables them, since the iint cache won't be enabled. But hey, we avoid a pointless branch...)
Signed-off-by: Kyle McMartin kyle@redhat.com
include/linux/ima.h | 66 +++++++++++++++++++++++++++++++++---- security/integrity/ima/ima_iint.c | 13 +++++-- security/integrity/ima/ima_main.c | 34 +++++++++++++------ 3 files changed, 91 insertions(+), 22 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h index 975837e..2fa456d 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -14,13 +14,65 @@ struct linux_binprm;
#ifdef CONFIG_IMA -extern int ima_bprm_check(struct linux_binprm *bprm); -extern int ima_inode_alloc(struct inode *inode); -extern void ima_inode_free(struct inode *inode); -extern int ima_file_check(struct file *file, int mask); -extern void ima_file_free(struct file *file); -extern int ima_file_mmap(struct file *file, unsigned long prot); -extern void ima_counts_get(struct file *file);
+extern int ima_enabled;
+extern int __ima_bprm_check(struct linux_binprm *bprm); +extern int __ima_inode_alloc(struct inode *inode); +extern void __ima_inode_free(struct inode *inode); +extern int __ima_file_check(struct file *file, int mask); +extern void __ima_file_free(struct file *file); +extern int __ima_file_mmap(struct file *file, unsigned long prot); +extern void __ima_counts_get(struct file *file);
+static inline int ima_bprm_check(struct linux_binprm *bprm) +{
- if (ima_enabled)
return __ima_bprm_check(bprm);
- return 0;
+}
+static inline int ima_inode_alloc(struct inode *inode) +{
- if (ima_enabled)
return __ima_inode_alloc(inode);
- return 0;
+}
+static inline void ima_inode_free(struct inode *inode) +{
- if (ima_enabled)
__ima_inode_free(inode);
- return;
+}
+static inline int ima_file_check(struct file *file, int mask) +{
- if (ima_enabled)
return __ima_file_check(file, mask);
- return 0;
+}
+static inline void ima_file_free(struct file *file) +{
- if (ima_enabled)
__ima_file_free(file);
- return;
+}
+static inline int ima_file_mmap(struct file *file, unsigned long prot) +{
- if (ima_enabled)
return __ima_file_mmap(file, prot);
- return 0;
+}
+static inline void ima_counts_get(struct file *file) +{
- if (ima_enabled)
return __ima_counts_get(file);
- return;
+}
#else static inline int ima_bprm_check(struct linux_binprm *bprm) diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index afba4ae..767f026 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -46,10 +46,10 @@ out: }
/**
- ima_inode_alloc - allocate an iint associated with an inode
*/
- __ima_inode_alloc - allocate an iint associated with an inode
- @inode: pointer to the inode
-int ima_inode_alloc(struct inode *inode) +int __ima_inode_alloc(struct inode *inode) { struct ima_iint_cache *iint = NULL; int rc = 0; @@ -107,12 +107,12 @@ void iint_rcu_free(struct rcu_head *rcu_head) }
/**
- ima_inode_free - called on security_inode_free
*/
- __ima_inode_free - called on security_inode_free
- @inode: pointer to the inode
- Free the integrity information(iint) associated with an inode.
-void ima_inode_free(struct inode *inode) +void __ima_inode_free(struct inode *inode) { struct ima_iint_cache *iint;
@@ -139,6 +139,11 @@ static void init_once(void *foo)
static int __init ima_iintcache_init(void) {
- extern int ima_enabled;
- if (!ima_enabled)
return 0;
- iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index e662b89..92e084c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -26,6 +26,7 @@ #include "ima.h"
int ima_initialized; +int ima_enabled = 0;
char *ima_hash = "sha1"; static int __init hash_setup(char *str) @@ -36,6 +37,14 @@ static int __init hash_setup(char *str) } __setup("ima_hash=", hash_setup);
+static int __init ima_enable(char *str) +{
- if (strncmp(str, "on", 2) == 0)
ima_enabled = 1;
- return 1;
+} +__setup("ima=", ima_enable);
struct ima_imbalance { struct hlist_node node; unsigned long fsmagic; @@ -130,7 +139,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode) }
/*
- ima_counts_get - increment file counts
- __ima_counts_get - increment file counts
- Maintain read/write counters for all files, but only
- invalidate the PCR for measured files:
@@ -140,7 +149,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
could result in a file measurement error.
*/ -void ima_counts_get(struct file *file) +void __ima_counts_get(struct file *file) { struct dentry *dentry = file->f_path.dentry; struct inode *inode = dentry->d_inode; @@ -204,13 +213,13 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode, }
/**
- ima_file_free - called on __fput()
*/
- __ima_file_free - called on __fput()
- @file: pointer to file structure being freed
- Flag files that changed, based on i_version;
- and decrement the iint readcount/writecount.
-void ima_file_free(struct file *file) +void __ima_file_free(struct file *file) { struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint; @@ -255,7 +264,7 @@ out: }
/**
- ima_file_mmap - based on policy, collect/store measurement.
- __ima_file_mmap - based on policy, collect/store measurement.
- @file: pointer to the file to be measured (May be NULL)
- @prot: contains the protection that will be applied by the kernel.
@@ -265,7 +274,7 @@ out:
- Return 0 on success, an error code on failure.
- (Based on the results of appraise_measurement().)
*/ -int ima_file_mmap(struct file *file, unsigned long prot) +int __ima_file_mmap(struct file *file, unsigned long prot) { int rc;
@@ -278,7 +287,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) }
/**
- ima_bprm_check - based on policy, collect/store measurement.
- __ima_bprm_check - based on policy, collect/store measurement.
- @bprm: contains the linux_binprm structure
- The OS protects against an executable file, already open for write,
@@ -290,7 +299,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
- Return 0 on success, an error code on failure.
- (Based on the results of appraise_measurement().)
*/ -int ima_bprm_check(struct linux_binprm *bprm) +int __ima_bprm_check(struct linux_binprm *bprm) { int rc;
@@ -300,7 +309,7 @@ int ima_bprm_check(struct linux_binprm *bprm) }
/**
- ima_path_check - based on policy, collect/store measurement.
- __ima_path_check - based on policy, collect/store measurement.
- @file: pointer to the file to be measured
- @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
@@ -309,7 +318,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
- Always return 0 and audit dentry_open failures.
- (Return code will be based upon measurement appraisal.)
*/ -int ima_file_check(struct file *file, int mask) +int __ima_file_check(struct file *file, int mask) { int rc;
@@ -318,12 +327,15 @@ int ima_file_check(struct file *file, int mask) FILE_CHECK); return 0; } -EXPORT_SYMBOL_GPL(ima_file_check); +EXPORT_SYMBOL_GPL(__ima_file_check);
static int __init init_ima(void) { int error;
- if (!ima_enabled)
return 0;
- error = ima_init(); ima_initialized = 1; return error;
"Mimi" == Mimi Zohar zohar@linux.vnet.ibm.com writes:
Mimi> On Mon, 2010-10-18 at 02:25 -0400, Kyle McMartin wrote:
If someone gives me a good reason why Fedora actually needs this enabled, I'm going to apply the following patch to our kernel so that IMA is globally an opt-in feature... Otherwise I'm inclined to just disable it.
Mimi> Am hoping others will chime in.
I'll chime in. I run Debian Squeeze and Ubuntu 10.10 and neither of them seem to have this monstrosity enabled at all, which is good. It should default to OFF and have a big fat warning saying it's a memory pig.
(But the more I look at this, the more it looks like a completely niche option that has little use outside of three-letter agencies.)
I regret not looking at this more closely when it was enabled, (although, in my defence, when the option first showed up, I left it off...)
It's probably way more heavyweight than necessary, as I think in most cases the iint_initalized test will cover the call into IMA. But better safe than sorry or something and there's a bunch of other cases where -ENOMEM will leak back up from failing kmem_cache_alloc, iirc.
regards, Kyle
Mimi> Thanks, will compile/test patch.
From what I'm reading, it's not even useful unless you have TPM
hardware?
I dunno... I'm just hesitant to use this or SElinux because the hassles are not worth the payoff.
John
On Sun, Oct 17, 2010 at 11:25 PM, Kyle McMartin kyle@mcmartin.ca wrote:
If someone gives me a good reason why Fedora actually needs this enabled, I'm going to apply the following patch to our kernel so that IMA is globally an opt-in feature... Otherwise I'm inclined to just disable it.
(But the more I look at this, the more it looks like a completely niche option that has little use outside of three-letter agencies.)
Not true. We'll be using it in MeeGo.
I regret not looking at this more closely when it was enabled, (although, in my defence, when the option first showed up, I left it off...)
It's probably way more heavyweight than necessary, as I think in most cases the iint_initalized test will cover the call into IMA. But better safe than sorry or something and there's a bunch of other cases where -ENOMEM will leak back up from failing kmem_cache_alloc, iirc.
Then let's bounce around ideas for making it more lightweight. I'd love to hear suggestions.
Ryan
regards, Kyle
From: Kyle McMartin kyle@mcmartin.ca Date: Mon, 18 Oct 2010 02:08:35 -0400 Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
Allow IMA to be entirely disabled, don't even bother calling into the provided hooks, and avoid initializing caches.
(A lot of the hooks will test iint_initialized, and so this doubly disables them, since the iint cache won't be enabled. But hey, we avoid a pointless branch...)
Signed-off-by: Kyle McMartin kyle@redhat.com
include/linux/ima.h | 66 +++++++++++++++++++++++++++++++++---- security/integrity/ima/ima_iint.c | 13 +++++-- security/integrity/ima/ima_main.c | 34 +++++++++++++------ 3 files changed, 91 insertions(+), 22 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h index 975837e..2fa456d 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -14,13 +14,65 @@ struct linux_binprm;
#ifdef CONFIG_IMA -extern int ima_bprm_check(struct linux_binprm *bprm); -extern int ima_inode_alloc(struct inode *inode); -extern void ima_inode_free(struct inode *inode); -extern int ima_file_check(struct file *file, int mask); -extern void ima_file_free(struct file *file); -extern int ima_file_mmap(struct file *file, unsigned long prot); -extern void ima_counts_get(struct file *file);
+extern int ima_enabled;
+extern int __ima_bprm_check(struct linux_binprm *bprm); +extern int __ima_inode_alloc(struct inode *inode); +extern void __ima_inode_free(struct inode *inode); +extern int __ima_file_check(struct file *file, int mask); +extern void __ima_file_free(struct file *file); +extern int __ima_file_mmap(struct file *file, unsigned long prot); +extern void __ima_counts_get(struct file *file);
+static inline int ima_bprm_check(struct linux_binprm *bprm) +{
- if (ima_enabled)
- return __ima_bprm_check(bprm);
- return 0;
+}
+static inline int ima_inode_alloc(struct inode *inode) +{
- if (ima_enabled)
- return __ima_inode_alloc(inode);
- return 0;
+}
+static inline void ima_inode_free(struct inode *inode) +{
- if (ima_enabled)
- __ima_inode_free(inode);
- return;
+}
+static inline int ima_file_check(struct file *file, int mask) +{
- if (ima_enabled)
- return __ima_file_check(file, mask);
- return 0;
+}
+static inline void ima_file_free(struct file *file) +{
- if (ima_enabled)
- __ima_file_free(file);
- return;
+}
+static inline int ima_file_mmap(struct file *file, unsigned long prot) +{
- if (ima_enabled)
- return __ima_file_mmap(file, prot);
- return 0;
+}
+static inline void ima_counts_get(struct file *file) +{
- if (ima_enabled)
- return __ima_counts_get(file);
- return;
+}
#else static inline int ima_bprm_check(struct linux_binprm *bprm) diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index afba4ae..767f026 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -46,10 +46,10 @@ out: }
/**
- ima_inode_alloc - allocate an iint associated with an inode
- __ima_inode_alloc - allocate an iint associated with an inode
* @inode: pointer to the inode */ -int ima_inode_alloc(struct inode *inode) +int __ima_inode_alloc(struct inode *inode) { struct ima_iint_cache *iint = NULL; int rc = 0; @@ -107,12 +107,12 @@ void iint_rcu_free(struct rcu_head *rcu_head) }
/**
- ima_inode_free - called on security_inode_free
- __ima_inode_free - called on security_inode_free
* @inode: pointer to the inode * * Free the integrity information(iint) associated with an inode. */ -void ima_inode_free(struct inode *inode) +void __ima_inode_free(struct inode *inode) { struct ima_iint_cache *iint;
@@ -139,6 +139,11 @@ static void init_once(void *foo)
static int __init ima_iintcache_init(void) {
- extern int ima_enabled;
- if (!ima_enabled)
- return 0;
iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index e662b89..92e084c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -26,6 +26,7 @@ #include "ima.h"
int ima_initialized; +int ima_enabled = 0;
char *ima_hash = "sha1"; static int __init hash_setup(char *str) @@ -36,6 +37,14 @@ static int __init hash_setup(char *str) } __setup("ima_hash=", hash_setup);
+static int __init ima_enable(char *str) +{
- if (strncmp(str, "on", 2) == 0)
- ima_enabled = 1;
- return 1;
+} +__setup("ima=", ima_enable);
struct ima_imbalance { struct hlist_node node; unsigned long fsmagic; @@ -130,7 +139,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode) }
/*
- ima_counts_get - increment file counts
- __ima_counts_get - increment file counts
* * Maintain read/write counters for all files, but only * invalidate the PCR for measured files: @@ -140,7 +149,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode) * could result in a file measurement error. * */ -void ima_counts_get(struct file *file) +void __ima_counts_get(struct file *file) { struct dentry *dentry = file->f_path.dentry; struct inode *inode = dentry->d_inode; @@ -204,13 +213,13 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode, }
/**
- ima_file_free - called on __fput()
- __ima_file_free - called on __fput()
* @file: pointer to file structure being freed * * Flag files that changed, based on i_version; * and decrement the iint readcount/writecount. */ -void ima_file_free(struct file *file) +void __ima_file_free(struct file *file) { struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint; @@ -255,7 +264,7 @@ out: }
/**
- ima_file_mmap - based on policy, collect/store measurement.
- __ima_file_mmap - based on policy, collect/store measurement.
* @file: pointer to the file to be measured (May be NULL) * @prot: contains the protection that will be applied by the kernel. * @@ -265,7 +274,7 @@ out: * Return 0 on success, an error code on failure. * (Based on the results of appraise_measurement().) */ -int ima_file_mmap(struct file *file, unsigned long prot) +int __ima_file_mmap(struct file *file, unsigned long prot) { int rc;
@@ -278,7 +287,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) }
/**
- ima_bprm_check - based on policy, collect/store measurement.
- __ima_bprm_check - based on policy, collect/store measurement.
* @bprm: contains the linux_binprm structure * * The OS protects against an executable file, already open for write, @@ -290,7 +299,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) * Return 0 on success, an error code on failure. * (Based on the results of appraise_measurement().) */ -int ima_bprm_check(struct linux_binprm *bprm) +int __ima_bprm_check(struct linux_binprm *bprm) { int rc;
@@ -300,7 +309,7 @@ int ima_bprm_check(struct linux_binprm *bprm) }
/**
- ima_path_check - based on policy, collect/store measurement.
- __ima_path_check - based on policy, collect/store measurement.
* @file: pointer to the file to be measured * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE * @@ -309,7 +318,7 @@ int ima_bprm_check(struct linux_binprm *bprm) * Always return 0 and audit dentry_open failures. * (Return code will be based upon measurement appraisal.) */ -int ima_file_check(struct file *file, int mask) +int __ima_file_check(struct file *file, int mask) { int rc;
@@ -318,12 +327,15 @@ int ima_file_check(struct file *file, int mask) FILE_CHECK); return 0; } -EXPORT_SYMBOL_GPL(ima_file_check); +EXPORT_SYMBOL_GPL(__ima_file_check);
static int __init init_ima(void) { int error;
- if (!ima_enabled)
- return 0;
error = ima_init(); ima_initialized = 1; return error; -- 1.7.3.1
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, 2010-10-18 at 02:25 -0400, Kyle McMartin wrote:
If someone gives me a good reason why Fedora actually needs this enabled, I'm going to apply the following patch to our kernel so that IMA is globally an opt-in feature... Otherwise I'm inclined to just disable it.
I'll can address this on the fedora list, but I think this is the wrong approach. IMA is supposed to be of negligible impact when not 'enabled' and I believe the right solution is to fix places where that isn't true. At the moment 3 have been identified.
1) IMA uses radix trees which end up wasting 500 bytes per inode because the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
2) IMA creates an entire integrity structure for every inode even when most or all of this structure will not be needed.
3) IMA serializes bringing inodes into and out of core because it takes a spinlock() when adding or removing the integrity structure to the radix/rbtree.
I've got a patch for #1 and think 2 and 3 are relatively simple to fix as well. Certainly #2 is, and I think that will lead to a solution to #3.
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index afba4ae..767f026 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c
@@ -139,6 +139,11 @@ static void init_once(void *foo)
static int __init ima_iintcache_init(void) {
- extern int ima_enabled;
extern in a .c file? didn't you already expose this in the appropriate .h file?
- if (!ima_enabled)
return 0;
- iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index e662b89..92e084c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -26,6 +26,7 @@ #include "ima.h"
int ima_initialized; +int ima_enabled = 0;
initializing a global?
char *ima_hash = "sha1"; static int __init hash_setup(char *str)
On Mon, Oct 18, 2010 at 12:48:54PM -0400, Eric Paris wrote:
I'll can address this on the fedora list, but I think this is the wrong approach. IMA is supposed to be of negligible impact when not 'enabled' and I believe the right solution is to fix places where that isn't true. At the moment 3 have been identified.
My beef is #2, which is what I want to see solved. If there's a million people using Fedora, and 2 people use IMA, that's an awful lot of bytes that could be otherwise used.
I think it should be entirely opt in, with a CONFIG_IMA_DEFAULT_ON or something like we do for security hooks.
Anyway, If you can address #2, then I'm happy having it enabled. If it's taken us this long to notice the impact, then it doesn't seem to be all that large in the general case, and if it can be reduced, then that should make everyone happy.
--Kyle
On Mon, Oct 18, 2010 at 12:48:54PM -0400, Eric Paris wrote:
A Red Hatter pointed out that ima_enabled needs to be exported for modular users of IMA (like NFS...) and also Eric's points lead me to think maybe just rolling them into the hooks might be a better solution in the interim for Fedora 14, and we'll pull in the rbtree and other fixes post-release and can talk about switching the toggle.
Thanks to you all for being so responsive about these issues.
regards, Kyle
---
security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_iint.c | 9 +++++++++ security/integrity/ima/ima_main.c | 24 +++++++++++++++++++++--- 3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 3fbcd1d..65c3977 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -37,6 +37,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; /* set during initialization */ extern int iint_initialized; extern int ima_initialized; +extern int ima_enabled; extern int ima_used_chip; extern char *ima_hash;
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index afba4ae..3d191ef 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -54,6 +54,9 @@ int ima_inode_alloc(struct inode *inode) struct ima_iint_cache *iint = NULL; int rc = 0;
+ if (!ima_enabled) + return 0; + iint = kmem_cache_alloc(iint_cache, GFP_NOFS); if (!iint) return -ENOMEM; @@ -116,6 +119,9 @@ void ima_inode_free(struct inode *inode) { struct ima_iint_cache *iint;
+ if (!ima_enabled) + return; + spin_lock(&ima_iint_lock); iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode); spin_unlock(&ima_iint_lock); @@ -139,6 +145,9 @@ static void init_once(void *foo)
static int __init ima_iintcache_init(void) { + if (!ima_enabled) + return 0; + iint_cache = kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0, SLAB_PANIC, init_once); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index e662b89..6e91905 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -26,6 +26,7 @@ #include "ima.h"
int ima_initialized; +int ima_enabled;
char *ima_hash = "sha1"; static int __init hash_setup(char *str) @@ -36,6 +37,14 @@ static int __init hash_setup(char *str) } __setup("ima_hash=", hash_setup);
+static int __init ima_enable(char *str) +{ + if (strncmp(str, "on", 2) == 0) + ima_enabled = 1; + return 1; +} +__setup("ima=", ima_enable); + struct ima_imbalance { struct hlist_node node; unsigned long fsmagic; @@ -148,7 +157,7 @@ void ima_counts_get(struct file *file) struct ima_iint_cache *iint; int rc;
- if (!iint_initialized || !S_ISREG(inode->i_mode)) + if (!ima_enabled || !iint_initialized || !S_ISREG(inode->i_mode)) return; iint = ima_iint_find_get(inode); if (!iint) @@ -215,7 +224,7 @@ void ima_file_free(struct file *file) struct inode *inode = file->f_dentry->d_inode; struct ima_iint_cache *iint;
- if (!iint_initialized || !S_ISREG(inode->i_mode)) + if (!ima_enabled || !iint_initialized || !S_ISREG(inode->i_mode)) return; iint = ima_iint_find_get(inode); if (!iint) @@ -269,7 +278,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) { int rc;
- if (!file) + if (!ima_enabled || !file) return 0; if (prot & PROT_EXEC) rc = process_measurement(file, file->f_dentry->d_name.name, @@ -294,6 +303,9 @@ int ima_bprm_check(struct linux_binprm *bprm) { int rc;
+ if (!ima_enabled) + return 0; + rc = process_measurement(bprm->file, bprm->filename, MAY_EXEC, BPRM_CHECK); return 0; @@ -313,6 +325,9 @@ int ima_file_check(struct file *file, int mask) { int rc;
+ if (!ima_enabled) + return 0; + rc = process_measurement(file, file->f_dentry->d_name.name, mask & (MAY_READ | MAY_WRITE | MAY_EXEC), FILE_CHECK); @@ -324,6 +339,9 @@ static int __init init_ima(void) { int error;
+ if (!ima_enabled) + return 0; + error = ima_init(); ima_initialized = 1; return error;
On Mon, Oct 18, 2010 at 9:48 AM, Eric Paris eparis@redhat.com wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because
the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Please. Look at the report more carefully.
The radix tree memory use is disgusting. Yes. But it is absolutely NOT sufficient to try to just fix that part. Go back, look at the original report email, and this line in particular:
2235648 2069791 92% 0.12K 69864 32 279456K iint_cache
There's 2.2 million iint_cache allocations too, each 128 bytes in size. That's still a quarter _gigabyte_ of crap that adds zero value at all.
Sure, memory is cheap, and this is on a machine with a lot of memory. But it has lots of memory because it wants to cache a lot of files, but because it wants to waste it on useless crap.
And yes, the radix tree usage is even worse, because radix trees are useful for densely clustered indexing, not sparse ones. But even if you were to get the indexing cost down to _zero_, it would still be unacceptable to waste memory like this for absolutely ZERO GAIN.
The IMA code clearly needs fixing at a much more fundamental level than just the indexing.
Linus
On Mon, 2010-10-18 at 10:56 -0700, Linus Torvalds wrote:
On Mon, Oct 18, 2010 at 9:48 AM, Eric Paris eparis@redhat.com wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because
the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Please. Look at the report more carefully.
The radix tree memory use is disgusting. Yes. But it is absolutely NOT sufficient to try to just fix that part. Go back, look at the original report email, and this line in particular:
2235648 2069791 92% 0.12K 69864 32 279456K iint_cache
There's 2.2 million iint_cache allocations too, each 128 bytes in size. That's still a quarter _gigabyte_ of crap that adds zero value at all.
That was #2 in my list of things to fix: 2) IMA creates an entire integrity structure for every inode even when most or all of this structure will not be needed.
I'm stating with #1 since that was 2G of wasted space (thus far my switch to rbtree seems to be surviving an xfstest) so I expect to send the patch this afternoon. #2 should attack the size of the iint_cache entries. #3 should attack the scalability. I'm certainly hoping I didn't miss part of the report....
-Eric
* Eric Paris eparis@redhat.com wrote:
On Mon, 2010-10-18 at 10:56 -0700, Linus Torvalds wrote:
On Mon, Oct 18, 2010 at 9:48 AM, Eric Paris eparis@redhat.com wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because
the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Please. Look at the report more carefully.
The radix tree memory use is disgusting. Yes. But it is absolutely NOT sufficient to try to just fix that part. Go back, look at the original report email, and this line in particular:
2235648 2069791 92% 0.12K 69864 32 279456K iint_cache
There's 2.2 million iint_cache allocations too, each 128 bytes in size. That's still a quarter _gigabyte_ of crap that adds zero value at all.
That was #2 in my list of things to fix:
- IMA creates an entire integrity structure for every inode even when most or all
of this structure will not be needed.
I'm stating with #1 since that was 2G of wasted space (thus far my switch to rbtree seems to be surviving an xfstest) so I expect to send the patch this afternoon. #2 should attack the size of the iint_cache entries. #3 should attack the scalability. I'm certainly hoping I didn't miss part of the report....
I think it would be fair to argue that #2 is the thing that should be fixed first and foremost - before touching any data structure details.
Because if you fix #2 then all the other items will become no-op to 99.9% of the people who are affected by this bug today.
It's also probably a much simpler fix for -stable, so should be done first, etc.
If you do the data structure changes first then #2 will likely not be backportable standalone and #1 will be risky to backport - creating nasty dependencies.
Thanks,
Ingo
On Mon, 2010-10-18 at 20:19 +0200, Ingo Molnar wrote:
- Eric Paris eparis@redhat.com wrote:
On Mon, 2010-10-18 at 10:56 -0700, Linus Torvalds wrote:
On Mon, Oct 18, 2010 at 9:48 AM, Eric Paris eparis@redhat.com wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because
the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Please. Look at the report more carefully.
The radix tree memory use is disgusting. Yes. But it is absolutely NOT sufficient to try to just fix that part. Go back, look at the original report email, and this line in particular:
2235648 2069791 92% 0.12K 69864 32 279456K iint_cache
There's 2.2 million iint_cache allocations too, each 128 bytes in size. That's still a quarter _gigabyte_ of crap that adds zero value at all.
That was #2 in my list of things to fix:
- IMA creates an entire integrity structure for every inode even when most or all
of this structure will not be needed.
I'm stating with #1 since that was 2G of wasted space (thus far my switch to rbtree seems to be surviving an xfstest) so I expect to send the patch this afternoon. #2 should attack the size of the iint_cache entries. #3 should attack the scalability. I'm certainly hoping I didn't miss part of the report....
I think it would be fair to argue that #2 is the thing that should be fixed first and foremost - before touching any data structure details.
Because if you fix #2 then all the other items will become no-op to 99.9% of the people who are affected by this bug today.
It's also probably a much simpler fix for -stable, so should be done first, etc.
If you do the data structure changes first then #2 will likely not be backportable standalone and #1 will be risky to backport - creating nasty dependencies.
Good point. I'll keep that in mind and possibly reorder.
-Eric
On Mon, 2010-10-18 at 20:19 +0200, Ingo Molnar wrote:
I think it would be fair to argue that #2 is the thing that should be fixed first and foremost - before touching any data structure details.
Because if you fix #2 then all the other items will become no-op to 99.9% of the people who are affected by this bug today.
Where I stand at the end of the day:
Executive summary for the TLDNR crowd: Before upcoming patch series IMA wasted 4,720k of memory on my test box when not configured to do anything. After patches IMA wastes 120k when not configured to do anything.
------------------------
I'm considering a system with 5000 inodes in core and 1500 inodes which IMA thinks should be measured (if it's on). (which just so happens to be close to the system I've been testing on shortly after reboot)
I'm going to consider 6 cases of memory usage and will post the patches shortly after this mail. My cases are going to be:
Linus - IMA Enabled Linus - IMA Disabled Allocate iint only when needed - IMA Enabled Allocate iint only when needed - IMA Disabled Allocate iint + RBTREE - IMA Enabled allocate iint + RBTREE - IMA Disabled
In each case I consider 'disabled' to be 'compiled in but you didn't tell it to do anything.'
So for Linus: sizeof(iint) = 312 sizeof(radix) = 632 sizeof(inode delta) = 0 (how much I grew struct inode)
Given my scenario of a stock F14ish machine where 5000 inodes in core and 1500 are IMA relevant when enabled we end up with:
Linus - Enabled Linus - Disabled ------------------ ---------------- iint_cache = 1,560k iint_cache = 1,560k radix = 3,160k radix = 3,160k inode d = 0 inode d = 0
total = 4,720k total = 4,720k
For Allocate iint only when needed: sizeof(iint) = 288 sizeof(radix) = 632 sizeof(inode delta) = 24 (24 bytes from iint move to struct inode)
Allocate iint - Enabled Alloce iint - Disabled ----------------------- ---------------------- iint_cache = 342k iint_cache = 0 radix = 948k radix = 0 inode d = 120k inode d = 120k
total = 1,410k total = 120k
For allocate iint only when needed and use rbtrees: sizeof(iint) = 320 sizeof(radix) = 632 (but irrelevant) sizeof(inode delta) = 24
Allocate + RBTREE - Enabled Allocate iint + RBTREE - Disabled --------------------------- --------------------------------- iint_cache = 480k iint_cache = 0 radix = 0 radix = 0 inode d = 120k inode d = 120k
total = 600k total = 120k
Seems like about the best we can do. This patch series attempts to addresses all 3 of the problems I believe we identified (we still serialize IMA relevant inodes but not the majority of them and none when IMA is not enabled)
IMA will continue to waste 24 bytes per inode in core even when it isn't doing anything useful just by compiling it in. Future work to use a freezer could get rid of this if the complexity is worth the tradeoff. But I don't think it's worth it tonight.
-Eric
On 10/18/2010 09:48 AM, Eric Paris wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because
the key is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Radix trees can efficiently store data associated with sparse keys *as long as the keys are clustered*. For random key distributions, they perform horribly.
-hpa
* H. Peter Anvin hpa@zytor.com wrote:
On 10/18/2010 09:48 AM, Eric Paris wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because the key
is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Radix trees can efficiently store data associated with sparse keys *as long as the keys are clustered*. For random key distributions, they perform horribly.
For random key distributions hash and rbtree data structures are pretty good choices.
But the (much) more fundamental question is to turn the non-trivial allocation overhead of this opt-in feature into truly opt-in overhead.
Thanks,
Ingo
On 10/18/2010 11:11 AM, Ingo Molnar wrote:
- H. Peter Anvin hpa@zytor.com wrote:
On 10/18/2010 09:48 AM, Eric Paris wrote:
- IMA uses radix trees which end up wasting 500 bytes per inode because the key
is too sparse. I've got a patch which uses an rbtree instead I'm testing and will send along shortly. I found it funny working on the patch to see that Documentation/rbtree.txt says "This differs from radix trees (which are used to efficiently store sparse arrays and thus use long integer indexes to insert/access/delete nodes)" Which flys in the face of this report.
Radix trees can efficiently store data associated with sparse keys *as long as the keys are clustered*. For random key distributions, they perform horribly.
For random key distributions hash and rbtree data structures are pretty good choices.
But the (much) more fundamental question is to turn the non-trivial allocation overhead of this opt-in feature into truly opt-in overhead.
Yes, and not just the allocation overhead, but apparently locking overhead, too.
-hpa
Hi!
Especially as our merge requirements for security/ are a lot lower than for the rest of the kernel given that James is very afraid of getting whacked by Linux for not mering things.
I think historically you'll see that I'm not afraid of getting whacked by Linus.
A procedure for merging security features has been adopted by consensus, based on suggestions from Arjan, with the aim of preventing the literally endless arguments which arise from security feature discussions. It's sometimes referred to as the Arjan protocol, essentially:
If the feature correctly implements a well-defined security goal, meets user needs without incurring unreasonable overheads, passes technical review, and is supported by competent developers, then it is likely to be merged.
If you disagree with a specific feature, you need to step up while it's being reviewed and make a case against it according to the above criteria.
Well, I'm arguing that the criteria are wrong. Duplicated crap is creeping in (TOMOYO vs. AppArmor), and strange stuff that has no legitimate use is in (IMA -- what is it good for? locking machines down, iPhone style).
If you disagree with the protocol, then you need to come up with a better one, and probably implement it yourself, to the satisfaction of all parties.
I do disagree, and I do not think 'satistfaction of all parties' is reasonable goal. Rest of kernel has different rules, and IMO they are better.
kernel@lists.fedoraproject.org