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(-)
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(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4a11697..e3378a4 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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;
}
--
1.7.1
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(-)
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index ace72ed..866d403 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -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);
--
1.7.1
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(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f209448..c707afb 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;
@@ -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);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index facd704..5675916 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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 */
--
1.7.1
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(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index ad9c168..f209448 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -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];
--
1.7.1
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(-)
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index bd418c4..ace72ed 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -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);
}
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index e3378a4..facd704 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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);
--
1.7.1
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(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index c707afb..eb53bc7 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -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);
}
--
1.7.1
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(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 40ed981..4a11697 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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;
--
1.7.1
On Thu, 2014-10-02 at 22:05 -0400, Richard Guy Briggs wrote:
> 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(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 4a11697..e3378a4 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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;
I left the default case and made it:
err = -EINVAL;
WARN_ON(1);
Seemed like better defensive coding....
> }
>
> + if (err || type == AUDIT_DEL_RULE)
> + audit_free_rule(entry);
> +
> return err;
> }
>
On Thu, 2014-10-02 at 22:05 -0400, Richard Guy Briggs wrote:
> 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(-)
>
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index bd418c4..ace72ed 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -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);
> }
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?
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index e3378a4..facd704 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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);
On Thu, 2014-10-02 at 22:05 -0400, Richard Guy Briggs wrote:
> 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...
> 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...
>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> kernel/audit_watch.c | 23 +++++++++++++++--------
> 1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index ad9c168..f209448 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -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];
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...
On Thu, 2014-10-02 at 22:05 -0400, Richard Guy Briggs wrote:
> 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(-)
>
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index f209448..c707afb 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;
> @@ -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);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index facd704..5675916 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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 */
>