11 a.m.
Hello,
There's design flaw (in my opinion) regarding how this function works. It's
purpose is to try to send packets to userspace. It calls netlink_unicast. If
the return value is < 0, it claims the netlink socket is too busy and marks a
packet as lost.
The fact is that as long as we have space in the backlog, we don't have to
lose a packet do we? Can we not defer claiming we've lost one and try again
later? What is the purpose of the backlog if we can't defer delivery? Also,
shouldn't we put retval in the "too busy" message to help troubleshooting?
-Steve
11:12 a.m.
On Wednesday 06 April 2005 12:00, Steve Grubb wrote:
The fact is that as long as we have space in the backlog, we
don't have to
lose a packet do we?
Specifically, I'm talking about this:
522 if (retval == -EAGAIN && ab->count < 5) {
523 ++ab->count;
524 skb_queue_tail(&ab->sklist, skb);
525 audit_log_end_irq(ab);
526 return 1;
527 }
ab->count < 5 sounds arbitrary. I'd rather see it:
if (retval == -EAGAIN && audit_backlog < (audit_backlog_limit-1))
Does this make more sense? The idea is to try to leave room for 1 more packet
that could trigger the audit_log_drain.
-Steve
11:41 a.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
On Wednesday 06 April 2005 12:00, Steve Grubb wrote:
> The fact is that as long as we have space in the backlog, we don't have to
> lose a packet do we?
Specifically, I'm talking about this:
522 if (retval == -EAGAIN && ab->count < 5) {
523 ++ab->count;
524 skb_queue_tail(&ab->sklist, skb);
525 audit_log_end_irq(ab);
526 return 1;
527 }
ab->count < 5 sounds arbitrary. I'd rather see it:
if (retval == -EAGAIN && audit_backlog < (audit_backlog_limit-1))
Does this make more sense? The idea is to try to leave room for 1 more packet
that could trigger the audit_log_drain.
That's different. 5 retries per audit buffer (yes I agree sounds random,
no idea where it came from), which could have multiple skb's. But max
outstanding audit_buffers of audit_backlog_limit. I messed with this a
while back, and I found the retry logic was nearly always just a small
delay to "netlink socket too busy". So, I agree, there's room for
improvement.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
3:10 p.m.
On Wednesday 06 April 2005 12:41, Chris Wright wrote:
So, I agree, there's room for improvement.
I have applied the following patch and ran Kris's test program. Didn't lose
any netlink packets and didn't need to raise the backlog limit from 64. I
bumped her test up to 100,000 loops. The audit daemon rotated logfiles and
did not drop a single packet.
I decided to leave 3 openings in the backlog in hopes of allowing something to
be enqueued that may trigger audit_log_drain.
Signed-Off-By: Steve Grubb<sgrubb(a)redhat.com>
diff -ur linux-2.6.9.orig/kernel/audit.c linux-2.6.9/kernel/audit.c
--- linux-2.6.9.orig/kernel/audit.c 2005-04-06 12:27:28.000000000 -0400
+++ linux-2.6.9/kernel/audit.c 2005-04-06 12:30:21.000000000 -0400
@@ -143,7 +143,6 @@
int total;
int type;
int pid;
- int count; /* Times requeued */
};
void audit_set_type(struct audit_buffer *ab, int type)
@@ -534,8 +533,8 @@
retval = netlink_unicast(audit_sock, skb, audit_pid,
MSG_DONTWAIT);
}
- if (retval == -EAGAIN && ab->count < 5) {
- ++ab->count;
+ if (retval == -EAGAIN &&
+ (atomic_read(&audit_backlog)+3) < audit_backlog_limit)
{ skb_queue_tail(&ab->sklist, skb);
audit_log_end_irq(ab);
return 1;
@@ -675,7 +674,6 @@
ab->total = 0;
ab->type = AUDIT_KERNEL;
ab->pid = 0;
- ab->count = 0;
#ifdef CONFIG_AUDITSYSCALL
if (ab->ctx)
3:57 p.m.
Hunk #1 FAILED at 143.
patch: **** malformed patch at line 91: {
skb_queue_tail(&ab->sklist, skb);
On the offending line, why skb_queue_tail() -- shouldn't this queue back
at the _head_ of the queue?
(I know it was already like that before you touched it, Steve, but I
still don't think it's _right_).
--
dwmw2
7:09 p.m.
* David Woodhouse (dwmw2(a)infradead.org) wrote:
Hunk #1 FAILED at 143.
patch: **** malformed patch at line 91: {
skb_queue_tail(&ab->sklist, skb);
On the offending line, why skb_queue_tail() -- shouldn't this queue back
at the _head_ of the queue?
(I know it was already like that before you touched it, Steve, but I
still don't think it's _right_).
Actually, I agree. What's the point? We should requeue the skb that we
choked on at the head to help guarantee that it's (as close as possible
to) the first retransmitted.
thanks,
-chris
7:33 p.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
On Wednesday 06 April 2005 12:41, Chris Wright wrote:
> So, I agree, there's room for improvement.
I have applied the following patch and ran Kris's test program. Didn't lose
any netlink packets and didn't need to raise the backlog limit from 64. I
bumped her test up to 100,000 loops. The audit daemon rotated logfiles and
did not drop a single packet.
What is Kris's test program? I was simply using something like:
while :
do
< /dev/null
done
With an audit rule to match that open. This causes congestion
immediately.
I decided to leave 3 openings in the backlog in hopes of allowing
something
to be enqueued that may trigger audit_log_drain.
It shouldn't matter. The act of dropping should re-schedule a drain.
I'd rather not see magic numbers (esp. if they are somewhat arbitrary).
For my test, it at least always waits until the backlog to give a
too full/busy message. My experience has been that once congested
there's little to no recovery that will happen, so in that sense the
change borders a bit on academic. I never tested with an audit buffer
with long skb list. But, given all that I don't see the value of the
old code. This differs from yours only in that I drop the 3, and change
to requeueing at the head. Does it still work for you?
thanks,
-chris
--
===== kernel/audit.c 1.11 vs edited =====
--- 1.11/kernel/audit.c 2005-03-11 12:32:31 -08:00
+++ edited/kernel/audit.c 2005-04-06 15:28:52 -07:00
@@ -142,7 +142,6 @@ struct audit_buffer {
int total;
int type;
int pid;
- int count; /* Times requeued */
};
void audit_set_type(struct audit_buffer *ab, int type)
@@ -522,9 +531,9 @@ static inline int audit_log_drain(struct
retval = netlink_unicast(audit_sock, skb, audit_pid,
MSG_DONTWAIT);
}
- if (retval == -EAGAIN && ab->count < 5) {
- ++ab->count;
- skb_queue_tail(&ab->sklist, skb);
+ if (retval == -EAGAIN &&
+ (atomic_read(&audit_backlog)) < audit_backlog_limit) {
+ skb_queue_head(&ab->sklist, skb);
audit_log_end_irq(ab);
return 1;
}
@@ -662,7 +676,6 @@ struct audit_buffer *audit_log_start(str
ab->total = 0;
ab->type = AUDIT_KERNEL;
ab->pid = 0;
- ab->count = 0;
#ifdef CONFIG_AUDITSYSCALL
if (ab->ctx)
8:02 a.m.
On Wednesday 06 April 2005 20:33, Chris Wright wrote:
What is Kris's test program? I was simply using something like:
while :
do
< /dev/null
done
With an audit rule to match that open. This causes congestion
immediately.
That's good enough.
> I decided to leave 3 openings in the backlog in hopes of
allowing
> something to be enqueued that may trigger audit_log_drain.
It shouldn't matter. The act of dropping should re-schedule a drain.
I'd rather not see magic numbers (esp. if they are somewhat arbitrary).
OK, I found the path of code that does that.
For my test, it at least always waits until the backlog to give a
too full/busy message. My experience has been that once congested
there's little to no recovery that will happen, so in that sense the
change borders a bit on academic.
I think we should get to the root cause of this. What I'm seeing is the audit
daemon sleeping because time slice ends. Audit records pile up because test
program has time slice, audit daemon wakes up drains many records, loses time
slice and records pile up.
It seems that it should either be woken up, given the time slice, or a
temporary boost in priority.
This differs from yours only in that I drop the 3, and change to
requeueing
at the head. Does it still work for you?
It'll be late today before I find out. The changes are minor so I don't expect
a regression.
One thing I haven't investigated, though, is how the skb reference counting
works out. We call skb_get, does netlink_unicast always decrement the count
even on errors?
-Steve
11:35 a.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
There's design flaw (in my opinion) regarding how this function
works. It's
purpose is to try to send packets to userspace. It calls netlink_unicast. If
the return value is < 0, it claims the netlink socket is too busy and marks a
packet as lost.
It tries to backlog the packet (it requeues the skb). My experience is
it's never made a difference (but that could be an issue because of the
retry happening too fast). Once the netlink socket send does EAGAIN
it's very likely to not recover (but hey, at least it's not a huge
memory leak like it used to be ;-)
The fact is that as long as we have space in the backlog, we
don't have to
lose a packet do we? Can we not defer claiming we've lost one and try again
later?
We do that. But 5 retries on same audit buffer sklist causes the real
error.
What is the purpose of the backlog if we can't defer delivery?
Also,
shouldn't we put retval in the "too busy" message to help troubleshooting?
Sure could.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
7198
days inactive
7199
days old
linux-audit@lists.linux-audit.osci.io
8 comments
3 participants
participants (3)
-
Chris Wright -
David Woodhouse -
Steve Grubb