linux-kernel - Re: [PATCH V6 2/2] audit: eliminate unnecessary extra layer of watch parent references

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150717014510.GE32473@madcap2.tricolour.ca>
Date:	Thu, 16 Jul 2015 21:45:10 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	Paul Moore <pmoore@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com
Subject: Re: [PATCH V6 2/2] audit: eliminate unnecessary extra layer of watch
 parent references

On 15/07/16, Paul Moore wrote:
> On Tuesday, July 14, 2015 11:40:42 AM Richard Guy Briggs wrote:
> > The audit watch parent count was imbalanced, adding an unnecessary layer of
> > watch parent references.  Decrement the additional parent reference when a
> > watch is reused, already having a reference to the parent.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > ---
> >  kernel/audit_watch.c |    6 ++----
> >  1 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index f33f54c..8f123d7 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -391,11 +391,12 @@ static void audit_add_to_parent(struct audit_krule
> > *krule,
> > 
> >  		audit_get_watch(w);
> >  		krule->watch = watch = w;
> > +
> > +		audit_put_parent(parent);
> >  		break;
> >  	}
> > 
> >  	if (!watch_found) {
> > -		audit_get_parent(parent);
> >  		watch->parent = parent;
> 
> I understand removing the get() here and the put() in audit_add_watch, but I 
> don't understand adding the put() above, can you help me understand?

audit_find_parent() gets a reference to the parent, if the parent is
already known.  This additional parental reference is not needed if the
watch is subsequently found by audit_add_to_parent(), and consumed if
the watch does not already exist, so we need to put the parent if the
watch is found, and do nothing if this new watch is added to the parent.

If the parent wasn't already known, it is created with a refcount of 1
and added to the audit_watch_group, then incremented by one to be
subsequently consumed by the newly created watch in
audit_add_to_parent().

The graph below may help to visualize it.

The rule points to the watch, not to the parent, so the rule's refcount
gets bumped, not the parent's.

> >  		audit_get_watch(watch);
> > @@ -436,9 +437,6 @@ int audit_add_watch(struct audit_krule *krule, struct
> > list_head **list)
> > 
> >  	audit_add_to_parent(krule, parent);
> > 
> > -	/* match get in audit_find_parent or audit_init_parent */
> > -	audit_put_parent(parent);
> > -
> >  	h = audit_hash_ino((u32)watch->ino);
> >  	*list = &audit_inode_hash[h];
> >  error:

	audit_add_watch(entry->rule)
		parent = audit_find_parent()
			fsnotify_find_inode_mark(audit_watch_group)
				fsnotify_find_mark(audit_watch_group)
					fsnotify_get_mark() parent->mark->refcnt ++
		OR parent = audit_init_parent()
			fsnotify_init_mark() parent->mark->refcnt = 1
			fsnotify_add_mark(parent->mark, audit_watch_group)
				fsnotify_add_mark_locked(parent->mark, audit_watch_group)
				|	fsnotify_get_group(audit_watch_group) audit_watch_group->refcnt ++ (mark->group)
				 |	audit_watch_group->num_marks ++ (mark->g_list)
				  |	fsnotify_get_mark(parent->mark) parent->mark->refcnt ++ (i_list/m_list?)
				|	on err, fsnotify_put_group(audit_watch_group) audit_watch_group->refcnt -- test
				 |	on err, audit_watch_group->num_marks --
				  |	on err, destroy_list fsnotify_put_mark(parent->mark) parent->mark->refcnt -- test
						fsnotify_put_group(parent->mark->group) audit_watch_group->refcnt -- test
							fsnotify_final_destroy_group(audit_watch_group) group->ops->free_group_priv(group)
			on err, audit_free_parent(parent)
		on err, return err
		audit_add_to_parent(entry->rule, parent)
			if found, call audit_put_watch(entry->rule->watch) entry->rule->watch->count -- and test?
			if found, call audit_get_watch(parent->watches->watch) parent->watches->watch->count ++
+p			if found, call audit_put_parent(parent) parent->mark->refcnt --
-p			if not found, call audit_get_parent(parent) parent->mark->refcnt ++
-p		audit_put_parent(parent)
			fsnotify_put_mark(parent->mark) parent->mark->refcnt -- test
				fsnotify_put_group(parent->mark->group) audit_watch_group->refcnt -- test
					fsnotify_final_destroy_group(audit_watch_group) group->ops->free_group_priv(group)

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/