Hi.
The following reproducer causes auditd daemon hang up.
(But the hang up is released after the audit_backlog_wait_time passes.)
# auditctl -a exit,always -S all
# reboot
I reproduced the hangup on KVM, and then got a crash dump.
After I analyzed the dump, I found auditd daemon hung up in audit_log_start.
(I have confirmed it on linux-3.12-rc4.)
Like this:
crash> bt 1426
PID: 1426 TASK: ffff88007b63e040 CPU: 1 COMMAND: "auditd"
#0 [ffff88007cb93918] __schedule at ffffffff8155d980
#1 [ffff88007cb939b0] schedule at ffffffff8155de99
#2 [ffff88007cb939c0] schedule_timeout at ffffffff8155b840
#3 [ffff88007cb93a60] audit_log_start at ffffffff810d3ce5
#4 [ffff88007cb93b20] audit_log_config_change at ffffffff810d3ece
#5 [ffff88007cb93b60] audit_receive_msg at ffffffff810d4fd6
#6 [ffff88007cb93c00] audit_receive at ffffffff810d5173
#7 [ffff88007cb93c30] netlink_unicast at ffffffff814c5269
#8 [ffff88007cb93c90] netlink_sendmsg at ffffffff814c6386
#9 [ffff88007cb93d20] sock_sendmsg at ffffffff814813c0
#10 [ffff88007cb93e30] SYSC_sendto at ffffffff81481524
#11 [ffff88007cb93f70] sys_sendto at ffffffff8148157e
#12 [ffff88007cb93f80] system_call_fastpath at ffffffff81568052
RIP: 00007f5c47f7fba3 RSP: 00007fffcf21a118 RFLAGS: 00010202
RAX: 000000000000002c RBX: ffffffff81568052 RCX: 0000000000000000
RDX: 0000000000000030 RSI: 00007fffcf21e7d0 RDI: 0000000000000003
RBP: 00007fffcf21e7d0 R8: 00007fffcf21a130 R9: 000000000000000c
R10: 0000000000000000 R11: 0000000000000293 R12: ffffffff8148157e
R13: ffff88007cb93f78 R14: 0000000000000020 R15: 0000000000000030
ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
The reason is that auditd daemon itself cannot consume its backlog
while audit_log_start is calling schedule_timeout on auditd daemon.
So, that is a deadlock!
Therefore, I think audit_log_start shouldn't handle auditd's backlog
when auditd daemon executes audit_log_start.
For example, I made the following fix patch.
--------------------------------------------------------------
auditd daemon can execute the audit_log_start, and then it can cause
a hang up because only auditd daemon can consume the backlog.
So, audit_log_start executed by auditd daemon should not handle the backlog
in case auditd daemon hangs up (while wait_for_auditd is calling).
Signed-off-by: Toshiyuki Okajima <[email protected]>
---
kernel/audit.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..86c389e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
int reserve;
unsigned long timeout_start = jiffies;
+ if (audit_pid && (audit_pid == current->pid))
+ return NULL;
+
if (audit_initialized != AUDIT_INITIALIZED)
return NULL;
--
1.5.5.6
On 10/11/2013 09:36 AM, Toshiyuki Okajima wrote:
> Hi.
>
> The following reproducer causes auditd daemon hang up.
> (But the hang up is released after the audit_backlog_wait_time passes.)
> # auditctl -a exit,always -S all
> # reboot
>
>
> I reproduced the hangup on KVM, and then got a crash dump.
> After I analyzed the dump, I found auditd daemon hung up in audit_log_start.
> (I have confirmed it on linux-3.12-rc4.)
>
> Like this:
> crash> bt 1426
> PID: 1426 TASK: ffff88007b63e040 CPU: 1 COMMAND: "auditd"
> #0 [ffff88007cb93918] __schedule at ffffffff8155d980
> #1 [ffff88007cb939b0] schedule at ffffffff8155de99
> #2 [ffff88007cb939c0] schedule_timeout at ffffffff8155b840
> #3 [ffff88007cb93a60] audit_log_start at ffffffff810d3ce5
> #4 [ffff88007cb93b20] audit_log_config_change at ffffffff810d3ece
> #5 [ffff88007cb93b60] audit_receive_msg at ffffffff810d4fd6
> #6 [ffff88007cb93c00] audit_receive at ffffffff810d5173
> #7 [ffff88007cb93c30] netlink_unicast at ffffffff814c5269
> #8 [ffff88007cb93c90] netlink_sendmsg at ffffffff814c6386
> #9 [ffff88007cb93d20] sock_sendmsg at ffffffff814813c0
> #10 [ffff88007cb93e30] SYSC_sendto at ffffffff81481524
> #11 [ffff88007cb93f70] sys_sendto at ffffffff8148157e
> #12 [ffff88007cb93f80] system_call_fastpath at ffffffff81568052
> RIP: 00007f5c47f7fba3 RSP: 00007fffcf21a118 RFLAGS: 00010202
> RAX: 000000000000002c RBX: ffffffff81568052 RCX: 0000000000000000
> RDX: 0000000000000030 RSI: 00007fffcf21e7d0 RDI: 0000000000000003
> RBP: 00007fffcf21e7d0 R8: 00007fffcf21a130 R9: 000000000000000c
> R10: 0000000000000000 R11: 0000000000000293 R12: ffffffff8148157e
> R13: ffff88007cb93f78 R14: 0000000000000020 R15: 0000000000000030
> ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
>
>
> The reason is that auditd daemon itself cannot consume its backlog
> while audit_log_start is calling schedule_timeout on auditd daemon.
> So, that is a deadlock!
>
> Therefore, I think audit_log_start shouldn't handle auditd's backlog
> when auditd daemon executes audit_log_start.
>
> For example, I made the following fix patch.
> --------------------------------------------------------------
> auditd daemon can execute the audit_log_start, and then it can cause
> a hang up because only auditd daemon can consume the backlog.
> So, audit_log_start executed by auditd daemon should not handle the backlog
> in case auditd daemon hangs up (while wait_for_auditd is calling).
>
> Signed-off-by: Toshiyuki Okajima <[email protected]>
> ---
> kernel/audit.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..86c389e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> int reserve;
> unsigned long timeout_start = jiffies;
>
> + if (audit_pid && (audit_pid == current->pid))
> + return NULL;
> +
audit_log_start can be called in interrupt context, such as iptables AUDIT module,
we can't use current here.
please try the patch below.
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..1f35f3d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -270,9 +270,13 @@ static int audit_log_config_change(char *function_name, int new, int old,
int allow_changes)
{
struct audit_buffer *ab;
+ gfp_t gfp_mask = GFP_KERNEL;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ if (audit_pid && audit_pid == current->pid)
+ gfp_mask = GFP_ATOMIC;
+
+ ab = audit_log_start(NULL, gfp_mask, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "%s=%d old=%d", function_name, new, old);
Thanks
Gao-san,
(2013/10/11 18:33), Gao feng wrote:
> On 10/11/2013 09:36 AM, Toshiyuki Okajima wrote:
>> Hi.
>>
>> The following reproducer causes auditd daemon hang up.
>> (But the hang up is released after the audit_backlog_wait_time passes.)
>> # auditctl -a exit,always -S all
>> # reboot
>>
>>
>> I reproduced the hangup on KVM, and then got a crash dump.
>> After I analyzed the dump, I found auditd daemon hung up in audit_log_start.
>> (I have confirmed it on linux-3.12-rc4.)
>>
>> Like this:
>> crash> bt 1426
>> PID: 1426 TASK: ffff88007b63e040 CPU: 1 COMMAND: "auditd"
>> #0 [ffff88007cb93918] __schedule at ffffffff8155d980
>> #1 [ffff88007cb939b0] schedule at ffffffff8155de99
>> #2 [ffff88007cb939c0] schedule_timeout at ffffffff8155b840
>> #3 [ffff88007cb93a60] audit_log_start at ffffffff810d3ce5
>> #4 [ffff88007cb93b20] audit_log_config_change at ffffffff810d3ece
>> #5 [ffff88007cb93b60] audit_receive_msg at ffffffff810d4fd6
>> #6 [ffff88007cb93c00] audit_receive at ffffffff810d5173
>> #7 [ffff88007cb93c30] netlink_unicast at ffffffff814c5269
>> #8 [ffff88007cb93c90] netlink_sendmsg at ffffffff814c6386
>> #9 [ffff88007cb93d20] sock_sendmsg at ffffffff814813c0
>> #10 [ffff88007cb93e30] SYSC_sendto at ffffffff81481524
>> #11 [ffff88007cb93f70] sys_sendto at ffffffff8148157e
>> #12 [ffff88007cb93f80] system_call_fastpath at ffffffff81568052
>> RIP: 00007f5c47f7fba3 RSP: 00007fffcf21a118 RFLAGS: 00010202
>> RAX: 000000000000002c RBX: ffffffff81568052 RCX: 0000000000000000
>> RDX: 0000000000000030 RSI: 00007fffcf21e7d0 RDI: 0000000000000003
>> RBP: 00007fffcf21e7d0 R8: 00007fffcf21a130 R9: 000000000000000c
>> R10: 0000000000000000 R11: 0000000000000293 R12: ffffffff8148157e
>> R13: ffff88007cb93f78 R14: 0000000000000020 R15: 0000000000000030
>> ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
>>
>>
>> The reason is that auditd daemon itself cannot consume its backlog
>> while audit_log_start is calling schedule_timeout on auditd daemon.
>> So, that is a deadlock!
>>
>> Therefore, I think audit_log_start shouldn't handle auditd's backlog
>> when auditd daemon executes audit_log_start.
>>
>> For example, I made the following fix patch.
>> --------------------------------------------------------------
>> auditd daemon can execute the audit_log_start, and then it can cause
>> a hang up because only auditd daemon can consume the backlog.
>> So, audit_log_start executed by auditd daemon should not handle the backlog
>> in case auditd daemon hangs up (while wait_for_auditd is calling).
>>
>> Signed-off-by: Toshiyuki Okajima <[email protected]>
>> ---
>> kernel/audit.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 7b0e23a..86c389e 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>> int reserve;
>> unsigned long timeout_start = jiffies;
>>
>> + if (audit_pid && (audit_pid == current->pid))
>> + return NULL;
>> +
>
> audit_log_start can be called in interrupt context, such as iptables AUDIT module,
> we can't use current here.
> please try the patch below.
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..1f35f3d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -270,9 +270,13 @@ static int audit_log_config_change(char *function_name, int new, int old,
> int allow_changes)
> {
> struct audit_buffer *ab;
> + gfp_t gfp_mask = GFP_KERNEL;
> int rc = 0;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + if (audit_pid && audit_pid == current->pid)
> + gfp_mask = GFP_ATOMIC;
> +
> + ab = audit_log_start(NULL, gfp_mask, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return rc;
> audit_log_format(ab, "%s=%d old=%d", function_name, new, old);
>
>
Thanks for your suggestion. I will try it.
Regards,
Toshiyuki Okajima
> Thanks
>
>
The backlog cannot be consumed when audit_log_start is running on auditd
even if audit_log_start calls wait_for_auditd to consume it.
The situation is a deadlock because only auditd can consume the backlog.
If the other process needs to send the backlog, it can be also stopped
by the deadlock.
So, audit_log_start running on auditd should not stop.
You can see the deadlock with the following reproducer:
# auditctl -a exit,always -S all
# reboot
Signed-off-by: Toshiyuki Okajima <[email protected]>
Cc: [email protected]
---
kernel/audit.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..ce1fb38 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
int reserve;
unsigned long timeout_start = jiffies;
+ if (audit_pid && audit_pid == current->pid)
+ gfp_mask &= ~__GFP_WAIT;
+
if (audit_initialized != AUDIT_INITIALIZED)
return NULL;
--
1.5.5.6
Hi Toshiyuki-san,
On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
> The backlog cannot be consumed when audit_log_start is running on auditd
> even if audit_log_start calls wait_for_auditd to consume it.
> The situation is a deadlock because only auditd can consume the backlog.
> If the other process needs to send the backlog, it can be also stopped
> by the deadlock.
>
> So, audit_log_start running on auditd should not stop.
>
> You can see the deadlock with the following reproducer:
> # auditctl -a exit,always -S all
> # reboot
>
> Signed-off-by: Toshiyuki Okajima <[email protected]>
> Cc: [email protected]
> ---
> kernel/audit.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..ce1fb38 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> int reserve;
> unsigned long timeout_start = jiffies;
>
> + if (audit_pid && audit_pid == current->pid)
> + gfp_mask &= ~__GFP_WAIT;
> +
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
>
Hmm, I see, There may be other code paths that auditd can call audit_log_start except
audit_log_config_change. so it's better to handle this problem in audit_log_start.
but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
so maybe the below patch is what you want.
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..10b4545 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
struct audit_buffer *ab = NULL;
struct timespec t;
unsigned int uninitialized_var(serial);
- int reserve;
+ int reserve = 5; /* Allow atomic callers to go up to five
+ entries over the normal backlog limit */
+
unsigned long timeout_start = jiffies;
if (audit_initialized != AUDIT_INITIALIZED)
@@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
if (unlikely(audit_filter_type(type)))
return NULL;
- if (gfp_mask & __GFP_WAIT)
- reserve = 0;
- else
- reserve = 5; /* Allow atomic callers to go up to five
- entries over the normal backlog limit */
+ if (gfp_mask & __GFP_WAIT) {
+ if (audit_pid && audit_pid == current->pid)
+ gfp_mask &= ~__GFP_WAIT;
+ else
+ reserve = 0;
+ }
while (audit_backlog_limit
&& skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
Thanks
Hi Gao-san,
(2013/10/15 15:30), Gao feng wrote:
> Hi Toshiyuki-san,
> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
>> The backlog cannot be consumed when audit_log_start is running on auditd
>> even if audit_log_start calls wait_for_auditd to consume it.
>> The situation is a deadlock because only auditd can consume the backlog.
>> If the other process needs to send the backlog, it can be also stopped
>> by the deadlock.
>>
>> So, audit_log_start running on auditd should not stop.
>>
>> You can see the deadlock with the following reproducer:
>> # auditctl -a exit,always -S all
>> # reboot
>>
>> Signed-off-by: Toshiyuki Okajima <[email protected]>
>> Cc: [email protected]
>> ---
>> kernel/audit.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 7b0e23a..ce1fb38 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1098,6 +1098,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>> int reserve;
>> unsigned long timeout_start = jiffies;
>>
>> + if (audit_pid && audit_pid == current->pid)
>> + gfp_mask &= ~__GFP_WAIT;
>> +
>> if (audit_initialized != AUDIT_INITIALIZED)
>> return NULL;
>>
>>
>
> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
Yeah. I found it when I reviewed the code paths to audit_log_start after I got your comment.
sys_sendto
-> sock_sendmsg
-> netlink_unicast
-> audit_receive
-> audit_receive_skb
-> audit_receive_msg
-> audit_log_config_change
-> audit_log_start
-> audit_log_common_recv_msg ***
-> audit_log_start ***
> audit_log_config_change. so it's better to handle this problem in audit_log_start.
>
> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
> so maybe the below patch is what you want.
OK.
I considered the code size, but I didn't consider the execution speed.
audit_log_start doesn't always need to execute "if (audit_pid && audit_pid == current->pid)"!
I will send your revised patch later.
Thanks,
Toshiyuki Okajima
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..10b4545 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
> struct audit_buffer *ab = NULL;
> struct timespec t;
> unsigned int uninitialized_var(serial);
> - int reserve;
> + int reserve = 5; /* Allow atomic callers to go up to five
> + entries over the normal backlog limit */
> +
> unsigned long timeout_start = jiffies;
>
> if (audit_initialized != AUDIT_INITIALIZED)
> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
> if (unlikely(audit_filter_type(type)))
> return NULL;
>
> - if (gfp_mask & __GFP_WAIT)
> - reserve = 0;
> - else
> - reserve = 5; /* Allow atomic callers to go up to five
> - entries over the normal backlog limit */
> + if (gfp_mask & __GFP_WAIT) {
> + if (audit_pid && audit_pid == current->pid)
> + gfp_mask &= ~__GFP_WAIT;
> + else
> + reserve = 0;
> + }
>
> while (audit_backlog_limit
> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
>
>
> Thanks
>
>
The backlog cannot be consumed when audit_log_start is running on auditd
even if audit_log_start calls wait_for_auditd to consume it.
The situation is the deadlock because only auditd can consume the backlog.
If the other process needs to send the backlog, it can be also stopped
by the deadlock.
So, audit_log_start running on auditd should not stop.
You can see the deadlock with the following reproducer:
# auditctl -a exit,always -S all
# reboot
Signed-off-by: Toshiyuki Okajima <[email protected]>
Cc: [email protected]
---
kernel/audit.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..29cfc94 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1095,7 +1095,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
struct audit_buffer *ab = NULL;
struct timespec t;
unsigned int uninitialized_var(serial);
- int reserve;
+ int reserve = 5; /* Allow atomic callers to go up to five
+ entries over the normal backlog limit */
unsigned long timeout_start = jiffies;
if (audit_initialized != AUDIT_INITIALIZED)
@@ -1104,11 +1105,12 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (unlikely(audit_filter_type(type)))
return NULL;
- if (gfp_mask & __GFP_WAIT)
- reserve = 0;
- else
- reserve = 5; /* Allow atomic callers to go up to five
- entries over the normal backlog limit */
+ if (gfp_mask & __GFP_WAIT) {
+ if (audit_pid && audit_pid == current->pid)
+ gfp_mask &= ~__GFP_WAIT;
+ else
+ reserve = 0;
+ }
while (audit_backlog_limit
&& skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
--
1.5.5.6
On 10/15/2013 03:58 PM, Toshiyuki Okajima wrote:
> The backlog cannot be consumed when audit_log_start is running on auditd
> even if audit_log_start calls wait_for_auditd to consume it.
> The situation is the deadlock because only auditd can consume the backlog.
> If the other process needs to send the backlog, it can be also stopped
> by the deadlock.
>
> So, audit_log_start running on auditd should not stop.
>
> You can see the deadlock with the following reproducer:
> # auditctl -a exit,always -S all
> # reboot
>
> Signed-off-by: Toshiyuki Okajima <[email protected]>
> Cc: [email protected]
> ---
> kernel/audit.c | 14 ++++++++------
> 1 files changed, 8 insertions(+), 6 deletions(-)
>
Looks good to me, thanks!
Reviewed-by: Gao feng <[email protected]>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..29cfc94 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1095,7 +1095,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> struct audit_buffer *ab = NULL;
> struct timespec t;
> unsigned int uninitialized_var(serial);
> - int reserve;
> + int reserve = 5; /* Allow atomic callers to go up to five
> + entries over the normal backlog limit */
> unsigned long timeout_start = jiffies;
>
> if (audit_initialized != AUDIT_INITIALIZED)
> @@ -1104,11 +1105,12 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> if (unlikely(audit_filter_type(type)))
> return NULL;
>
> - if (gfp_mask & __GFP_WAIT)
> - reserve = 0;
> - else
> - reserve = 5; /* Allow atomic callers to go up to five
> - entries over the normal backlog limit */
> + if (gfp_mask & __GFP_WAIT) {
> + if (audit_pid && audit_pid == current->pid)
> + gfp_mask &= ~__GFP_WAIT;
> + else
> + reserve = 0;
> + }
>
> while (audit_backlog_limit
> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
>
On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
> Hi Toshiyuki-san,
Toshiuki and Gao,
> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
> > The backlog cannot be consumed when audit_log_start is running on auditd
> > even if audit_log_start calls wait_for_auditd to consume it.
> > The situation is a deadlock because only auditd can consume the backlog.
> > If the other process needs to send the backlog, it can be also stopped
> > by the deadlock.
> >
> > So, audit_log_start running on auditd should not stop.
> >
> > You can see the deadlock with the following reproducer:
> > # auditctl -a exit,always -S all
> > # reboot
> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
> audit_log_config_change. so it's better to handle this problem in audit_log_start.
>
> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
> so maybe the below patch is what you want.
I have been following this thread with interest. I like the general
evolution of this patch. The first patch was a bit too abrupt, dropping
too much, but this one makes much more sense. I would be tempted to
make the reserve even bigger.
I see that you should be using a kernel that has included commit
8ac1c8d5 (which made it into v3.12-rc3)
audit: fix endless wait in audit_log_start()
That was an obvious bug, but I was still concerned about the cause of
the initial wait. There are other fixes and ideas in the works that
should alleviate some of the pressure to make the service more usable.
https://lkml.org/lkml/2013/9/18/453
I have tested with and without this v3 patch and I don't see any
significant difference with the reproducer provided above. I'm also
testing with a reproducer of the endless wait bug (readahead-collector).
What are your expected results? What are your actual results in each
case? How are they different?
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..10b4545 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
> struct audit_buffer *ab = NULL;
> struct timespec t;
> unsigned int uninitialized_var(serial);
> - int reserve;
> + int reserve = 5; /* Allow atomic callers to go up to five
> + entries over the normal backlog limit */
> +
> unsigned long timeout_start = jiffies;
>
> if (audit_initialized != AUDIT_INITIALIZED)
> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
> if (unlikely(audit_filter_type(type)))
> return NULL;
>
> - if (gfp_mask & __GFP_WAIT)
> - reserve = 0;
> - else
> - reserve = 5; /* Allow atomic callers to go up to five
> - entries over the normal backlog limit */
> + if (gfp_mask & __GFP_WAIT) {
> + if (audit_pid && audit_pid == current->pid)
> + gfp_mask &= ~__GFP_WAIT;
> + else
> + reserve = 0;
> + }
>
> while (audit_backlog_limit
> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
On 10/24/2013 03:55 AM, Richard Guy Briggs wrote:
> On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
>> Hi Toshiyuki-san,
>
> Toshiuki and Gao,
>
>> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
>>> The backlog cannot be consumed when audit_log_start is running on auditd
>>> even if audit_log_start calls wait_for_auditd to consume it.
>>> The situation is a deadlock because only auditd can consume the backlog.
>>> If the other process needs to send the backlog, it can be also stopped
>>> by the deadlock.
>>>
>>> So, audit_log_start running on auditd should not stop.
>>>
>>> You can see the deadlock with the following reproducer:
>>> # auditctl -a exit,always -S all
>>> # reboot
>
>> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
>> audit_log_config_change. so it's better to handle this problem in audit_log_start.
>>
>> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
>> so maybe the below patch is what you want.
>
> I have been following this thread with interest. I like the general
> evolution of this patch. The first patch was a bit too abrupt, dropping
> too much, but this one makes much more sense. I would be tempted to
> make the reserve even bigger.
>
> I see that you should be using a kernel that has included commit
> 8ac1c8d5 (which made it into v3.12-rc3)
> audit: fix endless wait in audit_log_start()
> That was an obvious bug,
include or not include?
The problem is, if the audit_backlog_limit is 3, but there are 5 tasks
calling audit_log_start, so 2 tasks will wait auditd to consume
audit_skb_queue. if before auditd consumes skbs, somebody want to kill
auditd, and auditd will set the audit_pid to zero, this will triger an
audit message. so auditd will wait for himself. and this waiting is endless,
since auditd cann't consume audit_skb_queue any more.
the commit 8ac1c8d5 prevent this problem happening. because if once a task is
blocked over the audit_backlog_wait_time. the audit_backlog_wait_time will
be set to zero(audit_backlog_wait_overflow which is zero). so the other tasks
will not wait anymore. but I'm confused if this is what we expected? these
audit messages will lost once any task is blocked over audit_backlog_wait_time.
So, AFAIK if commit 8ac1c8d5 exist, this patch is not necessary, but
we still do something to fix the problem commit 8ac1c8d5 brings.
but I was still concerned about the cause of
> the initial wait. There are other fixes and ideas in the works that
> should alleviate some of the pressure to make the service more usable.
> https://lkml.org/lkml/2013/9/18/453
>
> I have tested with and without this v3 patch and I don't see any
> significant difference with the reproducer provided above. I'm also
> testing with a reproducer of the endless wait bug (readahead-collector).
>
> What are your expected results? What are your actual results in each
> case? How are they different?
>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 7b0e23a..10b4545 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
>> struct audit_buffer *ab = NULL;
>> struct timespec t;
>> unsigned int uninitialized_var(serial);
>> - int reserve;
>> + int reserve = 5; /* Allow atomic callers to go up to five
>> + entries over the normal backlog limit */
>> +
>> unsigned long timeout_start = jiffies;
>>
>> if (audit_initialized != AUDIT_INITIALIZED)
>> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
>> if (unlikely(audit_filter_type(type)))
>> return NULL;
>>
>> - if (gfp_mask & __GFP_WAIT)
>> - reserve = 0;
>> - else
>> - reserve = 5; /* Allow atomic callers to go up to five
>> - entries over the normal backlog limit */
>> + if (gfp_mask & __GFP_WAIT) {
>> + if (audit_pid && audit_pid == current->pid)
>> + gfp_mask &= ~__GFP_WAIT;
>> + else
>> + reserve = 0;
>> + }
>>
>> while (audit_backlog_limit
>> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
>
> - RGB
>
> --
> Richard Guy Briggs <[email protected]>
> Senior Software Engineer
> Kernel Security
> AMER ENG Base Operating Systems
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635
> Internal: (81) 32635
> Alt: +1.613.693.0684x3545
>
On Thu, Oct 24, 2013 at 01:55:37PM +0800, Gao feng wrote:
> On 10/24/2013 03:55 AM, Richard Guy Briggs wrote:
> > On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
> >> Hi Toshiyuki-san,
> >
> > Toshiuki and Gao,
> >
> >> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
> >>> The backlog cannot be consumed when audit_log_start is running on auditd
> >>> even if audit_log_start calls wait_for_auditd to consume it.
> >>> The situation is a deadlock because only auditd can consume the backlog.
> >>> If the other process needs to send the backlog, it can be also stopped
> >>> by the deadlock.
> >>>
> >>> So, audit_log_start running on auditd should not stop.
> >>>
> >>> You can see the deadlock with the following reproducer:
> >>> # auditctl -a exit,always -S all
> >>> # reboot
> >
> >> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
> >> audit_log_config_change. so it's better to handle this problem in audit_log_start.
> >>
> >> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
> >> so maybe the below patch is what you want.
> >
> > I have been following this thread with interest. I like the general
> > evolution of this patch. The first patch was a bit too abrupt, dropping
> > too much, but this one makes much more sense. I would be tempted to
> > make the reserve even bigger.
> >
> > I see that you should be using a kernel that has included commit
> > 8ac1c8d5 (which made it into v3.12-rc3)
> > audit: fix endless wait in audit_log_start()
> > That was an obvious bug,
>
> include or not include?
You say you tested on v3.12-rc4, which does (should?) include this fix.
> The problem is, if the audit_backlog_limit is 3, but there are 5 tasks
> calling audit_log_start, so 2 tasks will wait auditd to consume
> audit_skb_queue. if before auditd consumes skbs, somebody want to kill
> auditd, and auditd will set the audit_pid to zero, this will triger an
> audit message. so auditd will wait for himself. and this waiting is endless,
> since auditd cann't consume audit_skb_queue any more.
>
> the commit 8ac1c8d5 prevent this problem happening. because if once a task is
> blocked over the audit_backlog_wait_time. the audit_backlog_wait_time will
> be set to zero(audit_backlog_wait_overflow which is zero). so the other tasks
> will not wait anymore. but I'm confused if this is what we expected? these
> audit messages will lost once any task is blocked over audit_backlog_wait_time.
This is configurable. The default is this behaviour, so that if there
is a problem with the audit subsystem, it will give a reasonable chance
for auditd to catch up, then give up, make noise in the syslog (and
configured colsoles) and then continue. Other auditd configurations
allow the system to halt if this condition occurs.
There is another patch coming that will set that audit_backlog_wait_time
back to the original value once auditd has recovered. It is not
upstream yet.
> So, AFAIK if commit 8ac1c8d5 exist, this patch is not necessary, but
> we still do something to fix the problem commit 8ac1c8d5 brings.
This is the condition in which I am interested. Do you have a way to
clearly show this condition without your patch and a way to show it is
solved with your patch? I'm looking for a clear reproducer.
> but I was still concerned about the cause of
> > the initial wait. There are other fixes and ideas in the works that
> > should alleviate some of the pressure to make the service more usable.
> > https://lkml.org/lkml/2013/9/18/453
> >
> > I have tested with and without this v3 patch and I don't see any
> > significant difference with the reproducer provided above. I'm also
> > testing with a reproducer of the endless wait bug (readahead-collector).
> >
> > What are your expected results? What are your actual results in each
> > case? How are they different?
> >
> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> index 7b0e23a..10b4545 100644
> >> --- a/kernel/audit.c
> >> +++ b/kernel/audit.c
> >> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
> >> struct audit_buffer *ab = NULL;
> >> struct timespec t;
> >> unsigned int uninitialized_var(serial);
> >> - int reserve;
> >> + int reserve = 5; /* Allow atomic callers to go up to five
> >> + entries over the normal backlog limit */
> >> +
> >> unsigned long timeout_start = jiffies;
> >>
> >> if (audit_initialized != AUDIT_INITIALIZED)
> >> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
> >> if (unlikely(audit_filter_type(type)))
> >> return NULL;
> >>
> >> - if (gfp_mask & __GFP_WAIT)
> >> - reserve = 0;
> >> - else
> >> - reserve = 5; /* Allow atomic callers to go up to five
> >> - entries over the normal backlog limit */
> >> + if (gfp_mask & __GFP_WAIT) {
> >> + if (audit_pid && audit_pid == current->pid)
> >> + gfp_mask &= ~__GFP_WAIT;
> >> + else
> >> + reserve = 0;
> >> + }
> >>
> >> while (audit_backlog_limit
> >> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
> >
> > - RGB
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
Hi.
Thanks for your interest.
(2013/10/24 4:55), Richard Guy Briggs wrote:
> On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
>> Hi Toshiyuki-san,
>
> Toshiuki and Gao,
>
>> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
>>> The backlog cannot be consumed when audit_log_start is running on auditd
>>> even if audit_log_start calls wait_for_auditd to consume it.
>>> The situation is a deadlock because only auditd can consume the backlog.
>>> If the other process needs to send the backlog, it can be also stopped
>>> by the deadlock.
>>>
>>> So, audit_log_start running on auditd should not stop.
>>>
>>> You can see the deadlock with the following reproducer:
>>> # auditctl -a exit,always -S all
>>> # reboot
>
>> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
>> audit_log_config_change. so it's better to handle this problem in audit_log_start.
>>
>> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
>> so maybe the below patch is what you want.
>
> I have been following this thread with interest. I like the general
> evolution of this patch. The first patch was a bit too abrupt, dropping
> too much, but this one makes much more sense. I would be tempted to
> make the reserve even bigger.
>
> I see that you should be using a kernel that has included commit
> 8ac1c8d5 (which made it into v3.12-rc3)
> audit: fix endless wait in audit_log_start()
> That was an obvious bug, but I was still concerned about the cause of
> the initial wait. There are other fixes and ideas in the works that
> should alleviate some of the pressure to make the service more usable.
> https://lkml.org/lkml/2013/9/18/453
>
> I have tested with and without this v3 patch and I don't see any
> significant difference with the reproducer provided above. I'm also
> testing with a reproducer of the endless wait bug (readahead-collector).
>
> What are your expected results? What are your actual results in each
> case? How are they different?
OK. I try to describe.
1) I found that the system cannot reboot smoothly because auditd daemon can stop for 60 seconds.
2) As the result of my investigation of this problem, I found that audit_log_start executed
on auditd called wait_for_auditd and stopped for "audit_backlog_wait_time".
Calling wait_for_auditd is to let auditd consume the backlog during this sleep.
However, when wait_for_auditd is called on auditd, the backlog cannot be consumed during this time.
In addition, audit_log_start called by other processes can also stop while auditd is stopping.
So, wait_for_auditd called on auditd is meaningless.
3) Therefore, I made the v3 patch that audit_log_start executed on auditd doesn't call wait_for_auditd.
4) On my environment, I couldn't see the situation that auditd stopped for 60 seconds by using
this patch.
5) So, I thought this problem could be fixed.
But after I received your comment, I encountered the other problem that auditd can stop for
60 seconds when I tried this patch on the other environment installed "systemd".
So, I think you encountered the other problem that was the same as mine.
The following situation was occurred:
zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
PID: 1 TASK: ffff88007c7d8000 CPU: 0 COMMAND: "systemd"
#0 [ffff88007c7758d8] __schedule at ffffffff815f231a
#1 [ffff88007c775950] schedule at ffffffff815f2a79
#2 [ffff88007c775960] schedule_timeout at ffffffff815f0f93
#3 [ffff88007c775a10] audit_log_start at ffffffff810d7d09
#4 [ffff88007c775ab0] audit_log_common_recv_msg at ffffffff810d8068
#5 [ffff88007c775b00] audit_receive_msg at ffffffff810d8ca0
#6 [ffff88007c775bb0] audit_receive at ffffffff810d8f98
#7 [ffff88007c775be0] netlink_unicast at ffffffff8150bf31
#8 [ffff88007c775c30] netlink_sendmsg at ffffffff8150c2b1
#9 [ffff88007c775cc0] sock_sendmsg at ffffffff814ca3fc
#10 [ffff88007c775e50] sys_sendto at ffffffff814ccd6d
#11 [ffff88007c775f80] system_call_fastpath at ffffffff815fc259
RIP: 00007fbee5653543 RSP: 00007fff23d684b8 RFLAGS: 00000246
RAX: 000000000000002c RBX: ffffffff815fc259 RCX: ffffffffffffffff
RDX: 0000000000000078 RSI: 00007fff23d5d8c0 RDI: 0000000000000003
RBP: 0000000000000003 R8: 00007fff23d5d8b0 R9: 000000000000000c
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000050
R13: 00007fff23d66380 R14: 000000000000046b R15: 0000000000000067
ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
PID: 488 TASK: ffff880036edcbf0 CPU: 1 COMMAND: "auditd"
#0 [ffff88007ba93a98] __schedule at ffffffff815f231a
#1 [ffff88007ba93b10] schedule at ffffffff815f2a79
#2 [ffff88007ba93b20] schedule_preempt_disabled at ffffffff815f2d0e
#3 [ffff88007ba93b30] __mutex_lock_slowpath at ffffffff815f1903
#4 [ffff88007ba93b90] mutex_lock at ffffffff815f143a
#5 [ffff88007ba93bb0] audit_receive at ffffffff810d8f71
#6 [ffff88007ba93be0] netlink_unicast at ffffffff8150bf31
#7 [ffff88007ba93c30] netlink_sendmsg at ffffffff8150c2b1
#8 [ffff88007ba93cc0] sock_sendmsg at ffffffff814ca3fc
#9 [ffff88007ba93e50] sys_sendto at ffffffff814ccd6d
#10 [ffff88007ba93f80] system_call_fastpath at ffffffff815fc259
RIP: 00007fc256ff16f3 RSP: 00007fffff6b39d8 RFLAGS: 00010202
RAX: 000000000000002c RBX: ffffffff815fc259 RCX: 0000000000000000
RDX: 0000000000000030 RSI: 00007fffff6b3a00 RDI: 0000000000000003
RBP: 0000000000000003 R8: 00007fffff6b39f0 R9: 000000000000000c
R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000005
R13: 00007fffff6ba3f0 R14: 00000000000003e9 R15: 0000000000000020
ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
systemd |auditd
-------------------------------------------+-----------------------------------
... |
-> audit_receive |...
-> mutex_lock(&audit_cmd_mutex) |-> audit_receive
... -> audit_log_start | -> mutex_lock(&audit_cmd_mutex)
-> wait_for_auditd | // wait for systemd
-> schedule_timeout(60*HZ) |
zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
V3 patch cannot fix the above problem. V3 patch only fixes the problem that audit_log_start
on auditd stops for 60 seconds in vain. The other problem is that auditd stops for
about 60 seconds by the mutex lock which the other process gets.
If the other process is "systemd", changing V3 patch like the following fixes the other problem.
(I confirm that the new v3 patch can fix the other problem which I encountered on installed
"systemd" environment.)
zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
@@ -1104,11 +1105,12 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (unlikely(audit_filter_type(type)))
return NULL;
- if (gfp_mask & __GFP_WAIT)
- reserve = 0;
- else
- reserve = 5; /* Allow atomic callers to go up to five
- entries over the normal backlog limit */
+ if (gfp_mask & __GFP_WAIT) {
+ if (audit_pid && (current->pid == 1 || audit_pid == current->pid))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+ gfp_mask &= ~__GFP_WAIT;
+ else
+ reserve = 0;
+ }
zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
However, if the other process is not "systemd", another problem cannot be corrected by a
simple correction.
At present, a drastic method to fix the other problem doesn't occur to me.
Regards,
Toshiyuki Okajima
>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 7b0e23a..10b4545 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
>> struct audit_buffer *ab = NULL;
>> struct timespec t;
>> unsigned int uninitialized_var(serial);
>> - int reserve;
>> + int reserve = 5; /* Allow atomic callers to go up to five
>> + entries over the normal backlog limit */
>> +
>> unsigned long timeout_start = jiffies;
>>
>> if (audit_initialized != AUDIT_INITIALIZED)
>> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
>> if (unlikely(audit_filter_type(type)))
>> return NULL;
>>
>> - if (gfp_mask & __GFP_WAIT)
>> - reserve = 0;
>> - else
>> - reserve = 5; /* Allow atomic callers to go up to five
>> - entries over the normal backlog limit */
>> + if (gfp_mask & __GFP_WAIT) {
>> + if (audit_pid && audit_pid == current->pid)
>> + gfp_mask &= ~__GFP_WAIT;
>> + else
>> + reserve = 0;
>> + }
>>
>> while (audit_backlog_limit
>> && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv
>
> - RGB
>
> --
> Richard Guy Briggs <[email protected]>
> Senior Software Engineer
> Kernel Security
> AMER ENG Base Operating Systems
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635
> Internal: (81) 32635
> Alt: +1.613.693.0684x3545
>
>
On Fri, 2013-10-25 at 10:36 +0900, Toshiyuki Okajima wrote:
> systemd |auditd
> -------------------------------------------+-----------------------------------
> ... |
> -> audit_receive |...
> -> mutex_lock(&audit_cmd_mutex) |-> audit_receive
> ... -> audit_log_start | -> mutex_lock(&audit_cmd_mutex)
> -> wait_for_auditd | // wait for systemd
> -> schedule_timeout(60*HZ) |
Ugggh, definitely a problem. Adding a similar hack to systemd really
does not seem like an acceptable answer. It seems to me that in
audit_receive_msg()
case AUDIT_USER:
case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
we do not need to hold the audit_cmd_mutex. So a quick and dirty patch
should be to just drop the mutex there (and we need to verify there
aren't issues running the audit_filter_user() without the lock). That
will take care of systemd and anything USING audit. It still means that
you could race with something configuring audit and auditd shutting
down. Seems like a good quick and dirty 'fix' while we work on a better
fix...
To take care of that I think maybe we could drop the cmd_mutex every
time we call audit_log_start. That's not necessarily going to be
pretty. Maybe make a new switch at the top of the function which knows
which operations we are going to have to allocate an audit_buffer. Drop
the lock, allocate the buffer, then retake the lock to finish running
audit_receive_msg()....
Maybe that second option isn't so hard and we can go directly after that
instead of just dealing with userspace audit messages?
Thoughts?
Hi.
(2013/10/26 0:12), Eric Paris wrote:
> On Fri, 2013-10-25 at 10:36 +0900, Toshiyuki Okajima wrote:
>
>> systemd |auditd
>> -------------------------------------------+-----------------------------------
>> ... |
>> -> audit_receive |...
>> -> mutex_lock(&audit_cmd_mutex) |-> audit_receive
>> ... -> audit_log_start | -> mutex_lock(&audit_cmd_mutex)
>> -> wait_for_auditd | // wait for systemd
>> -> schedule_timeout(60*HZ) |
>
> Ugggh, definitely a problem. Adding a similar hack to systemd really
> does not seem like an acceptable answer. It seems to me that in
I think so, too. We should fix it against the various cases.
> audit_receive_msg()
>
> case AUDIT_USER:
> case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
> case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
>
> we do not need to hold the audit_cmd_mutex. So a quick and dirty patch
> should be to just drop the mutex there (and we need to verify there
> aren't issues running the audit_filter_user() without the lock). That
> will take care of systemd and anything USING audit. It still means that
> you could race with something configuring audit and auditd shutting
> down. Seems like a good quick and dirty 'fix' while we work on a better
> fix...
>
> To take care of that I think maybe we could drop the cmd_mutex every
> time we call audit_log_start. That's not necessarily going to be
> pretty. Maybe make a new switch at the top of the function which knows
> which operations we are going to have to allocate an audit_buffer. Drop
> the lock, allocate the buffer, then retake the lock to finish running
> audit_receive_msg()....
>
> Maybe that second option isn't so hard and we can go directly after that
> instead of just dealing with userspace audit messages?
>
> Thoughts?
Does it mean that we can also fix the problem only in the userspace?
Even if we fix userspace process (auditd, readahead-collector and systemd) only,
the problem would happen again if a new userspace audit process is implemented.
Therefore, I think we should fix only in the kernel.
Sorry, but I don't have clear method to fix it.
Regards,
Toshiyuki Okajima
>
>
>