Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753198AbbHETVs (ORCPT ); Wed, 5 Aug 2015 15:21:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42561 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751686AbbHETVr (ORCPT ); Wed, 5 Aug 2015 15:21:47 -0400 Date: Wed, 5 Aug 2015 15:21:43 -0400 From: Richard Guy Briggs To: Paul Moore Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH V5] audit: save signal match info in case entry passed in is the one deleted Message-ID: <20150805192143.GJ32407@madcap2.tricolour.ca> References: <1583724.gJuObs13E5@sifl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1583724.gJuObs13E5@sifl> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3431 Lines: 98 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 > > > > 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 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 -- 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/