[PATCH V6 0/2] audit: rebalance and remove extra layers of watch references

[PATCH V6 0/2] audit: rebalance and remove extra layers of watch references

[Richard Guy Briggs]

While working on the audit by executable path feature, it was discovered that
watches and parent references were slightly imbalanced and deeper than
necessary.

Only bump up references when they are actually used and decrease when removed.

Richard Guy Briggs (2):
  audit: eliminate unnecessary extra layer of watch references
  audit: eliminate unnecessary extra layer of watch parent references

 kernel/audit_watch.c |   11 ++++-------
 kernel/auditfilter.c |    9 ---------
 2 files changed, 4 insertions(+), 16 deletions(-)

[PATCH V6 1/2] audit: eliminate unnecessary extra layer of watch references

[Richard Guy Briggs]

The audit watch count was imbalanced, adding an unnecessary layer of watch
references.  Only add the second reference when it is added to a parent.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit_watch.c |    5 ++---
 kernel/auditfilter.c |    9 ---------
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 6e30024..f33f54c 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -203,7 +203,6 @@ int audit_to_watch(struct audit_krule *krule, char *path, int len, u32 op)
 	if (IS_ERR(watch))
 		return PTR_ERR(watch);
 
-	audit_get_watch(watch);
 	krule->watch = watch;
 
 	return 0;
@@ -387,8 +386,7 @@ static void audit_add_to_parent(struct audit_krule *krule,
 
 		watch_found = 1;
 
-		/* put krule's and initial refs to temporary watch */
-		audit_put_watch(watch);
+		/* put krule's ref to temporary watch */
 		audit_put_watch(watch);
 
 		audit_get_watch(w);
@@ -400,6 +398,7 @@ static void audit_add_to_parent(struct audit_krule *krule,
 		audit_get_parent(parent);
 		watch->parent = parent;
 
+		audit_get_watch(watch);
 		list_add(&watch->wlist, &parent->watches);
 	}
 	list_add(&krule->rlist, &watch->rules);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 72e1660..74cc077 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -549,8 +549,6 @@ exit_nofree:
 	return entry;
 
 exit_free:
-	if (entry->rule.watch)
-		audit_put_watch(entry->rule.watch); /* matches initial get */
 	if (entry->rule.tree)
 		audit_put_tree(entry->rule.tree); /* that's the temporary one */
 	audit_free_rule(entry);
@@ -933,11 +931,7 @@ static inline int audit_add_rule(struct audit_entry *entry)
 #endif
 	mutex_unlock(&audit_filter_mutex);
 
- 	return 0;
-
 error:
-	if (watch)
-		audit_put_watch(watch); /* tmp watch, matches initial get */
 	return err;
 }
 
@@ -945,7 +939,6 @@ error:
 static inline int audit_del_rule(struct audit_entry *entry)
 {
 	struct audit_entry  *e;
-	struct audit_watch *watch = entry->rule.watch;
 	struct audit_tree *tree = entry->rule.tree;
 	struct list_head *list;
 	int ret = 0;
@@ -986,8 +979,6 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	mutex_unlock(&audit_filter_mutex);
 
 out:
-	if (watch)
-		audit_put_watch(watch); /* match initial get */
 	if (tree)
 		audit_put_tree(tree);	/* that's the temporary one */
 
-- 
1.7.1

Re: [PATCH V6 1/2] audit: eliminate unnecessary extra layer of watch references

[Paul Moore]

On Tuesday, July 14, 2015 11:40:41 AM Richard Guy Briggs wrote:
> The audit watch count was imbalanced, adding an unnecessary layer of watch
> references.  Only add the second reference when it is added to a parent.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit_watch.c |    5 ++---
>  kernel/auditfilter.c |    9 ---------
>  2 files changed, 2 insertions(+), 12 deletions(-)

Nice catch, we never needed that extra refcnt bump in audit_to_watch().  One 
minor comment below...

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 72e1660..74cc077 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -549,8 +549,6 @@ exit_nofree:
>  	return entry;
> 
>  exit_free:
> -	if (entry->rule.watch)
> -		audit_put_watch(entry->rule.watch); /* matches initial get */
>  	if (entry->rule.tree)
>  		audit_put_tree(entry->rule.tree); /* that's the temporary one */
>  	audit_free_rule(entry);
> @@ -933,11 +931,7 @@ static inline int audit_add_rule(struct audit_entry
> *entry) #endif
>  	mutex_unlock(&audit_filter_mutex);
> 
> - 	return 0;
> -
>  error:
> -	if (watch)
> -		audit_put_watch(watch); /* tmp watch, matches initial get */
>  	return err;
>  }

Since the error label is now just a "return err;", how about removing the 
label entirely and replacing the gotos with returns?

-- 
paul moore
security @ redhat

Re: [PATCH V6 1/2] audit: eliminate unnecessary extra layer of watch references

[Richard Guy Briggs]

On 15/07/16, Paul Moore wrote:
> On Tuesday, July 14, 2015 11:40:41 AM Richard Guy Briggs wrote:
> > The audit watch count was imbalanced, adding an unnecessary layer of watch
> > references.  Only add the second reference when it is added to a parent.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit_watch.c |    5 ++---
> >  kernel/auditfilter.c |    9 ---------
> >  2 files changed, 2 insertions(+), 12 deletions(-)
> 
> Nice catch, we never needed that extra refcnt bump in audit_to_watch().  One 
> minor comment below...
> 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 72e1660..74cc077 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -549,8 +549,6 @@ exit_nofree:
> >  	return entry;
> > 
> >  exit_free:
> > -	if (entry->rule.watch)
> > -		audit_put_watch(entry->rule.watch); /* matches initial get */
> >  	if (entry->rule.tree)
> >  		audit_put_tree(entry->rule.tree); /* that's the temporary one */
> >  	audit_free_rule(entry);
> > @@ -933,11 +931,7 @@ static inline int audit_add_rule(struct audit_entry
> > *entry) #endif
> >  	mutex_unlock(&audit_filter_mutex);
> > 
> > - 	return 0;
> > -
> >  error:
> > -	if (watch)
> > -		audit_put_watch(watch); /* tmp watch, matches initial get */
> >  	return err;
> >  }
> 
> Since the error label is now just a "return err;", how about removing the 
> label entirely and replacing the gotos with returns?

Yup, sounds reasonable.

> 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

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

[Richard Guy Briggs]

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@redhat.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;
 
 		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:
-- 
1.7.1

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

[Paul Moore]

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@redhat.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_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:

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

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

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

[Paul Moore]

On Thursday, July 16, 2015 09:45:10 PM Richard Guy Briggs wrote:
> 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@redhat.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.

Great, thanks for the explanation.  Fix up the goto/return in patch 1/2, 
resubmit and I'll merge this patchset into audit#next.

-- 
paul moore
security @ redhat