audit: rebalance and remove extra layers of watch references

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 <[email protected]>

---
 kernel/audit_watch.c |    5 ++---
 kernel/auditfilter.c |    9 ---------
 2 files changed, 2 insertions(+), 12 deletions(-)

kernel/auditfilter.c +0 -2

index 72e1660..74cc077 100644
@@ -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);

Quoted 8 lines · 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 <[email protected]>

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

Quoted 17 lines

kernel/auditfilter.c +0 -2

index 72e1660..74cc077 100644
@@ -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);
 

kernel/auditfilter.c static inline int audit_add_rule(struct audit_entry *entry) · line 936

Quoted 7 lines

kernel/auditfilter.c +0 -4

- 	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

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 <[email protected]>

---
 kernel/audit_watch.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

kernel/audit_watch.c +0 -0

index f33f54c..8f123d7 100644
@@ -391,11 +391,12 @@ static void audit_add_to_parent(struct audit_krule
 *krule,
 

kernel/audit_watch.c +0 -3

 		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:

Quoted 23 lines · 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 <[email protected]>

---
 kernel/audit_watch.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

kernel/audit_watch.c +2 -0

index f33f54c..8f123d7 100644
@@ -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;
 	}
 

kernel/audit_watch.c static void audit_add_to_parent(struct audit_krule *krule, · line 400

Quoted 3 lines

kernel/audit_watch.c +0 -1

 	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?

Quoted 4 lines

kernel/audit_watch.c +0 -0

 		audit_get_watch(watch);

@@ -436,9 +437,6 @@ int audit_add_watch(struct audit_krule *krule, struct
 list_head **list)
 

Quoted 8 lines

kernel/audit_watch.c +0 -3

 	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

Quoted 30 lines · Paul Moore

Quoted 29 lines · 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 <[email protected]>

---

 kernel/audit_watch.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

kernel/audit_watch.c +2 -0

index f33f54c..8f123d7 100644
@@ -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;
 
 	}
 

Quoted 8 lines

kernel/audit_watch.c static void audit_add_to_parent(struct audit_krule *krule,

Quoted 5 lines

kernel/audit_watch.c +0 -1

 	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.