Re: [PATCH V5] audit: save signal match info in case entry passed in is the one deleted

[Richard Guy Briggs <rgb@redhat.com>]

On 15/08/05, Paul Moore wrote:
> On Wednesday, August 05, 2015 05:23:10 AM Richard Guy Briggs wrote:
> > Move the access to the entry for audit_match_signal() to the beginning of
> > the function in case the entry found is the same one passed in.  This will
> > enable it to be used by audit_remove_mark_rule().
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > Revision history:
> > v4 -> v5:
> >     Move mutex_unlock after out label.
> >     Move list_del group after test for signal to remove temp variable.
> > 
> > ---
> > This patch was split out from the audit by executable path patch set due to
> > the potential to use it elsewhere.
> > 
> > In particular, some questions came up while assessing the potential for code
> > reuse:
> > 
> > 	Why does audit_remove_parent_watches() not call audit_del_rule() for
> > 	each entry found?
> >                 Is audit_signals not properly decremented?
> >                 Is audit_n_rules not properly decremented?
> > 
> >         Why does kill_rules() not call audit_del_rule() for each entry
> > found? Is audit_signals not properly decremented?
> >                 Is audit_n_rules not properly decremented?
> > 
> >  kernel/auditfilter.c |   13 ++++++++-----
> >  1 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 4cb9b44..1b110fb 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) mutex_lock(&audit_filter_mutex);
> >  	e = audit_find_rule(entry, &list);
> >  	if (!e) {
> > -		mutex_unlock(&audit_filter_mutex);
> >  		ret = -ENOENT;
> >  		goto out;
> >  	}
> > @@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (e->rule.tree)
> >  		audit_remove_tree_rule(&e->rule);
> > 
> > -	list_del_rcu(&e->list);
> > -	list_del(&e->rule.list);
> > -	call_rcu(&e->rcu, audit_free_rule_rcu);
> > +	if (e->rule.exe)
> > +		audit_remove_mark_rule(&e->rule);
> 
> What?

Wow, whoops!  I had to stare at it a while to see what was wrong...
Those last two lines belong in a different patch set...

> I think you munged a cut n' paste somehow.  This code doesn't even compile.

That was a local git tree rebase merge conflict manual fix error...

Not a bisect, but with the other patch set, it does.  :)
Re-generating audit-by-executable patchset too...

> >  #ifdef CONFIG_AUDITSYSCALL
> >  	if (!dont_count)
> > @@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (!audit_match_signal(entry))
> >  		audit_signals--;
> >  #endif
> > -	mutex_unlock(&audit_filter_mutex);
> > +
> > +	list_del_rcu(&e->list);
> > +	list_del(&e->rule.list);
> > +	call_rcu(&e->rcu, audit_free_rule_rcu);
> > 
> >  out:
> > +	mutex_unlock(&audit_filter_mutex);
> > +
> >  	if (tree)
> >  		audit_put_tree(tree);	/* that's the temporary one */
> 
> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545