ima: policy search speedup

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

Hello,

Any thoughts about this proposal?

- Dmitry

Quoted 28 lines · 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

Quoted 9 lines

Quoted 9 lines

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?

Quoted 3 lines · Dmitry

Quoted 3 lines

Quoted 3 lines

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

Quoted 4 lines · Mimi Zohar

Quoted 3 lines · Dmitry

Quoted 3 lines

Quoted 3 lines

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

Quite frankly, this seems stupid.

What exactly seems stupid here?

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

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.

Quoted 10 lines · Linus Torvalds

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

Quoted 10 lines · Linus Torvalds

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.

Quoted 14 lines · Eric Paris

Quoted 10 lines · Linus Torvalds

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.

Quoted 5 lines

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?

Quoted 4 lines · Dmitry

Quoted 2 lines

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?

Quoted 2 lines

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

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.

Quoted 4 lines · Dmitry

Quoted 2 lines

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?

Quoted 2 lines

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

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

Quoted 3 lines · Dmitry

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

Quoted 4 lines · Mimi Zohar

Quoted 3 lines · Dmitry

Quoted 3 lines

Quoted 3 lines

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.

Quoted 7 lines · Mimi Zohar

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?

Bottom line,
we can't say definitively whether or not a file needs to be measured for any caller.

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.

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.

Quoted 5 lines · Linus Torvalds

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

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?

Quoted 19 lines · Linus Torvalds

Quoted 6 lines

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

Quoted 12 lines · 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?

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

Quoted 22 lines · Mimi Zohar

Quoted 19 lines · Linus Torvalds

Quoted 6 lines

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

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

Quoted 62 lines · Dmitry

Quoted 12 lines · 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?

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

Quoted 22 lines · Mimi Zohar

Quoted 19 lines · Linus Torvalds

Quoted 6 lines

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

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.