[PATCH 0/2] ima: policy search speedup

[PATCH 0/2] ima: policy search speedup

[Dmitry Kasatkin]

Hello,

Here is two patches for policy search speedup.

First patch adds additional features flags to superblock.
Second - implementation for IMA.

Two months ago I was asking about it on mailing lists.
Suggestion was not to use s_flags, but e.g. s_feature_flags.

Any objections about such approach?

Thanks,
Dmitry

Dmitry Kasatkin (2):
  vfs: new super block feature flags attribute
  ima: skip policy search for never appraised or measured files

 include/linux/fs.h                  |    4 ++++
 security/integrity/ima/ima_api.c    |    8 ++------
 security/integrity/ima/ima_policy.c |   20 +++++++++++++++++---
 security/integrity/integrity.h      |    3 +++
 4 files changed, 26 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] vfs: new super block feature flags attribute

[Dmitry Kasatkin]

This patch introduces new super block attribute flag s_feature_flags
and SF_IMA_DISABLED flag. This flag will be used by Integrity Measurement
Architecture (IMA). Name suggested by Bruce Fields.

Certain file system types and partitions will never be measured or
appraised by IMA depending on the policy. For example, pseudo file
systems are never measured and appraised. In current implementation
policy will be checked again and again. It happens thousands times
per second. That is absolute waste of CPU and may be battery resources.

IMA will set the SF_IMA_DISABLED flag when file system will not be measured
and appraised and test this flag during subsequent calls to skip policy search.

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
 include/linux/fs.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index b33cfc9..0bef2b2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1321,6 +1321,8 @@ struct super_block {
 
 	/* Being remounted read-only */
 	int s_readonly_remount;
+
+	unsigned long s_feature_flags;
 };
 
 /* superblock cache pruning functions */
@@ -1746,6 +1748,8 @@ struct super_operations {
 
 #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
 
+#define SF_IMA_DISABLED		0x0001
+
 extern void __mark_inode_dirty(struct inode *, int);
 static inline void mark_inode_dirty(struct inode *inode)
 {
-- 
1.7.10.4

[PATCH 2/2] ima: skip policy search for never appraised or measured files

[Dmitry Kasatkin]

Certain file system types and partitions will never be measured or
appraised by IMA depending on the policy. For example, pseudo file
systems are never measured and appraised. In current implementation
policy will be checked again and again. It happens thousands times
per second. That is absolute waste of CPU and may be battery resources.

This patch uses new super block SF_IMA_DISABLED flag. IMA set the
SF_IMA_DISABLED flag when file system will not be measured and
appraised and test this flag during subsequent calls to skip policy
search.

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
 security/integrity/ima/ima_api.c    |    8 ++------
 security/integrity/ima/ima_policy.c |   20 +++++++++++++++++---
 security/integrity/integrity.h      |    3 +++
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index b356884..2156020 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -114,12 +114,8 @@ err_out:
  */
 int ima_get_action(struct inode *inode, int mask, int function)
 {
-	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE;
-
-	if (!ima_appraise)
-		flags &= ~IMA_APPRAISE;
-
-	return ima_match_policy(inode, function, mask, flags);
+	return ima_match_policy(inode, function, mask,
+				IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE);
 }
 
 int ima_must_measure(struct inode *inode, int mask, int function)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c7dacd2..a68ea55 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -22,7 +22,6 @@
 /* flags definitions */
 #define IMA_FUNC 	0x0001
 #define IMA_MASK 	0x0002
-#define IMA_FSMAGIC	0x0004
 #define IMA_UID		0x0008
 #define IMA_FOWNER	0x0010
 
@@ -198,7 +197,16 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		     int flags)
 {
 	struct ima_rule_entry *entry;
-	int action = 0, actmask = flags | (flags << 1);
+	int all_actions = (flags == IMA_DO_MASK);
+	int action = 0, actmask;
+
+	if (inode->i_sb->s_feature_flags & SF_IMA_DISABLED)
+		return 0;
+
+	if (!ima_appraise)
+		flags &= ~IMA_APPRAISE;
+
+	actmask = flags | (flags << 1);
 
 	list_for_each_entry(entry, ima_rules, list) {
 
@@ -208,6 +216,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		if (!ima_match_rules(entry, inode, func, mask))
 			continue;
 
+		action |= entry->flags & IMA_ACTION_FLAGS;
 		action |= entry->action & IMA_DO_MASK;
 		if (entry->action & IMA_DO_MASK)
 			actmask &= ~(entry->action | entry->action << 1);
@@ -217,7 +226,12 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		if (!actmask)
 			break;
 	}
-
+	if (all_actions && (action & IMA_FS_MASK)) {
+		action &= ~IMA_FS_MASK;
+		/* dont_measure, dont_audit and dont_appraise */
+		if (!action)
+			inode->i_sb->s_feature_flags |= SF_IMA_DISABLED;
+	}
 	return action;
 }
 
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e9db763..d67d867 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -26,7 +26,10 @@
 #define IMA_AUDITED		0x0080
 
 /* iint cache flags */
+#define IMA_ACTION_FLAGS	0xff00
 #define IMA_DIGSIG		0x0100
+#define IMA_FSMAGIC		0x0200
+#define IMA_FS_MASK		IMA_FSMAGIC
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT)
 #define IMA_DONE_MASK		(IMA_MEASURED | IMA_APPRAISED | IMA_AUDITED \
-- 
1.7.10.4

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

