Skip to contentSkip to conversation

audit fsnotify cleanups for watches and trees

PATCH In review Richard Guy Briggs 2 participants 12 messages Oct 2, 2014 – Oct 10, 2014 Audit

lore
Share & tools
lore

Richard Guy Briggs

author
#0 lore raw

cover audit fsnotify cleanups for watches and trees

This is a collection of patches to clean up some issues discovered while implementing audit by exe path.

They compile and have been lightly tested.

I'd be interested in feedback about approaches or details or grossly misunderstanding some fundamental concepts.

Thanks!

Richard Guy Briggs (7):
audit: put rule existence check in canonical order
audit: cull redundancy in audit_rule_change
audit: eliminate string copy for new tree rules
audit: optimize add to parent skipping needless search and consuming
parent ref
audit: remove redundant watch refcount
audit: remove extra audit_get_parent()
audit: rename audit_log_remove_rule to disambiguate for trees

 kernel/audit_tree.c  |   13 +++++++------
 kernel/audit_watch.c |   29 ++++++++++++++++-------------
 kernel/auditfilter.c |   34 +++++++++++-----------------------
 3 files changed, 34 insertions(+), 42 deletions(-)
author
#1 lore raw

PATCH 1/7 audit: put rule existence check in canonical order

Use same rule existence check order as audit_make_tree(), audit_to_watch(), update_lsm_rule() for legibility.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/auditfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/auditfilter.c +1 -1
index 40ed981..4a11697 100644
@@ -163,7 +163,7 @@ static inline int audit_to_inode(struct audit_krule *krule,
 				 struct audit_field *f)
 {
 	if (krule->listnr != AUDIT_FILTER_EXIT ||
-	    krule->watch || krule->inode_f || krule->tree ||
+	    krule->inode_f || krule->watch || krule->tree ||
 	    (f->op != Audit_equal && f->op != Audit_not_equal))
 		return -EINVAL;
Signature · 1 line
1.7.1
author
#2 lore raw

PATCH 2/7 audit: cull redundancy in audit_rule_change

In reply to Richard Guy Briggs

Re-factor audit_rule_change() to reduce the amount of code redundancy and simplify the logic.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/auditfilter.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)
kernel/auditfilter.c +7 -13
index 4a11697..e3378a4 100644
@@ -1064,30 +1064,24 @@ int audit_rule_change(int type, __u32 portid, int seq, void *data,
 	int err = 0;
 	struct audit_entry *entry;
 
+	entry = audit_data_to_entry(data, datasz);
+	if (IS_ERR(entry))
+		return PTR_ERR(entry);
+
 	switch (type) {
 	case AUDIT_ADD_RULE:
-		entry = audit_data_to_entry(data, datasz);
-		if (IS_ERR(entry))
-			return PTR_ERR(entry);
-
 		err = audit_add_rule(entry);
 		audit_log_rule_change("add_rule", &entry->rule, !err);
-		if (err)
-			audit_free_rule(entry);
 		break;
 	case AUDIT_DEL_RULE:
-		entry = audit_data_to_entry(data, datasz);
-		if (IS_ERR(entry))
-			return PTR_ERR(entry);
-
 		err = audit_del_rule(entry);
 		audit_log_rule_change("remove_rule", &entry->rule, !err);
-		audit_free_rule(entry);
 		break;
-	default:
-		return -EINVAL;
 	}
 
+	if (err || type == AUDIT_DEL_RULE)
+		audit_free_rule(entry);
+
 	return err;
 }
Signature · 1 line
1.7.1
maintainer
#3 lore raw

Re: [PATCH 2/7] audit: cull redundancy in audit_rule_change

Quoted 32 lines · Richard Guy Briggs

