[PATCH RFC] audit: move the tree pruning to a dedicated thread

[PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

From: "Palik, Imre" <imrep@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 <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 kernel/audit_tree.c |   91 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 63 insertions(+), 28 deletions(-)

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
@@ -37,6 +37,7 @@ struct audit_chunk {
 
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
+static struct task_struct *prune_thread;
 
 /*
  * One struct chunk is attached to each inode of interest.
@@ -641,6 +642,55 @@ static int tag_mount(struct vfsmount *mnt, void *arg)
 	return tag_chunk(mnt->mnt_root->d_inode, arg);
 }
 
+/*
+ * 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)
+{
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&prune_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
+
+		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 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");
+		prune_thread = NULL;
+		return -ENOSYS;
+	} else {
+		wake_up_process(prune_thread);
+		return 0;
+	}
+}
+
 /* 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;
+	}
+
 	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;
+
 	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);
 }
 
 /*
@@ -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);
+
 }
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
-- 
1.7.9.5

[PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

From: "Palik, Imre" <imrep@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 <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 kernel/audit_tree.c |   91 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 63 insertions(+), 28 deletions(-)

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
@@ -37,6 +37,7 @@ struct audit_chunk {
 
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
+static struct task_struct *prune_thread;
 
 /*
  * One struct chunk is attached to each inode of interest.
@@ -641,6 +642,55 @@ static int tag_mount(struct vfsmount *mnt, void *arg)
 	return tag_chunk(mnt->mnt_root->d_inode, arg);
 }
 
+/*
+ * 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)
+{
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&prune_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
+
+		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 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");
+		prune_thread = NULL;
+		return -ENOSYS;
+	} else {
+		wake_up_process(prune_thread);
+		return 0;
+	}
+}
+
 /* 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;
+	}
+
 	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;
+
 	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);
 }
 
 /*
@@ -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);
+
 }
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
-- 
1.7.9.5

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Paul Moore]

On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
> From: "Palik, Imre" <imrep@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 <msw@amazon.com>
> Cc: Matt Wilson <msw@amazon.com>
> Signed-off-by: Imre Palik <imrep@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

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Paul Moore]

On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
> From: "Palik, Imre" <imrep@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 <msw@amazon.com>
> Cc: Matt Wilson <msw@amazon.com>
> Signed-off-by: Imre Palik <imrep@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

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

On 01/08/15 22:53, Paul Moore wrote:
> On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
>> From: "Palik, Imre" <imrep@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 <msw@amazon.com>
>> Cc: Matt Wilson <msw@amazon.com>
>> Signed-off-by: Imre Palik <imrep@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.

Sorry for that.  I realised that I botched the subject immediately after
sending the mail :-(

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

I thought disabling the bigger part of the file auditing would warrant a bigger
bang than pr_err().  If you think, it is an overkill, then I can change it.

But see my comment below in audit_schedule_prune()

>> +		prune_thread = NULL;
>> +		return -ENOSYS;
> 
> Out of curiosity, why ENOSYS?

I thought it is a bit less confusing than ESRCH.

>> +	} 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()?

I would like to do this without holding audit_filter_mutex.

if (!prune_thread) then the file auditing is disabled, so it probably won't
hurt moving this to the beginning of the function, but I feel better when it
is done here.

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

As far as I see, I disabled all the codepaths that can reach this point with
!prune_thread.  So I thought leaving the BUG_ON() here doesn't really matter.

> 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;
> 	}

if we do the thread creation in audit_schedule_prune, we won't necessarily
need the dedicated thread.  This would be the alternative approach I mentioned
in the comment part of my original mail.  Sorry if I was not clear enough.

Basically I see the following approaches:

1) dedicated thread created on syscall entry, with disabling file auditing as
   long as the thread cannot be created.

2) on-demand thread-creation/destruction with some serialisation to ensure
   that we won't create too many threads.

3) dedicated thread created on syscall entry, with fallback to thread creation
   at cleanup time, if original thread creation fails.

Am I right that you would like to see the third one?

> 
>>  /*
>> @@ -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.

Could you point me to that whitespace?  I cannot see it in the patch, and
scripts/checkpatch.pl was not complaining either.

It might be my mail setup, but without knowing what is amiss, I cannot troubleshoot
it.

> 

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Paul Moore]

On Monday, January 12, 2015 09:11:21 AM Imre Palik wrote:
> On 01/08/15 22:53, Paul Moore wrote:
> > On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
> >> From: "Palik, Imre" <imrep@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 <msw@amazon.com>
> >> Cc: Matt Wilson <msw@amazon.com>
> >> Signed-off-by: Imre Palik <imrep@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.
> 
> Sorry for that.  I realised that I botched the subject immediately after
> sending the mail :-(

No worries, you'll take care of it next time.

> >> 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?
> 
> I thought disabling the bigger part of the file auditing would warrant a
> bigger bang than pr_err().  If you think, it is an overkill, then I can
> change it.
>
> But see my comment below in audit_schedule_prune()

My concern with audit_panic() is that it only generates a panic() in the 
*very* rare circumstance that someone has configured it that way.  While there 
are some users who do configure their systems with AUDIT_FAIL_PANIC, I think 
it is safe to say that most do not.  Further, audit_panic() can be rate 
limited or even silenced in the case of AUDIT_FAIL_SILENT.

The choice of pr_err() is not perfect for all scenarios, but I think it is the 
better choice for most of the common scenarios. 

> >> +		prune_thread = NULL;
> >> +		return -ENOSYS;
> > 
> > Out of curiosity, why ENOSYS?
> 
> I thought it is a bit less confusing than ESRCH.

Originally I was thinking about -ENOMEM, thoughts?

> >> +	} 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()?
> 
> I would like to do this without holding audit_filter_mutex.

Sorry, I forgot that audit_add_tree_rule() is called with the 
audit_filter_mutex locked.

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

Still, 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.
> 
> As far as I see, I disabled all the codepaths that can reach this point with
> !prune_thread.  So I thought leaving the BUG_ON() here doesn't really
> matter.

If it doesn't do anything, then you can remove it ;)

BUG_ON()/BUG() does have its uses, but I'm not sure this in one of those 
cases.

> > 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;
> > 	
> > 	}
> 
> if we do the thread creation in audit_schedule_prune, we won't necessarily
> need the dedicated thread.  This would be the alternative approach I
> mentioned in the comment part of my original mail.  Sorry if I was not
> clear enough.

The code in the snippet I provided starts the thread if it doesn't exist, and 
wakes the thread if it exists.  I don't understand how that is different from 
what you were doing, I just happen to think it is a little more robust.

> Basically I see the following approaches:
> 
> 1) dedicated thread created on syscall entry, with disabling file auditing
> as long as the thread cannot be created.
> 
> 2) on-demand thread-creation/destruction with some serialisation to ensure
>    that we won't create too many threads.
> 
> 3) dedicated thread created on syscall entry, with fallback to thread
> creation at cleanup time, if original thread creation fails.
> 
> Am I right that you would like to see the third one?

I don't want #2, and I think in general we should do whatever we can to 
recover from errors such that we don't disable auditing.  That is just good 
practice.

> >>  /*
> >> 
> >> @@ -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.
> 
> Could you point me to that whitespace?  I cannot see it in the patch, and
> scripts/checkpatch.pl was not complaining either.

In your patch it looks like there is a blank empty line at the end of 
evict_chunk(); it appears like you are replacing the last mutex_unlock() with 
blank line.

-- 
paul moore
www.paul-moore.com

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

On 01/13/15 02:47, Paul Moore wrote:
> On Monday, January 12, 2015 09:11:21 AM Imre Palik wrote:
>> On 01/08/15 22:53, Paul Moore wrote:
>>> On Tuesday, January 06, 2015 03:51:20 PM Imre Palik wrote:
>>>> From: "Palik, Imre" <imrep@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 <msw@amazon.com>
>>>> Cc: Matt Wilson <msw@amazon.com>
>>>> Signed-off-by: Imre Palik <imrep@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.
>>
>> Sorry for that.  I realised that I botched the subject immediately after
>> sending the mail :-(
> 
> No worries, you'll take care of it next time.
> 
>>>> 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?
>>
>> I thought disabling the bigger part of the file auditing would warrant a
>> bigger bang than pr_err().  If you think, it is an overkill, then I can
>> change it.
>>
>> But see my comment below in audit_schedule_prune()
> 
> My concern with audit_panic() is that it only generates a panic() in the 
> *very* rare circumstance that someone has configured it that way.  While there 
> are some users who do configure their systems with AUDIT_FAIL_PANIC, I think 
> it is safe to say that most do not.  Further, audit_panic() can be rate 
> limited or even silenced in the case of AUDIT_FAIL_SILENT.
> 
> The choice of pr_err() is not perfect for all scenarios, but I think it is the 
> better choice for most of the common scenarios. 

Ok.  so pr_err() is the one with reliably the bigger bang.

>>>> +		prune_thread = NULL;
>>>> +		return -ENOSYS;
>>>
>>> Out of curiosity, why ENOSYS?
>>
>> I thought it is a bit less confusing than ESRCH.
> 
> Originally I was thinking about -ENOMEM, thoughts?

That might be better in the sense that the application programmer's response
to that case might be the one we want here.

> 
>>>> +	} 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()?
>>
>> I would like to do this without holding audit_filter_mutex.
> 
> Sorry, I forgot that audit_add_tree_rule() is called with the 
> audit_filter_mutex locked.
> 
>>>>  	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?
> 
> Still, why?

You are right, it is not needed.
The codepath I was worried about is impossible.

>>>>  	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.
>>
>> As far as I see, I disabled all the codepaths that can reach this point with
>> !prune_thread.  So I thought leaving the BUG_ON() here doesn't really
>> matter.
> 
> If it doesn't do anything, then you can remove it ;)

Will do.

> 
> BUG_ON()/BUG() does have its uses, but I'm not sure this in one of those 
> cases.
> 
>>> 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;
>>> 	
>>> 	}
>>
>> if we do the thread creation in audit_schedule_prune, we won't necessarily
>> need the dedicated thread.  This would be the alternative approach I
>> mentioned in the comment part of my original mail.  Sorry if I was not
>> clear enough.
> 
> The code in the snippet I provided starts the thread if it doesn't exist, and 
> wakes the thread if it exists.  I don't understand how that is different from 
> what you were doing, I just happen to think it is a little more robust.

The thing that worries me there, is that thread creation still getting called
from filesystem code.  As far as I see no filesystem locks are held on the
codepath right now, but if things change, we will be hit with the same error
I am trying to fix here.  I.e., it might be more robust in one sense, but it is
brittle too.

Moreover it needs an intricate dance to serialise the thread creation (so we
won't leak threads), without holding a mutex during the thread creation itself.

> 
>> Basically I see the following approaches:
>>
>> 1) dedicated thread created on syscall entry, with disabling file auditing
>> as long as the thread cannot be created.
>>
>> 2) on-demand thread-creation/destruction with some serialisation to ensure
>>    that we won't create too many threads.
>>
>> 3) dedicated thread created on syscall entry, with fallback to thread
>> creation at cleanup time, if original thread creation fails.
>>
>> Am I right that you would like to see the third one?
> 
> I don't want #2, and I think in general we should do whatever we can to 
> recover from errors such that we don't disable auditing.  That is just good 
> practice.
> 
>>>>  /*
>>>>
>>>> @@ -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.
>>
>> Could you point me to that whitespace?  I cannot see it in the patch, and
>> scripts/checkpatch.pl was not complaining either.
> 
> In your patch it looks like there is a blank empty line at the end of 
> evict_chunk(); it appears like you are replacing the last mutex_unlock() with 
> blank line.
> 

Re: [PATCH RFC] audit: move the tree pruning to a dedicated thread

[Paul Moore]

On Thursday, December 04, 2014 12:39:21 PM Imre Palik wrote:
> From: "Palik, Imre" <imrep@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_tree_init(), and thus avoids the deadlock that the on-demand thread
> creation can cause.
> 
> An alternative approach would be to move the thread creation outside of the
> lock.  This would assume that other layers of the filesystem code don't
> hold any locks, and it would need some rewrite of the code to limit the
> amount of threads possibly spawned.
> 
> Reported-by: Matt Wilson <msw@amazon.com>
> Cc: Matt Wilson <msw@amazon.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Imre Palik <imrep@amazon.de>
> ---
>  kernel/audit_tree.c |   53 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 18 deletions(-)

Sorry for the delay, we've changed maintainers recently and some patches/issue 
were lost in the handoff.  Some comments below ...

> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 0caf1f8..cf6db88 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -37,6 +37,7 @@ struct audit_chunk {
> 
>  static LIST_HEAD(tree_list);
>  static LIST_HEAD(prune_list);
> +static struct task_struct *prune_thread;
> 
>  /*
>   * One struct chunk is attached to each inode of interest.
> @@ -806,30 +807,39 @@ int audit_tag_tree(char *old, char *new)
>   */
>  static int prune_tree_thread(void *unused)
>  {
> -	mutex_lock(&audit_cmd_mutex);
> -	mutex_lock(&audit_filter_mutex);
> +	for (;;) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (list_empty(&prune_list))
> +			schedule();
> +		__set_current_state(TASK_RUNNING);
> 
> -	while (!list_empty(&prune_list)) {
> -		struct audit_tree *victim;
> +		mutex_lock(&audit_cmd_mutex);
> +		mutex_lock(&audit_filter_mutex);
> 
> -		victim = list_entry(prune_list.next, struct audit_tree, list);
> -		list_del_init(&victim->list);
> +		while (!list_empty(&prune_list)) {
> +			struct audit_tree *victim;
> 
> -		mutex_unlock(&audit_filter_mutex);
> +			victim = list_entry(prune_list.next,
> +					struct audit_tree, list);
> +			list_del_init(&victim->list);
> 
> -		prune_one(victim);
> +			mutex_unlock(&audit_filter_mutex);
> 
> -		mutex_lock(&audit_filter_mutex);
> -	}
> +			prune_one(victim);
> 
> -	mutex_unlock(&audit_filter_mutex);
> -	mutex_unlock(&audit_cmd_mutex);
> +			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);

I don't really like the BUG_ON() here.  If we can't guarantee that the thread 
is still alive, we should look into some fallback approach so that we can 
still prune the tree.  I imagine something could be done with the parameter to 
prune_tree_thread() to indicate if it is running in a dedicated thread or not.

> +	wake_up_process(prune_thread);
>  }
> 
>  /*
> @@ -896,9 +906,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);
> +
>  }
> 
>  static int audit_tree_handle_event(struct fsnotify_group *group,
> @@ -938,10 +949,16 @@ static int __init audit_tree_init(void)
>  {
>  	int i;
> 
> -	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
> -	if (IS_ERR(audit_tree_group))
> -		audit_panic("cannot initialize fsnotify group for rectree watches");
> -
> +	prune_thread = kthread_create(prune_tree_thread, NULL,
> +				"audit_prune_tree");
> +	if (IS_ERR(prune_thread)) {
> +		audit_panic("cannot start thread audit_prune_tree");

Only in the most extreme configurations is audit_panic() an actual panic().  
This goes hand in hand with the comment above regarding the case where the 
pruning thread may not exist.

> +	} else {
> +		wake_up_process(prune_thread);
> +		audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
> +		if (IS_ERR(audit_tree_group))
> +			audit_panic("cannot initialize fsnotify group for rectree 
watches");
> +	}

The above doesn't really need to be in an else block does it?

>  	for (i = 0; i < HASH_SIZE; i++)
>  		INIT_LIST_HEAD(&chunk_hash_heads[i]);

-- 
paul moore
www.paul-moore.com

[PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

From: "Palik, Imre" <imrep@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_tree_init(), and thus avoids the deadlock that the on-demand thread
creation can cause.

An alternative approach would be to move the thread creation outside of the
lock.  This would assume that other layers of the filesystem code don't
hold any locks, and it would need some rewrite of the code to limit the
amount of threads possibly spawned.

Reported-by: Matt Wilson <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 kernel/audit_tree.c |   53 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 0caf1f8..cf6db88 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -37,6 +37,7 @@ struct audit_chunk {
 
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
+static struct task_struct *prune_thread;
 
 /*
  * One struct chunk is attached to each inode of interest.
@@ -806,30 +807,39 @@ int audit_tag_tree(char *old, char *new)
  */
 static int prune_tree_thread(void *unused)
 {
-	mutex_lock(&audit_cmd_mutex);
-	mutex_lock(&audit_filter_mutex);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&prune_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
 
-	while (!list_empty(&prune_list)) {
-		struct audit_tree *victim;
+		mutex_lock(&audit_cmd_mutex);
+		mutex_lock(&audit_filter_mutex);
 
-		victim = list_entry(prune_list.next, struct audit_tree, list);
-		list_del_init(&victim->list);
+		while (!list_empty(&prune_list)) {
+			struct audit_tree *victim;
 
-		mutex_unlock(&audit_filter_mutex);
+			victim = list_entry(prune_list.next,
+					struct audit_tree, list);
+			list_del_init(&victim->list);
 
-		prune_one(victim);
+			mutex_unlock(&audit_filter_mutex);
 
-		mutex_lock(&audit_filter_mutex);
-	}
+			prune_one(victim);
 
-	mutex_unlock(&audit_filter_mutex);
-	mutex_unlock(&audit_cmd_mutex);
+			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);
 }
 
 /*
@@ -896,9 +906,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);
+
 }
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
@@ -938,10 +949,16 @@ static int __init audit_tree_init(void)
 {
 	int i;
 
-	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
-	if (IS_ERR(audit_tree_group))
-		audit_panic("cannot initialize fsnotify group for rectree watches");
-
+	prune_thread = kthread_create(prune_tree_thread, NULL,
+				"audit_prune_tree");
+	if (IS_ERR(prune_thread)) {
+		audit_panic("cannot start thread audit_prune_tree");
+	} else {
+		wake_up_process(prune_thread);
+		audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
+		if (IS_ERR(audit_tree_group))
+			audit_panic("cannot initialize fsnotify group for rectree watches");
+	}
 	for (i = 0; i < HASH_SIZE; i++)
 		INIT_LIST_HEAD(&chunk_hash_heads[i]);
 
-- 
1.7.9.5

[PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

From: "Palik, Imre" <imrep@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_tree_init(), and thus avoids the deadlock that the on-demand thread
creation can cause.

An alternative approach would be to move the thread creation outside of the
lock.  This would assume that other layers of the filesystem code don't
hold any locks, and it would need some rewrite of the code to limit the
amount of threads possibly spawned.

Reported-by: Matt Wilson <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 kernel/audit_tree.c |   53 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 0caf1f8..cf6db88 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -37,6 +37,7 @@ struct audit_chunk {
 
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
+static struct task_struct *prune_thread;
 
 /*
  * One struct chunk is attached to each inode of interest.
@@ -806,30 +807,39 @@ int audit_tag_tree(char *old, char *new)
  */
 static int prune_tree_thread(void *unused)
 {
-	mutex_lock(&audit_cmd_mutex);
-	mutex_lock(&audit_filter_mutex);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&prune_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
 
-	while (!list_empty(&prune_list)) {
-		struct audit_tree *victim;
+		mutex_lock(&audit_cmd_mutex);
+		mutex_lock(&audit_filter_mutex);
 
-		victim = list_entry(prune_list.next, struct audit_tree, list);
-		list_del_init(&victim->list);
+		while (!list_empty(&prune_list)) {
+			struct audit_tree *victim;
 
-		mutex_unlock(&audit_filter_mutex);
+			victim = list_entry(prune_list.next,
+					struct audit_tree, list);
+			list_del_init(&victim->list);
 
-		prune_one(victim);
+			mutex_unlock(&audit_filter_mutex);
 
-		mutex_lock(&audit_filter_mutex);
-	}
+			prune_one(victim);
 
-	mutex_unlock(&audit_filter_mutex);
-	mutex_unlock(&audit_cmd_mutex);
+			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);
 }
 
 /*
@@ -896,9 +906,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);
+
 }
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
@@ -938,10 +949,16 @@ static int __init audit_tree_init(void)
 {
 	int i;
 
-	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
-	if (IS_ERR(audit_tree_group))
-		audit_panic("cannot initialize fsnotify group for rectree watches");
-
+	prune_thread = kthread_create(prune_tree_thread, NULL,
+				"audit_prune_tree");
+	if (IS_ERR(prune_thread)) {
+		audit_panic("cannot start thread audit_prune_tree");
+	} else {
+		wake_up_process(prune_thread);
+		audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
+		if (IS_ERR(audit_tree_group))
+			audit_panic("cannot initialize fsnotify group for rectree watches");
+	}
 	for (i = 0; i < HASH_SIZE; i++)
 		INIT_LIST_HEAD(&chunk_hash_heads[i]);
 
-- 
1.7.9.5

[PATCH RFC] audit: move the tree pruning to a dedicated thread

[Imre Palik]

From: "Palik, Imre" <imrep@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_tree_init(), and thus avoids the deadlock that the on-demand thread
creation can cause.

An alternative approach would be to move the thread creation outside of the
lock.  This would assume that other layers of the filesystem code don't
hold any locks, and it would need some rewrite of the code to limit the
amount of threads possibly spawned.

Reported-by: Matt Wilson <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 kernel/audit_tree.c |   53 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 0caf1f8..cf6db88 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -37,6 +37,7 @@ struct audit_chunk {
 
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
+static struct task_struct *prune_thread;
 
 /*
  * One struct chunk is attached to each inode of interest.
@@ -806,30 +807,39 @@ int audit_tag_tree(char *old, char *new)
  */
 static int prune_tree_thread(void *unused)
 {
-	mutex_lock(&audit_cmd_mutex);
-	mutex_lock(&audit_filter_mutex);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&prune_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
 
-	while (!list_empty(&prune_list)) {
-		struct audit_tree *victim;
+		mutex_lock(&audit_cmd_mutex);
+		mutex_lock(&audit_filter_mutex);
 
-		victim = list_entry(prune_list.next, struct audit_tree, list);
-		list_del_init(&victim->list);
+		while (!list_empty(&prune_list)) {
+			struct audit_tree *victim;
 
-		mutex_unlock(&audit_filter_mutex);
+			victim = list_entry(prune_list.next,
+					struct audit_tree, list);
+			list_del_init(&victim->list);
 
-		prune_one(victim);
+			mutex_unlock(&audit_filter_mutex);
 
-		mutex_lock(&audit_filter_mutex);
-	}
+			prune_one(victim);
 
-	mutex_unlock(&audit_filter_mutex);
-	mutex_unlock(&audit_cmd_mutex);
+			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);
 }
 
 /*
@@ -896,9 +906,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);
+
 }
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
@@ -938,10 +949,16 @@ static int __init audit_tree_init(void)
 {
 	int i;
 
-	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
-	if (IS_ERR(audit_tree_group))
-		audit_panic("cannot initialize fsnotify group for rectree watches");
-
+	prune_thread = kthread_create(prune_tree_thread, NULL,
+				"audit_prune_tree");
+	if (IS_ERR(prune_thread)) {
+		audit_panic("cannot start thread audit_prune_tree");
+	} else {
+		wake_up_process(prune_thread);
+		audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
+		if (IS_ERR(audit_tree_group))
+			audit_panic("cannot initialize fsnotify group for rectree watches");
+	}
 	for (i = 0; i < HASH_SIZE; i++)
 		INIT_LIST_HEAD(&chunk_hash_heads[i]);
 
-- 
1.7.9.5