Hello,

Any thoughts about this proposal?

- Dmitry

On Thu, Nov 22, 2012 at 11:54 PM, Dmitry Kasatkin
<dmitry.kasatkin@intel.com> wrote:
> Hello,
>
> Here is two patches for policy search speedup.
>
> First patch adds additional features flags to superblock.
> Second - implementation for IMA.
>
> Two months ago I was asking about it on mailing lists.
> Suggestion was not to use s_flags, but e.g. s_feature_flags.
>
> Any objections about such approach?
>
> Thanks,
> Dmitry
>
> Dmitry Kasatkin (2):
>   vfs: new super block feature flags attribute
>   ima: skip policy search for never appraised or measured files
>
>  include/linux/fs.h                  |    4 ++++
>  security/integrity/ima/ima_api.c    |    8 ++------
>  security/integrity/ima/ima_policy.c |   20 +++++++++++++++++---
>  security/integrity/integrity.h      |    3 +++
>  4 files changed, 26 insertions(+), 9 deletions(-)
>
> --
> 1.7.10.4
>

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

Hello Linus,

Can you please comment on the feature flag in this patchset?

Thanks,
Dmitry


On Tue, Nov 27, 2012 at 3:42 PM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:
> Hello,
>
> Any thoughts about this proposal?
>
> - Dmitry
>
> On Thu, Nov 22, 2012 at 11:54 PM, Dmitry Kasatkin
> <dmitry.kasatkin@intel.com> wrote:
>> Hello,
>>
>> Here is two patches for policy search speedup.
>>
>> First patch adds additional features flags to superblock.
>> Second - implementation for IMA.
>>
>> Two months ago I was asking about it on mailing lists.
>> Suggestion was not to use s_flags, but e.g. s_feature_flags.
>>
>> Any objections about such approach?
>>
>> Thanks,
>> Dmitry
>>
>> Dmitry Kasatkin (2):
>>   vfs: new super block feature flags attribute
>>   ima: skip policy search for never appraised or measured files
>>
>>  include/linux/fs.h                  |    4 ++++
>>  security/integrity/ima/ima_api.c    |    8 ++------
>>  security/integrity/ima/ima_policy.c |   20 +++++++++++++++++---
>>  security/integrity/integrity.h      |    3 +++
>>  4 files changed, 26 insertions(+), 9 deletions(-)
>>
>> --
>> 1.7.10.4
>>

Re: [PATCH 0/2] ima: policy search speedup

[Mimi Zohar]

On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote:

> >> Here is two patches for policy search speedup.
> >>
> >> First patch adds additional features flags to superblock.
> >> Second - implementation for IMA.
> >>
> >> Two months ago I was asking about it on mailing lists.
> >> Suggestion was not to use s_flags, but e.g. s_feature_flags.
> >>
> >> Any objections about such approach?

First of all my appologies for not responding earlier.  Based on the
previous discussion https://lkml.org/lkml/2012/9/19/9, this looks like
the right direction.  Specific comments will be posted on the individual
patches.

thanks,

Mimi

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote:
>> >>
>> >> Two months ago I was asking about it on mailing lists.
>> >> Suggestion was not to use s_flags, but e.g. s_feature_flags.

Quite frankly, this seems stupid.

Without really knowing the problem space, the sane thing to do would
seem to be inode->i_flags. At which point it's

 (a) faster to test (no need to dereference inode->i_sb)

 (b) matches what the integrity layer does with S_IMA (well, there the
logic is reversed: S_IMA means that it has a integrity structure
associated with it)

 (c) allows you to mark individual inodes as "no checking".

and quite frankly, (c) in particular seems to make sense to me, since
it would seem to be rather possible to do things like "I've checked
this inode, it had no policies associated with it, I never need to
check it again". Clear the flag when policies change or whatever.

What's the advantage of making it per-filesystem?

            Linus

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 6:59 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote:
>>> >>
>>> >> Two months ago I was asking about it on mailing lists.
>>> >> Suggestion was not to use s_flags, but e.g. s_feature_flags.
>
> Quite frankly, this seems stupid.

What exactly seems stupid here?

>
> Without really knowing the problem space, the sane thing to do would
> seem to be inode->i_flags. At which point it's
>
>  (a) faster to test (no need to dereference inode->i_sb)
>
>  (b) matches what the integrity layer does with S_IMA (well, there the
> logic is reversed: S_IMA means that it has a integrity structure
> associated with it)
>
>  (c) allows you to mark individual inodes as "no checking".
>

There are inode specific objects which IMA uses for such perpose.

> and quite frankly, (c) in particular seems to make sense to me, since
> it would seem to be rather possible to do things like "I've checked
> this inode, it had no policies associated with it, I never need to
> check it again". Clear the flag when policies change or whatever.
>
> What's the advantage of making it per-filesystem?
>

There are different filesystems which are not checked by IMA/EVM,
such as pseudo-filesystems. For this reason it is good to have a way to
ignore such filesystems without to much work in IMA code.
No reason to check policy again and again for every inode on the filesystem
when the result will always be to ignore the filesystem.

Per-filesystem flag soles this problem.

- Dmitry


>             Linus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:
>>
>> Quite frankly, this seems stupid.
>
> What exactly seems stupid here?

What I said. Go back and read it. I gave three reasons. Why do you ask?

I'll give one more reason, but you probably won't read *this* email
either, will you?

