On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
> From: "Palik, Imre" <im...@amazon.de>
>
> When file auditing is enabled, during a low memory situation, a memory
> allocation with __GFP_FS can lead to pruning the inode cache. Which can,
> in turn lead to audit_tree_freeing_mark() being called. This can call
> audit_schedule_prune(), that tries to fork a pruning thread, and
> waits until the thread is created. But forking needs memory, and the
> memory allocations there are done with __GFP_FS.
>
> So we are waiting merrily for some __GFP_FS memory allocations to complete,
> while holding some filesystem locks. This can take a while ...
>
> This patch creates a single thread for pruning the tree from
> audit_add_tree_rule(), and thus avoids the deadlock that the on-demand
> thread creation can cause.
>
> Reported-by: Matt Wilson <m...@amazon.com>
> Cc: Matt Wilson <m...@amazon.com>
> Signed-off-by: Imre Palik <im...@amazon.de>
Thanks for sticking with this and posting a revised patch, my comments are
inline with the patch below ... also as a FYI, when sending a revised patch it
is often helpful to put a revision indicator in the subject line, as an
example:
"[RFC PATCH v2] audit: make audit less awful"
It's not strictly necessary, but it makes my life just a little bit easier.
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 0caf1f8..0ada577 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
...
> +static int launch_prune_thread(void)
> +{
> + prune_thread = kthread_create(prune_tree_thread, NULL,
> + "audit_prune_tree");
> + if (IS_ERR(prune_thread)) {
> + audit_panic("cannot start thread audit_prune_tree");
I'm not certain audit_panic() is warranted here, pr_err() might be a better
choice. What is the harm if the thread doesn't start and we return an error
code?
> + prune_thread = NULL;
> + return -ENOSYS;
Out of curiosity, why ENOSYS?
> + } else {
> + wake_up_process(prune_thread);
> + return 0;
> + }
> +}
See my comments below in audit_schedule_prune().
> /* called with audit_filter_mutex */
> int audit_add_tree_rule(struct audit_krule *rule)
> {
> @@ -663,6 +713,12 @@ int audit_add_tree_rule(struct audit_krule *rule)
> /* do not set rule->tree yet */
> mutex_unlock(&audit_filter_mutex);
>
> + if (unlikely(!prune_thread)) {
> + err = launch_prune_thread();
> + if (err)
> + goto Err;
> + }
> +
Why not put this at the top of audit_add_tree_rule()?
> err = kern_path(tree->pathname, 0, &path);
> if (err)
> goto Err;
> @@ -713,6 +769,9 @@ int audit_tag_tree(char *old, char *new)
> struct vfsmount *tagged;
> int err;
>
> + if (!prune_thread)
> + return -ENOSYS;
Help me out - why?
> err = kern_path(new, 0, &path2);
> if (err)
> return err;
> @@ -800,36 +859,11 @@ int audit_tag_tree(char *old, char *new)
> return failed;
> }
>
> -/*
> - * That gets run when evict_chunk() ends up needing to kill audit_tree.
> - * Runs from a separate thread.
> - */
> -static int prune_tree_thread(void *unused)
> -{
> - mutex_lock(&audit_cmd_mutex);
> - mutex_lock(&audit_filter_mutex);
> -
> - while (!list_empty(&prune_list)) {
> - struct audit_tree *victim;
> -
> - victim = list_entry(prune_list.next, struct audit_tree, list);
> - list_del_init(&victim->list);
> -
> - mutex_unlock(&audit_filter_mutex);
> -
> - prune_one(victim);
> -
> - mutex_lock(&audit_filter_mutex);
> - }
> -
> - mutex_unlock(&audit_filter_mutex);
> - mutex_unlock(&audit_cmd_mutex);
> - return 0;
> -}
>
> static void audit_schedule_prune(void)
> {
> - kthread_run(prune_tree_thread, NULL, "audit_prune_tree");
> + BUG_ON(!prune_thread);
> + wake_up_process(prune_thread);
> }
First, I probably wasn't clear last time so I'll be more clear now: no
BUG_ON() here, handle the error.
Second, and closely related to the last sentence, perhaps the right approach
is to merge the launch_prune_thread() code with audit_schedule_prune() such
that we only have one function which starts the thread if it isn't present,
and wakes it up if it is, something like the following:
static int audit_schedule_prune(void)
{
if (!prune_thread) {
prune_thread = kthread_create(...);
if (IS_ERR(prune_thread)) {
pr_err("cannot start thread audit_prune_tree");
prune_thread = NULL;
return -ENOSYS;
}
}
wake_up_process(prune_thread);
return 0;
}
> /*
> @@ -896,9 +930,10 @@ static void evict_chunk(struct audit_chunk *chunk)
> for (n = 0; n < chunk->count; n++)
> list_del_init(&chunk->owners[n].list);
> spin_unlock(&hash_lock);
> + mutex_unlock(&audit_filter_mutex);
> if (need_prune)
> audit_schedule_prune();
> - mutex_unlock(&audit_filter_mutex);
> +
> }
Remove that trailing empty vertical whitespace please. If you aren't using it
already, you should look into scripts/checkpatch.pl to sanity check your
patches before sending.
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
