On Mon, 2015-10-26 at 16:01 +0200, Petko Manolov wrote:
> On 15-10-25 07:50:32, Mimi Zohar wrote:
> > On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> >
> > > > @@ -171,9 +172,8 @@ static int __init
> > > > default_appraise_policy_setup(char *str)
> > > > __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > > >
> > > > /*
> > > > - * Although the IMA policy does not change, the LSM policy can be
> > > > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > > > - * stale LSM policy.
> >
> > Please don't remove this comment.
>
> OK, i will leave it, but the beginning of the first sentence should be
> changed
> to reflect that the IMA policy _may_ change.
Right, please start the sentence with "The LSM policy can be ...".
>
> > > > + * Blocking here is not legal as we hold an RCU lock.
> > > > ima_update_policy()
> > > > + * should make it safe to walk the list at any time.
> > > >
> >
> > I'm not sure this comment is needed. If it is needed, this should be moved
> > to
> > below the function description.
>
> From personal experience, i like it when i see such comments, especially at
> critical code section. I'll move it down below if you don't like it there.
I prefer to start with the reason for the function.
> > > > * Update the IMA LSM based rules to reflect the reloaded LSM policy.
> > > > * We assume the rules still exist; and BUG_ON() if they don't.
> > > > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > > > int result;
> > > > int i;
> > > >
> > > > - mutex_lock(&ima_rules_mutex);
> > > > list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
> > >
> > >
> > > Use of "list_for_each_entry_safe" makes me having a doubt.
> > >
> > > "safe" versions are used when entries are removed while walk.
> > >
> > > If it is so, then entire RCU case is impossible?
> >
> > Taking the lock here was to prevent modifying the LSM rule number multiple
> > times. Using the "safe" version was never needed. The worst that can
> > happen
> > here is that the LSM rule number is updated multiple times.
>
> "safe" is a remnant from the original version of the code. I don't think it
> is
> necessary, but didn't want to make too many changes in one go. In the worst
> case the list traversal is a bit slower, which should not happen too often
> anyway.
Right, this was my mistake. Please feel free to change it.
Mimi
--
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