> There are different filesystems which are not checked by IMA/EVM,
> such as pseudo-filesystems.

Did you read my email?

There are probably *also* individual that aren't checked by IMA/EVM
even on filesystems that *do* check other files.

No?

And your "pseudo-filesystems" argument is pretty stupid too, since WE
ALREADY HAVE A FLAG FOR THAT!

Guess where it is? Oh, it's in the place I already mentioned makes
more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
users. It's what the other security models already use to avoid
bothering calling down to the security layers. The fact that the
integrity layer bypasses the normal security layer in
ima_file_check(), for example, is no excuse to then make up totally
new flags.

So let me repeat: adding a new superblock flag seems STUPID. Why is it
in a completely different place than all the other flags that we
already have for these kinds of things? Why should we add a new field,
when we have existing fields that seem to do exactly this, do it
better, and are already used?

And don't ask me why without reading this email. OK?

                 Linus

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> And your "pseudo-filesystems" argument is pretty stupid too, since WE
> ALREADY HAVE A FLAG FOR THAT!
>
> Guess where it is? Oh, it's in the place I already mentioned makes
> more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
> users. It's what the other security models already use to avoid
> bothering calling down to the security layers. The fact that the
> integrity layer bypasses the normal security layer in
> ima_file_check(), for example, is no excuse to then make up totally
> new flags.

IS_PRIVATE() is not used by and darn well better not be used by, all
psuedo filesystems like procfs which IMA may want to ignore.  LSMs
like to do control on them.  I thought S_PRIVATE was really only used
by the anon_inode and reiserfs's really crazy ass internal inodes.  I
could always be wrong.

I don't know if I agree with dmitry but let me explain what's going on here.

Lets say the user accesses an inode in procfs.  Without this patch one
would search the ima policy and find a rule that says 'if this inode
is on procfs we don't care.'  We can then cache that in the struct
inode like you say and move along.  If another inode on procfs is
opened we will have the same policy search and the same per inode 'i
don't give a crap' marking.  This absolutely works you are right.  But
we search the IMA policy for every inode.

With Dmitry's patch we can instead search the IMA policy one time and
mark the whole superblock as 'i don't give a crap' if IMA policy says
we don't care about that fstype.  When the second procfs inode is
opened we instead look at the per superblock 'who gives a crap' and
never search the IMA policy.  So we save all future policy searches.

I'd say this patch would only be a good idea if there is a real
performance hit which is measurable in a real work situation.  Not,
'look how much faster it is to access proc inodes' microbenchmark,
since noone is actually going to do that, but some results of a useful
benchmark you care about.  Maybe Dmitry gave those numbers and I
missed them?  Otherwise, stick with per inode like Linus wants...

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 7:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
>>>
>>> Quite frankly, this seems stupid.
>>
>> What exactly seems stupid here?
>
> What I said. Go back and read it. I gave three reasons. Why do you ask?
>
> I'll give one more reason, but you probably won't read *this* email
> either, will you?
>

No comments. Should I say more here?
Better read further...

>> There are different filesystems which are not checked by IMA/EVM,
>> such as pseudo-filesystems.
>
> Did you read my email?
>
> There are probably *also* individual that aren't checked by IMA/EVM
> even on filesystems that *do* check other files.
>
> No?
>
> And your "pseudo-filesystems" argument is pretty stupid too, since WE
> ALREADY HAVE A FLAG FOR THAT!
>
> Guess where it is? Oh, it's in the place I already mentioned makes
> more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
> users. It's what the other security models already use to avoid
> bothering calling down to the security layers. The fact that the
> integrity layer bypasses the normal security layer in
> ima_file_check(), for example, is no excuse to then make up totally
> new flags.
>
> So let me repeat: adding a new superblock flag seems STUPID. Why is it
> in a completely different place than all the other flags that we
> already have for these kinds of things? Why should we add a new field,
> when we have existing fields that seem to do exactly this, do it
> better, and are already used?
>

I was suggesting to add a flag for sb->s_flags field.
Al said that he would not like this, because s_flags are mount specific.
One of suggestions in response was to use s_feature flags, because other FSs
would benefit from that as well..

https://lkml.org/lkml/2012/9/19/460


> And don't ask me why without reading this email. OK?

Again, no comments.


>
>                  Linus

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 7:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
>>>
>>> Quite frankly, this seems stupid.
>>
>> What exactly seems stupid here?
>
> What I said. Go back and read it. I gave three reasons. Why do you ask?
>
> I'll give one more reason, but you probably won't read *this* email
> either, will you?
>
>> There are different filesystems which are not checked by IMA/EVM,
>> such as pseudo-filesystems.
>
> Did you read my email?
>
> There are probably *also* individual that aren't checked by IMA/EVM
> even on filesystems that *do* check other files.
>
> No?
>
> And your "pseudo-filesystems" argument is pretty stupid too, since WE
> ALREADY HAVE A FLAG FOR THAT!
>
> Guess where it is? Oh, it's in the place I already mentioned makes
> more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
> users. It's what the other security models already use to avoid
> bothering calling down to the security layers. The fact that the
> integrity layer bypasses the normal security layer in
> ima_file_check(), for example, is no excuse to then make up totally
> new flags.

Actually S_PRIVATE does not work work for normal filesystems which IMA
might want to ignore.