Re-factor audit_rule_change() to reduce the amount of code redundancy and simplify the logic.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/auditfilter.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)
kernel/auditfilter.c +4 -6
index 4a11697..e3378a4 100644
@@ -1064,30 +1064,24 @@ int audit_rule_change(int type, __u32 portid, int seq, void *data,
 	int err = 0;
 	struct audit_entry *entry;
 
+	entry = audit_data_to_entry(data, datasz);
+	if (IS_ERR(entry))
+		return PTR_ERR(entry);
+
 	switch (type) {
 	case AUDIT_ADD_RULE:
-		entry = audit_data_to_entry(data, datasz);
-		if (IS_ERR(entry))
-			return PTR_ERR(entry);
-
 		err = audit_add_rule(entry);
 		audit_log_rule_change("add_rule", &entry->rule, !err);
-		if (err)
-			audit_free_rule(entry);
 		break;
 	case AUDIT_DEL_RULE:

kernel/auditfilter.c int audit_rule_change(int type, __u32 portid, int seq, void *data, · line 1088

Quoted 10 lines
kernel/auditfilter.c +0 -7
-		entry = audit_data_to_entry(data, datasz);
-		if (IS_ERR(entry))
-			return PTR_ERR(entry);
-
 		err = audit_del_rule(entry);
 		audit_log_rule_change("remove_rule", &entry->rule, !err);
-		audit_free_rule(entry);
 		break;
-	default:
-		return -EINVAL;

I left the default case and made it:

                err = -EINVAL;
                WARN_ON(1);

Seemed like better defensive coding....

Quoted 8 lines
kernel/auditfilter.c +3 -0
 	}
 
+	if (err || type == AUDIT_DEL_RULE)
+		audit_free_rule(entry);
+
 	return err;
 }
author
#4 lore raw

PATCH 3/7 audit: eliminate string copy for new tree rules

In reply to Richard Guy Briggs

New tree rules copy the path twice and discard the intermediary copy.

This saves one pointer at the expense of one path string copy.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_tree.c  |    9 +++++----
 kernel/auditfilter.c |    5 +++--
 2 files changed, 8 insertions(+), 6 deletions(-)
kernel/audit_tree.c +5 -4
index bd418c4..ace72ed 100644
@@ -17,7 +17,7 @@ struct audit_tree {
 	struct list_head list;
 	struct list_head same_root;
 	struct rcu_head head;
-	char pathname[];
+	char *pathname;
 };
 
 struct audit_chunk {

@@ -70,11 +70,11 @@ static LIST_HEAD(prune_list);
 
 static struct fsnotify_group *audit_tree_group;
 
-static struct audit_tree *alloc_tree(const char *s)
+static struct audit_tree *alloc_tree(char *s)
 {
 	struct audit_tree *tree;
 
-	tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL);
+	tree = kmalloc(sizeof(struct audit_tree), GFP_KERNEL);
 	if (tree) {
 		atomic_set(&tree->count, 1);
 		tree->goner = 0;

@@ -83,7 +83,7 @@ static struct audit_tree *alloc_tree(const char *s)
 		INIT_LIST_HEAD(&tree->list);
 		INIT_LIST_HEAD(&tree->same_root);
 		tree->root = NULL;
-		strcpy(tree->pathname, s);
+		tree->pathname = s;
 	}
 	return tree;
 }

@@ -96,6 +96,7 @@ static inline void get_tree(struct audit_tree *tree)
 static inline void put_tree(struct audit_tree *tree)
 {
 	if (atomic_dec_and_test(&tree->count))
+		kfree(tree->pathname);
 		kfree_rcu(tree, head);
 }
kernel/auditfilter.c +3 -2
index e3378a4..facd704 100644
@@ -534,9 +534,10 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			entry->rule.buflen += f->val;
 
 			err = audit_make_tree(&entry->rule, str, f->op);
-			kfree(str);
-			if (err)
+			if (err) {
+				kfree(str);
 				goto exit_free;
+			}
 			break;
 		case AUDIT_INODE:
 			err = audit_to_inode(&entry->rule, f);
Signature · 1 line
1.7.1
maintainer
#5 lore raw

Re: [PATCH 3/7] audit: eliminate string copy for new tree rules

Quoted 47 lines · Richard Guy Briggs

New tree rules copy the path twice and discard the intermediary copy.

This saves one pointer at the expense of one path string copy.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_tree.c  |    9 +++++----
 kernel/auditfilter.c |    5 +++--
 2 files changed, 8 insertions(+), 6 deletions(-)
kernel/audit_tree.c +4 -4
index bd418c4..ace72ed 100644
@@ -17,7 +17,7 @@ struct audit_tree {
 	struct list_head list;
 	struct list_head same_root;
 	struct rcu_head head;
-	char pathname[];
+	char *pathname;
 };
 
 struct audit_chunk {

@@ -70,11 +70,11 @@ static LIST_HEAD(prune_list);
 
 static struct fsnotify_group *audit_tree_group;
 
-static struct audit_tree *alloc_tree(const char *s)
+static struct audit_tree *alloc_tree(char *s)
 {
 	struct audit_tree *tree;
 
-	tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL);
+	tree = kmalloc(sizeof(struct audit_tree), GFP_KERNEL);
 	if (tree) {
 		atomic_set(&tree->count, 1);
 		tree->goner = 0;

@@ -83,7 +83,7 @@ static struct audit_tree *alloc_tree(const char *s)
 		INIT_LIST_HEAD(&tree->list);
 		INIT_LIST_HEAD(&tree->same_root);
 		tree->root = NULL;
-		strcpy(tree->pathname, s);
+		tree->pathname = s;
 	}
 	return tree;
 }

kernel/audit_tree.c static inline void get_tree(struct audit_tree *tree) · line 101

Quoted 6 lines
kernel/audit_tree.c +1 -0
@@ -96,6 +96,7 @@ static inline void get_tree(struct audit_tree *tree)
 static inline void put_tree(struct audit_tree *tree)
 {
 	if (atomic_dec_and_test(&tree->count))
+		kfree(tree->pathname);
 		kfree_rcu(tree, head);
 }

Why does the tree need to be freed after an RCU grace period but the pathname can be freed immediately? What's the locking/access that makes this safe?

Quoted 18 lines
kernel/auditfilter.c +3 -2
index e3378a4..facd704 100644
@@ -534,9 +534,10 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			entry->rule.buflen += f->val;
 
 			err = audit_make_tree(&entry->rule, str, f->op);
-			kfree(str);
-			if (err)
+			if (err) {
+				kfree(str);
 				goto exit_free;
+			}
 			break;
 		case AUDIT_INODE:
 			err = audit_to_inode(&entry->rule, f);
author
#6 lore raw

PATCH 4/7 audit: optimize add to parent skipping needless search and consuming parent ref

In reply to Richard Guy Briggs

When parent has just been created there is no need to search for the parent in the list. Add a parameter to skip the search and consume the parent reference no matter what happens.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_watch.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)
kernel/audit_watch.c +15 -8
index ad9c168..f209448 100644
@@ -372,15 +372,20 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent)
 }
 
 /* Associate the given rule with an existing parent.
- * Caller must hold audit_filter_mutex. */
+ * Caller must hold audit_filter_mutex.
+ * Consumes parent reference. */
 static void audit_add_to_parent(struct audit_krule *krule,
-				struct audit_parent *parent)
+				struct audit_parent *parent,
+				int new)
 {
 	struct audit_watch *w, *watch = krule->watch;
 	int watch_found = 0;
 
 	BUG_ON(!mutex_is_locked(&audit_filter_mutex));
 
+	if (new)
+		goto not_found;
+
 	list_for_each_entry(w, &parent->watches, wlist) {
 		if (strcmp(watch->path, w->path))
 			continue;

@@ -396,12 +401,15 @@ static void audit_add_to_parent(struct audit_krule *krule,
 		break;
 	}
 
+not_found:
 	if (!watch_found) {
-		audit_get_parent(parent);
 		watch->parent = parent;
 
 		list_add(&watch->wlist, &parent->watches);
-	}
+	} else
+		/* match get in audit_find_parent or audit_init_parent */
+		audit_put_parent(parent);
+
 	list_add(&krule->rlist, &watch->rules);
 }
 