>
> So let me repeat: adding a new superblock flag seems STUPID. Why is it
> in a completely different place than all the other flags that we
> already have for these kinds of things? Why should we add a new field,
> when we have existing fields that seem to do exactly this, do it
> better, and are already used?
>
> And don't ask me why without reading this email. OK?
>
>                  Linus

Re: [PATCH 0/2] ima: policy search speedup

[Mimi Zohar]

On Tue, 2012-12-11 at 08:59 -0800, Linus Torvalds wrote:
> On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote:
> >> >>
> >> >> Two months ago I was asking about it on mailing lists.
> >> >> Suggestion was not to use s_flags, but e.g. s_feature_flags.
> 
> Quite frankly, this seems stupid.
> 
> Without really knowing the problem space, the sane thing to do would
> seem to be inode->i_flags. At which point it's
> 
>  (a) faster to test (no need to dereference inode->i_sb)
> 
>  (b) matches what the integrity layer does with S_IMA (well, there the
> logic is reversed: S_IMA means that it has a integrity structure
> associated with it)
> 
>  (c) allows you to mark individual inodes as "no checking".

The appraisal policy is based on the object metadata, such as the uid,
so the result is static and can be cached.  The measurement policy, on
the other hand, is normally based on the subject (eg. who is
reading/executing) the file.  Knowledge of whether the file has been
measured is cached in the iint, but unlike the appraisal policy, not
whether it needs to be measured.  Having the flag on a per inode basis,
doesn't really help.

thanks,

Mimi

Re: [PATCH 0/2] ima: policy search speedup

[Al Viro]

On Tue, Dec 11, 2012 at 08:10:07PM +0200, Kasatkin, Dmitry wrote:

> I was suggesting to add a flag for sb->s_flags field.
> Al said that he would not like this, because s_flags are mount specific.

s/mount-specific/in an incestous relationship with mount(2) ABI/.

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 8:09 PM, Eric Paris <eparis@parisplace.org> wrote:
> On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>
>> And your "pseudo-filesystems" argument is pretty stupid too, since WE
>> ALREADY HAVE A FLAG FOR THAT!
>>
>> Guess where it is? Oh, it's in the place I already mentioned makes
>> more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
>> users. It's what the other security models already use to avoid
>> bothering calling down to the security layers. The fact that the
>> integrity layer bypasses the normal security layer in
>> ima_file_check(), for example, is no excuse to then make up totally
>> new flags.
>
> IS_PRIVATE() is not used by and darn well better not be used by, all
> psuedo filesystems like procfs which IMA may want to ignore.  LSMs
> like to do control on them.  I thought S_PRIVATE was really only used
> by the anon_inode and reiserfs's really crazy ass internal inodes.  I
> could always be wrong.
>
> I don't know if I agree with dmitry but let me explain what's going on here.
>
> Lets say the user accesses an inode in procfs.  Without this patch one
> would search the ima policy and find a rule that says 'if this inode
> is on procfs we don't care.'  We can then cache that in the struct
> inode like you say and move along.  If another inode on procfs is
> opened we will have the same policy search and the same per inode 'i
> don't give a crap' marking.  This absolutely works you are right.  But
> we search the IMA policy for every inode.
>
> With Dmitry's patch we can instead search the IMA policy one time and
> mark the whole superblock as 'i don't give a crap' if IMA policy says
> we don't care about that fstype.  When the second procfs inode is
> opened we instead look at the per superblock 'who gives a crap' and
> never search the IMA policy.  So we save all future policy searches.
>
> I'd say this patch would only be a good idea if there is a real
> performance hit which is measurable in a real work situation.  Not,
> 'look how much faster it is to access proc inodes' microbenchmark,
> since noone is actually going to do that, but some results of a useful
> benchmark you care about.  Maybe Dmitry gave those numbers and I
> missed them?  Otherwise, stick with per inode like Linus wants...

I was measuring that during on the system without super block flag,
policy search was happening 100 000 times, but with the flag just bellow 10 000.
For desktop multi-core systems powered from the plug it might be unnoticeable.
But for the handhald it might save the battery.

- Dmitry

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

On Tue, Dec 11, 2012 at 1:18 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> The appraisal policy is based on the object metadata, such as the uid,
> so the result is static and can be cached.  The measurement policy, on
> the other hand, is normally based on the subject (eg. who is
> reading/executing) the file.  Knowledge of whether the file has been
> measured is cached in the iint, but unlike the appraisal policy, not
> whether it needs to be measured.  Having the flag on a per inode basis,
> doesn't really help.

Can you try again?  Even I can't parse this.  Not sure what to tell
you to try again, maybe give us a summary at a high level again and
then why this patch is specifically necessary?

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 10:12 AM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:
>
> Actually S_PRIVATE does not work work for normal filesystems which IMA
> might want to ignore.

The reading comprehension here is abysmal.

First you claim that you need the new flag for pseudo-filesystems, and
now that I point out that we have an *old* flag for pseudo-filesystems
you turn around 180 degrees and talk about other filesystems.

And none of that matters for my argument AT ALL.

My argument has not been that we cannot add a new flag.

My argument has been that we already have the logical place for such a
flag, and that adding a totally new field seems so stupid.

Seriously. The i_flags place is where we already do pretty much
*exactly* what you ask for. The fact that it is faster and more
flexible to boot should be a bonus.

Now, there are real reasons to avoid "s_flags", notably the fact that
we're running out of bits there (unlike i_flags), and they are exposed
as generic fields and are generally meant for mount options etc. So I
understand why we might want to avoid that (although the whole
mount-option thing could also be seen as an advantage), but I really
don't see any argument against i_flags, considering that we already
use it for S_IMA and S_PRIVATE, both of which are related to exactly
what you seem to want to do.

The one downside of i_flags may be that any update should own the
inode semaphore. But within the context of a security model, that
should be fine (and normally you'd update it once per lifetime of the
inode).

                  Linus

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 8:35 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 11, 2012 at 10:12 AM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
>>
>> Actually S_PRIVATE does not work work for normal filesystems which IMA
>> might want to ignore.
>
> The reading comprehension here is abysmal.
>
> First you claim that you need the new flag for pseudo-filesystems, and
> now that I point out that we have an *old* flag for pseudo-filesystems
> you turn around 180 degrees and talk about other filesystems.
>

I have not claimed that. This is exactly what I have written:
"There are different filesystems which are not checked by IMA/EVM,
such as pseudo-filesystems"
Pseudo-filesystems was an example of possible cases.
Sorry if it was not clear enough.

> And none of that matters for my argument AT ALL.
>
> My argument has not been that we cannot add a new flag.
>
> My argument has been that we already have the logical place for such a
> flag, and that adding a totally new field seems so stupid.
>
> Seriously. The i_flags place is where we already do pretty much
> *exactly* what you ask for. The fact that it is faster and more
> flexible to boot should be a bonus.
>
> Now, there are real reasons to avoid "s_flags", notably the fact that
> we're running out of bits there (unlike i_flags), and they are exposed
> as generic fields and are generally meant for mount options etc. So I
> understand why we might want to avoid that (although the whole
> mount-option thing could also be seen as an advantage), but I really
> don't see any argument against i_flags, considering that we already
> use it for S_IMA and S_PRIVATE, both of which are related to exactly
> what you seem to want to do.

Not every inode on the filesystem might be checked by IMA.
It depends on policy. And S_IMA flag is used exactly for this purpose.

But when the entire FS is not checked, SB flag seems to be very appropriate.
Eric has given nice explanation.

>
> The one downside of i_flags may be that any update should own the
> inode semaphore. But within the context of a security model, that
> should be fine (and normally you'd update it once per lifetime of the
> inode).
>

This is exactly how IMA works at the moment.
See my response to Eric about performance.

>                   Linus

Re: [PATCH 0/2] ima: policy search speedup

[Mimi Zohar]

On Tue, 2012-12-11 at 13:35 -0500, Eric Paris wrote:
> On Tue, Dec 11, 2012 at 1:18 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > The appraisal policy is based on the object metadata, such as the uid,
> > so the result is static and can be cached.  The measurement policy, on
> > the other hand, is normally based on the subject (eg. who is
> > reading/executing) the file.  Knowledge of whether the file has been
> > measured is cached in the iint, but unlike the appraisal policy, not
> > whether it needs to be measured.  Having the flag on a per inode basis,
> > doesn't really help.
> 
> Can you try again?  Even I can't parse this.  Not sure what to tell
> you to try again, maybe give us a summary at a high level again and
> then why this patch is specifically necessary?

sigh. The IMA policy contains rules for the original IMA measurement,
hash auditing, and now for IMA appraisal.  These policies overlap with
each other, but are not the same.

Although the policy doesn't change, the rules can be dependent on the
calling process.  For example, the original default 'ima_tcb' policy is
based, not on the file owner, but on the uid reading/executing the file.
The 'ima_tcb' policy measures all files read/executed by root.  So we
cache whether the file has been measured, not if the file needs to be
measured, because depending on the caller, that changes.  Bottom line,
we can't say definitively whether or not a file needs to be measured for
any caller.

Dmitry's patch addresses the issue of eliminating an entire filesystem
from being appraised, measured, or audited.

I hope this clarifies the issues a bit better.

Mimi

 

Re: [PATCH 0/2] ima: policy search speedup

[Mimi Zohar]

On Tue, 2012-12-11 at 13:09 -0500, Eric Paris wrote:
> On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> 
> > And your "pseudo-filesystems" argument is pretty stupid too, since WE
> > ALREADY HAVE A FLAG FOR THAT!
> >
> > Guess where it is? Oh, it's in the place I already mentioned makes
> > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
> > users. It's what the other security models already use to avoid
> > bothering calling down to the security layers. The fact that the
> > integrity layer bypasses the normal security layer in
> > ima_file_check(), for example, is no excuse to then make up totally
> > new flags.
> 
> IS_PRIVATE() is not used by and darn well better not be used by, all
> psuedo filesystems like procfs which IMA may want to ignore.  LSMs
> like to do control on them.  I thought S_PRIVATE was really only used
> by the anon_inode and reiserfs's really crazy ass internal inodes.  I
> could always be wrong.

I was actually wondering about the MS_NOSEC flag.  It's currently being
used by fuse, gfs2, ocfs2 and tmpfs.  (Not sure about xfs.)  Can someone
explain what it is being used for?

thanks,

Mimi

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 10:59 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
>  Bottom line,
> we can't say definitively whether or not a file needs to be measured for
> any caller.

.. but is that actually *always* the case? Is there some fundamental
reason why integrity rules can never have "this file just doesn't
matter"?

For example, if you have cases where you know that a file has a
particular policy (or no policy what-so-ever) that never cares, then
for that file you can say "don't bother measuring this file" even
though in the generic case that may not be true.