@@ -413,6 +421,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
 	struct audit_parent *parent;
 	struct path parent_path;
 	int h, ret = 0;
+	int new = 0;
 
 	mutex_unlock(&audit_filter_mutex);
 

@@ -433,12 +442,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
 			ret = PTR_ERR(parent);
 			goto error;
 		}
+		new = 1;
 	}
 
-	audit_add_to_parent(krule, parent);
-
-	/* match get in audit_find_parent or audit_init_parent */
-	audit_put_parent(parent);
+	audit_add_to_parent(krule, parent, new);
 
 	h = audit_hash_ino((u32)watch->ino);
 	*list = &audit_inode_hash[h];
Signature · 1 line
1.7.1
maintainer
#7 lore raw

Re: [PATCH 4/7] audit: optimize add to parent skipping needless search and consuming parent ref

Quoted 2 lines · Richard Guy Briggs

When parent has just been created there is no need to search for the parent in the list. Add a parameter to skip the search

Since the parent was just allocated, and thus has an empty list, this "search" is just as fast as the check against 'new' and doesn't complicate things...

Quoted 2 lines

and consume the parent reference
no matter what happens.

Now the refcnt change... I guess it's personal taste, but I don't
like it at all. If in audit_add_watch() I always get a reference to parent it makes the code a whole lot easier to read if we always put that refcnt in the same function. I don't like sub functions that
consume my ref... Especially since that makes it a whole lot less
obvious in audit_add_watch when I'm allowed to use parent and when I'm not...