Things like that might be the common case. Like "normal files owned by
uid > 500 are user files, and we simply don't care, and fall back to
just normal unix permissions".

And yes, things like that may end up requiring that the flag be
cleared on other inode events (ie changing file ownership of
executable flags or whatever, which might change a file from "don't
bother" to "hey, now it might be interesting again"). So maybe
chmod/chown/chgrp would clear that flag..

Anyway, the whole "you can do it at file granularity" isn't the bulk
of my argument (the "we already have the field that makes sense" is).
But my point is that per-inode is not only the logically more
straightforward place to do it, it's also the much more flexible place
to do it. Because it *allows* for things like that.

                  Linus

Re: [PATCH 0/2] ima: policy search speedup

[Mimi Zohar]

On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote:

> Anyway, the whole "you can do it at file granularity" isn't the bulk
> of my argument (the "we already have the field that makes sense" is).
> But my point is that per-inode is not only the logically more
> straightforward place to do it, it's also the much more flexible place
> to do it. Because it *allows* for things like that.

Ok. To summarize, S_IMA indicates that there is a rule and that the iint
was allocated.  To differentiate between 'haven't looked/don't know' and
'definitely not', we need another bit.  For this, you're suggesting
using IS_PRIVATE()?  Hopefully, I misunderstood.

thanks,

Mimi

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 11:48 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> Ok. To summarize, S_IMA indicates that there is a rule and that the iint
> was allocated.  To differentiate between 'haven't looked/don't know' and
> 'definitely not', we need another bit.  For this, you're suggesting
> using IS_PRIVATE()?  Hopefully, I misunderstood.

No, for that, I'm suggesting using a new bit in i_flags.

The "IS_PRIVATE()" thing is more a "if you know a-priori that you
aren't interested in pseudo-filesystems, you can already check that
bit, because it will be set for things like /proc and shmem mappings
and pipes etc".

Dmitry seemed to imply that the biggest use for the new bit was for
taking out whole pseudo-filesystems in one go. That would pretty much
be what S_PRIVATE is.

                  Linus

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

S_PRIVATE is totally unacceptable as it has a meaning across all LSMs,
not just IMA.

S_NOSEC means 'this is not setuid or setgid and we don't need to do
those checks on modify'

You are going to need to use a S_NOIMA.

Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how
many of them are the same inode over and over again which would be
circumvented using S_NOIMA per inode flag?



On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote:
>
>> Anyway, the whole "you can do it at file granularity" isn't the bulk
>> of my argument (the "we already have the field that makes sense" is).
>> But my point is that per-inode is not only the logically more
>> straightforward place to do it, it's also the much more flexible place
>> to do it. Because it *allows* for things like that.
>
> Ok. To summarize, S_IMA indicates that there is a rule and that the iint
> was allocated.  To differentiate between 'haven't looked/don't know' and
> 'definitely not', we need another bit.  For this, you're suggesting
> using IS_PRIVATE()?  Hopefully, I misunderstood.
>
> thanks,
>
> Mimi
>
>
>

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

On Tue, Dec 11, 2012 at 3:05 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> The "IS_PRIVATE()" thing is more a "if you know a-priori that you
> aren't interested in pseudo-filesystems, you can already check that
> bit, because it will be set for things like /proc and shmem mappings
> and pipes etc".

I know it isn't relevant to the final solution, but this is simply
wrong.  IS_PRIVATE() means 'this inode is filesystem internal.'  It is
not used by anything except rieser and the anon_inode.  If it is used
by psuedo filesystems in general, like /proc, shmem mappings, and
pipes that is a huge bug and is absolutely wrong.

You are correct IS_PRIVATE is sufficient to know you don't need to do
any IMA stuff with that inode, but it is used in damn few places and
better remain that way....

-Eric

Re: [PATCH 0/2] ima: policy search speedup

[Linus Torvalds]

On Tue, Dec 11, 2012 at 12:15 PM, Eric Paris <eparis@parisplace.org> wrote:
>
> I know it isn't relevant to the final solution, but this is simply
> wrong.  IS_PRIVATE() means 'this inode is filesystem internal.'  It is
> not used by anything except rieser and the anon_inode.  If it is used
> by psuedo filesystems in general, like /proc, shmem mappings, and
> pipes that is a huge bug and is absolutely wrong.

Hmm.. The magic anonfs inode definitely sets it, as far as I can tell.

But it turns out that I was wrong anyway, and you are largely right:
pipes and sockets don't use the anonfs inode (they allocate their own
inodes directly using 'new_inode_pseudo()'), and neither does /proc.

So it's actually only signalfd and timerfd and some other special
things like kvm internal file descriptors that use it.

So never mind about S_PRIVATE, it has odd semantics.

                Linus

Re: [PATCH 0/2] ima: policy search speedup

[Dave Chinner]

On Tue, Dec 11, 2012 at 02:07:22PM -0500, Mimi Zohar wrote:
> On Tue, 2012-12-11 at 13:09 -0500, Eric Paris wrote:
> > On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > 
> > > And your "pseudo-filesystems" argument is pretty stupid too, since WE
> > > ALREADY HAVE A FLAG FOR THAT!
> > >
> > > Guess where it is? Oh, it's in the place I already mentioned makes
> > > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in
> > > users. It's what the other security models already use to avoid
> > > bothering calling down to the security layers. The fact that the
> > > integrity layer bypasses the normal security layer in
> > > ima_file_check(), for example, is no excuse to then make up totally
> > > new flags.
> > 
> > IS_PRIVATE() is not used by and darn well better not be used by, all
> > psuedo filesystems like procfs which IMA may want to ignore.  LSMs
> > like to do control on them.  I thought S_PRIVATE was really only used
> > by the anon_inode and reiserfs's really crazy ass internal inodes.  I
> > could always be wrong.
> 
> I was actually wondering about the MS_NOSEC flag.  It's currently being
> used by fuse, gfs2, ocfs2 and tmpfs.  (Not sure about xfs.)  Can someone
> explain what it is being used for?

For determining whether to clearing suid bits on writes to a
file. It defaults to on for all filesystems that use mount_bdev(),
which is just about every local filesystem (include XFS).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris <eparis@parisplace.org> wrote:
> S_PRIVATE is totally unacceptable as it has a meaning across all LSMs,
> not just IMA.
>
> S_NOSEC means 'this is not setuid or setgid and we don't need to do
> those checks on modify'
>
> You are going to need to use a S_NOIMA.
>
> Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how
> many of them are the same inode over and over again which would be
> circumvented using S_NOIMA per inode flag?
>

IIRC those were only newly instantiated inodes.

For new inodes, S_NOIMA would not be set and used at that point.
IMA must do the policy search and then would set the S_NOIMA.

For subsequent calls, S_NOIMA makes sense.

If we would have the SB flag like MS_NOIMA, then we could replicate sb
flag to inode S_NOIMA flag,
in similar way as inode_has_no_xattr() does:

static inline void inode_has_no_xattr(struct inode *inode)
{
	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC))
		inode->i_flags |= S_NOSEC;
}

Then later there is no need to dereference inode->i_sb...

Can we reach conclusion about it?
Will we have SB flag?
if yes, then where, s_flags, or in some other field like s_feature_flags?
if not, then we can stop this discussion...

- Dmitry

>
>
> On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote:
>>
>>> Anyway, the whole "you can do it at file granularity" isn't the bulk
>>> of my argument (the "we already have the field that makes sense" is).
>>> But my point is that per-inode is not only the logically more
>>> straightforward place to do it, it's also the much more flexible place
>>> to do it. Because it *allows* for things like that.
>>
>> Ok. To summarize, S_IMA indicates that there is a rule and that the iint
>> was allocated.  To differentiate between 'haven't looked/don't know' and
>> 'definitely not', we need another bit.  For this, you're suggesting
>> using IS_PRIVATE()?  Hopefully, I misunderstood.
>>
>> thanks,
>>
>> Mimi
>>
>>
>>

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

Linus made it clear he likes per-inode.  Do a test with per-inode
S_NOIMA.  See how that compares to your 100,000 vs 10,000 lookups.
Then we can discuss with facts if per sb is worth it or not.  We still
don't actually know if 100,000 lookups vs 10,000 lookups really
matters, but at least we'll have some facts...

On Tue, Dec 11, 2012 at 5:57 PM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:
> On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris <eparis@parisplace.org> wrote:
>> S_PRIVATE is totally unacceptable as it has a meaning across all LSMs,
>> not just IMA.
>>
>> S_NOSEC means 'this is not setuid or setgid and we don't need to do
>> those checks on modify'
>>
>> You are going to need to use a S_NOIMA.
>>
>> Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how
>> many of them are the same inode over and over again which would be
>> circumvented using S_NOIMA per inode flag?
>>
>
> IIRC those were only newly instantiated inodes.
>
> For new inodes, S_NOIMA would not be set and used at that point.
> IMA must do the policy search and then would set the S_NOIMA.
>
> For subsequent calls, S_NOIMA makes sense.
>
> If we would have the SB flag like MS_NOIMA, then we could replicate sb
> flag to inode S_NOIMA flag,
> in similar way as inode_has_no_xattr() does:
>
> static inline void inode_has_no_xattr(struct inode *inode)
> {
>         if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC))
>                 inode->i_flags |= S_NOSEC;
> }
>
> Then later there is no need to dereference inode->i_sb...
>
> Can we reach conclusion about it?
> Will we have SB flag?
> if yes, then where, s_flags, or in some other field like s_feature_flags?
> if not, then we can stop this discussion...
>
> - Dmitry
>
>>
>>
>> On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote:
>>>
>>>> Anyway, the whole "you can do it at file granularity" isn't the bulk
>>>> of my argument (the "we already have the field that makes sense" is).
>>>> But my point is that per-inode is not only the logically more
>>>> straightforward place to do it, it's also the much more flexible place
>>>> to do it. Because it *allows* for things like that.
>>>
>>> Ok. To summarize, S_IMA indicates that there is a rule and that the iint
>>> was allocated.  To differentiate between 'haven't looked/don't know' and
>>> 'definitely not', we need another bit.  For this, you're suggesting
>>> using IS_PRIVATE()?  Hopefully, I misunderstood.
>>>
>>> thanks,
>>>
>>> Mimi
>>>
>>>
>>>

Re: [PATCH 0/2] ima: policy search speedup

[Kasatkin, Dmitry]

On Wed, Dec 12, 2012 at 1:02 AM, Eric Paris <eparis@parisplace.org> wrote:
> Linus made it clear he likes per-inode.  Do a test with per-inode
> S_NOIMA.  See how that compares to your 100,000 vs 10,000 lookups.
> Then we can discuss with facts if per sb is worth it or not.  We still
> don't actually know if 100,000 lookups vs 10,000 lookups really
> matters, but at least we'll have some facts...
>