So I'm not going to apply this patch. I don't believe it improves things...

Quoted 74 lines
Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_watch.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)
kernel/audit_watch.c +15 -8
index ad9c168..f209448 100644
@@ -372,15 +372,20 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent)
 }
 
 /* Associate the given rule with an existing parent.
- * Caller must hold audit_filter_mutex. */
+ * Caller must hold audit_filter_mutex.
+ * Consumes parent reference. */
 static void audit_add_to_parent(struct audit_krule *krule,
-				struct audit_parent *parent)
+				struct audit_parent *parent,
+				int new)
 {
 	struct audit_watch *w, *watch = krule->watch;
 	int watch_found = 0;
 
 	BUG_ON(!mutex_is_locked(&audit_filter_mutex));
 
+	if (new)
+		goto not_found;
+
 	list_for_each_entry(w, &parent->watches, wlist) {
 		if (strcmp(watch->path, w->path))
 			continue;

@@ -396,12 +401,15 @@ static void audit_add_to_parent(struct audit_krule *krule,
 		break;
 	}
 
+not_found:
 	if (!watch_found) {
-		audit_get_parent(parent);
 		watch->parent = parent;
 
 		list_add(&watch->wlist, &parent->watches);
-	}
+	} else
+		/* match get in audit_find_parent or audit_init_parent */
+		audit_put_parent(parent);
+
 	list_add(&krule->rlist, &watch->rules);
 }
 

@@ -413,6 +421,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
 	struct audit_parent *parent;
 	struct path parent_path;
 	int h, ret = 0;
+	int new = 0;
 
 	mutex_unlock(&audit_filter_mutex);
 

@@ -433,12 +442,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
 			ret = PTR_ERR(parent);
 			goto error;
 		}
+		new = 1;
 	}
 
-	audit_add_to_parent(krule, parent);
-
-	/* match get in audit_find_parent or audit_init_parent */
-	audit_put_parent(parent);
+	audit_add_to_parent(krule, parent, new);
 
 	h = audit_hash_ino((u32)watch->ino);
 	*list = &audit_inode_hash[h];
author
#8 lore raw

PATCH 5/7 audit: remove redundant watch refcount

In reply to Richard Guy Briggs

Remove extra layer of audit_{get,put}_watch() calls.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_watch.c |    5 +----
 kernel/auditfilter.c |    7 -------
 2 files changed, 1 insertions(+), 11 deletions(-)
kernel/audit_watch.c +1 -4
index f209448..c707afb 100644
@@ -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;