Hello,

I have done few tests.
Ratio between lookups is different, but I do not really remember what
exactly it was.
Probably I did measurement with directory integrity protection...

First test is done using upstream IMA.

IMA-appraisal
[   59.554072] counter1 - before (inode->i_flags & S_NOIMA): 74586
[   59.554628] counter2 - after (inode->i_flags & S_NOIMA): 74558
[   59.555024] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): 52895

You can see 3 counters after 55 seconds uptime.
Counters count entries to the policy search function.
First counter counts every entry.
Second counter counts if control passes after inode flag S_NOIMA.
Third counts if control passes after SB flag SF_NOIMA.

As you can see, inode flag does not make a difference - just 28
entries differences.

But for SB flag, difference is over 22000.

Directory integrity protection is not really relevant to this discussion,
because it is under work now,  but with directory integrity protection
ratio is very different.

IMA-appraisal with directories
[   59.999077] counter1 - before (inode->i_flags & S_NOIMA): 198761
[   59.999078] counter2 - after (inode->i_flags & S_NOIMA): 198728
[   59.999079] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): 50266

Again, inode flags does not make a difference.
SB flag gives ~150 000 less searches.

Basically what I want to show is that inode flag has no benefit.
We need SB flag for that.

Regarding referencing i_sb, IMA policy search might do it for every rule, like

	if ((rule->flags & IMA_FSMAGIC)
	    && rule->fsmagic != inode->i_sb->s_magic)
		return false;


So 20k for 55 seconds can be multiplied roughly by the number of rules.

In fact earlier check for (inode->i_sb->s_feature_flags & SF_NOIMA)
only decreases
the total number of referencing.


- Dmitry


> On Tue, Dec 11, 2012 at 5:57 PM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
>> On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris <eparis@parisplace.org> wrote:
>>> S_PRIVATE is totally unacceptable as it has a meaning across all LSMs,
>>> not just IMA.
>>>
>>> S_NOSEC means 'this is not setuid or setgid and we don't need to do
>>> those checks on modify'
>>>
>>> You are going to need to use a S_NOIMA.
>>>
>>> Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how
>>> many of them are the same inode over and over again which would be
>>> circumvented using S_NOIMA per inode flag?
>>>
>>
>> IIRC those were only newly instantiated inodes.
>>
>> For new inodes, S_NOIMA would not be set and used at that point.
>> IMA must do the policy search and then would set the S_NOIMA.
>>
>> For subsequent calls, S_NOIMA makes sense.
>>
>> If we would have the SB flag like MS_NOIMA, then we could replicate sb
>> flag to inode S_NOIMA flag,
>> in similar way as inode_has_no_xattr() does:
>>
>> static inline void inode_has_no_xattr(struct inode *inode)
>> {
>>         if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC))
>>                 inode->i_flags |= S_NOSEC;
>> }
>>
>> Then later there is no need to dereference inode->i_sb...
>>
>> Can we reach conclusion about it?
>> Will we have SB flag?
>> if yes, then where, s_flags, or in some other field like s_feature_flags?
>> if not, then we can stop this discussion...
>>
>> - Dmitry
>>
>>>
>>>
>>> On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>>> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote:
>>>>
>>>>> Anyway, the whole "you can do it at file granularity" isn't the bulk
>>>>> of my argument (the "we already have the field that makes sense" is).
>>>>> But my point is that per-inode is not only the logically more
>>>>> straightforward place to do it, it's also the much more flexible place
>>>>> to do it. Because it *allows* for things like that.
>>>>
>>>> Ok. To summarize, S_IMA indicates that there is a rule and that the iint
>>>> was allocated.  To differentiate between 'haven't looked/don't know' and
>>>> 'definitely not', we need another bit.  For this, you're suggesting
>>>> using IS_PRIVATE()?  Hopefully, I misunderstood.
>>>>
>>>> thanks,
>>>>
>>>> Mimi
>>>>
>>>>
>>>>

Re: [PATCH 0/2] ima: policy search speedup

[Eric Paris]

On Wed, Dec 12, 2012 at 8:56 AM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:

> I have done few tests.
> Ratio between lookups is different, but I do not really remember what
> exactly it was.
> Probably I did measurement with directory integrity protection...
>
> First test is done using upstream IMA.
>
> IMA-appraisal
> [   59.554072] counter1 - before (inode->i_flags & S_NOIMA): 74586
> [   59.554628] counter2 - after (inode->i_flags & S_NOIMA): 74558
> [   59.555024] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): 52895
>
> You can see 3 counters after 55 seconds uptime.
> Counters count entries to the policy search function.
> First counter counts every entry.
> Second counter counts if control passes after inode flag S_NOIMA.
> Third counts if control passes after SB flag SF_NOIMA.
>
> As you can see, inode flag does not make a difference - just 28
> entries differences.

That is quite shocking to me that on system start up we only ever
reference the same /proc file 28 times.  Wow.  I just don't know what
to say.  Obviously the per inode flag is dead.  Now there is probably
some CPU benefit to a per-sb flag, but I'm not sure it is worth the
code to do it if you can't really measure that as something that makes
a difference in userspace.

Maybe if you can show that after your integrity protection work is
done we can revisit.  But I'd think this patch is dead until you can
show the impact on real userspace.  Someone else might disagree, but
that's my thought.

Sorry.

-Eric