@@ -306,7 +305,6 @@ static void audit_update_watch(struct audit_parent *parent,
 				 * new watch.
 				 */
 				audit_put_watch(nentry->rule.watch);
-				audit_get_watch(nwatch);
 				nentry->rule.watch = nwatch;
 				list_add(&nentry->rule.rlist, &nwatch->rules);
 				list_add_rcu(&nentry->list, &audit_inode_hash[h]);

@@ -392,8 +390,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);
kernel/auditfilter.c +0 -7
index facd704..5675916 100644
@@ -563,8 +563,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);

@@ -942,8 +940,6 @@ static inline int audit_add_rule(struct audit_entry *entry)
  	return 0;
 
 error:
-	if (watch)
-		audit_put_watch(watch); /* tmp watch, matches initial get */
 	return err;
 }
 

@@ -951,7 +947,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;

@@ -992,8 +987,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 */
Signature · 1 line
1.7.1
maintainer
#9 lore raw

Re: [PATCH 5/7] audit: remove redundant watch refcount

Having a hard time convincing myself of the next 2... Doesn't mean they're wrong or bad, but my brain isn't seeing it today...

Quoted 77 lines · Richard Guy Briggs

Remove extra layer of audit_{get,put}_watch() calls.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_watch.c |    5 +----
 kernel/auditfilter.c |    7 -------
 2 files changed, 1 insertions(+), 11 deletions(-)
kernel/audit_watch.c +1 -4
index f209448..c707afb 100644
@@ -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;

@@ -306,7 +305,6 @@ static void audit_update_watch(struct audit_parent *parent,
 				 * new watch.
 				 */
 				audit_put_watch(nentry->rule.watch);
-				audit_get_watch(nwatch);
 				nentry->rule.watch = nwatch;
 				list_add(&nentry->rule.rlist, &nwatch->rules);
 				list_add_rcu(&nentry->list, &audit_inode_hash[h]);

@@ -392,8 +390,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);
kernel/auditfilter.c +0 -7
index facd704..5675916 100644
@@ -563,8 +563,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);

@@ -942,8 +940,6 @@ static inline int audit_add_rule(struct audit_entry *entry)
  	return 0;
 
 error:
-	if (watch)
-		audit_put_watch(watch); /* tmp watch, matches initial get */
 	return err;
 }
 

@@ -951,7 +947,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;

@@ -992,8 +987,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 */
author
#10 lore raw

PATCH 6/7 audit: remove extra audit_get_parent()

In reply to Richard Guy Briggs

There appears to be an extra parent reference taken. Remove it.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_watch.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
kernel/audit_watch.c +0 -1
index c707afb..eb53bc7 100644
@@ -462,7 +462,6 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 		audit_remove_watch(watch);
 
 		if (list_empty(&parent->watches)) {
-			audit_get_parent(parent);
 			fsnotify_destroy_mark(&parent->mark, audit_watch_group);
 			audit_put_parent(parent);
 		}
Signature · 1 line
1.7.1
author
#11 lore raw

PATCH 7/7 audit: rename audit_log_remove_rule to disambiguate for trees

In reply to Richard Guy Briggs

Rename audit_log_remove_rule() to audit_tree_log_remove_rule() to avoid confusion with watch and mark rule removal/changes.

Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit_tree.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
kernel/audit_tree.c +2 -2
index ace72ed..866d403 100644
@@ -450,7 +450,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	return 0;
 }
 
-static void audit_log_remove_rule(struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_krule *rule)
 {
 	struct audit_buffer *ab;
 

@@ -477,7 +477,7 @@ static void kill_rules(struct audit_tree *tree)
 		list_del_init(&rule->rlist);
 		if (rule->tree) {
 			/* not a half-baked one */
-			audit_log_remove_rule(rule);
+			audit_tree_log_remove_rule(rule);
 			rule->tree = NULL;
 			list_del_rcu(&entry->list);
 			list_del(&entry->rule.list);
Signature · 1 line
1.7.1