Repository navigation
Conversation
|
Thanks. First pass review, looks good, but instead of "count_dropped", I think functionally it should be "increment_plugin_dropped" so that the code that calls it reads better. Might also adjust the variable name to make it clear what drop is being counted. After a deeper reading of the code: Cover failures before enqueueing. In dispatch_event(), count allocation failures and existing validation rejections when a record was intended for plugins. Don’t count the normal “no plugins configured” return, and don’t increment again when libdisp_enqueue() returns an error already counted by the queue. (If this happens, we've had a bad day and audit is very unhealthy. But cover it for completeness.) Count subsequent records skipped for a failed plugin. A plugin marked inactive after a runtime failure remains an intended recipient until configuration removes it. Otherwise, the triggering loss gets counted but potentially millions of subsequent losses remain invisible. Also cover the dispatcher’s formatting/discard path. Exclude shutdown-related skips. Tests should be updated accordingly. And with this expansion, the placement of the variable might or might not need moving. |
|
Thanks for the review! Makes sense, I'll rename it to increment_plugin_dropped and give the counter a name that says which drop it counts. I'll also widen it to the cases you list: the failures in dispatch_event() without double counting what libdisp_enqueue() already counted, every record skipped for a plugin that went inactive, and the formatting/discard path, with shutdown left out. Tests will cover each of them, and I'll see where the counter fits best once all the callers are in, then push it here. |
The state report only says "plugin queue overflow detected = yes" once the plugin queue has overflowed. The syslog warning stops after five messages and overflow_action=suspend logs once, then rejects everything silently. A reader of auditd.state cannot tell one lost event from a million, and the flag is cleared again when a SIGHUP grows q_depth. Events rejected while a resize has queueing suspended do not set the flag at all. Add a counter of the events enqueue() discards because the queue is full or suspended: when the retries run out and when queueing is suspended. The not initialized path that returns -1 is not counted. It is not reset by a reconfigure that clears the overflow flag nor by destroy_queue(). It is an unsigned long, so on 64-bit systems it does not wrap even when every event is lost. Report it as "plugin events dropped", the last line of the plugin queue section, so the existing plugin queue lines keep their text and position for scripts that parse them, and mention it in the q_depth description. Add a queue test that checks the exact count for O_IGNORE and O_SUSPEND and that it survives a resize plus reset_suspended() and a destroy_queue()/init_queue() cycle, and that enqueue() on a destroyed queue does not count. Assisted-by: Claude Opus 5.5
dispatch_event() drops a record meant for the plugins when the event allocation fails, the netlink length does not fit or the protocol version is unknown, and libdisp_enqueue() returns -1 without counting it when the queue is not initialized. Count those. A record the queue rejects with 1 is already counted, and having no plugins is not a drop. The test now includes auditd-dispatch.c so it can fail the allocation. Assisted-by: Claude Opus 5.5
A plugin that fails at runtime stays in the list marked inactive until a reconfigure, and every later record is skipped for it silently. Count a record once when it misses a plugin: inactive, failed write, or died and not delivered by a restart. Records that cannot be formatted count too, skips while the dispatcher is stopping do not. The dispatcher thread now updates the counter too, so the fallback without stdatomic uses __atomic builtins like libdisp_child_changed(). Growth no longer always means q_depth is too small, so auditd-plugins.5 describes the line instead. Assisted-by: Claude Opus 5.5
45467b7 to
ba30775
Compare
|
Pushed. It's three commits now: the rename in the first one, then dispatch_event() and the dispatcher loop. The counter stays in queue.c, since enqueue() and write_queue_state() use it and the queue test links only libqueue. Its prototype is hidden, so it doesn't get exported from libauplugin. It's an unsigned long now, changed in the first commit, because with a dead plugin it climbs at the full event rate and a 32-bit count would wrap within days on a busy 64-bit box. A record counts once even when it missed more than one plugin, same as a queue drop. Happy to make it per plugin if you'd rather. I also count two cases you didn't list. One is a write that fails with something other than EPIPE: the plugin stays active, but that record is gone. The other is the -1 from libdisp_enqueue() when the queue isn't initialized, for example after init_queue() failed, because the queue doesn't count that one itself. One gap is left. reconfigure() can return 0 when stop_plugins() fails, when no new plugin starts, or when a reload fails while every old plugin is inactive. The dispatcher thread then exits and libdisp_active() drops to 0, so later records look like "no plugins configured" and aren't counted. Closing that needs libdisp to tell "gave up" apart from "none configured". Should I do that here or in a separate PR? |
As discussed in #562, this adds a count of the events the plugin queue drops, so auditd.state shows how many were lost and not just that an overflow happened.
enqueue() bumps the counter when it gives up after the retries and when queueing is suspended. A reconfigure that clears the overflow flag doesn't reset it, and neither does destroy_queue(). It shows up as a new last line of the plugin queue section, "plugin queue dropped events = N", so the existing lines don't move. I also mentioned it in the q_depth description in auditd.conf.5.
You may see the count go up while "overflow detected" still says no. A SIGHUP resize suspends the queue briefly, and events that arrive then are dropped without setting the flag. Right now nothing records those at all.
The new test in test-queue.c overflows a 4 slot queue by 10 events with O_IGNORE and O_SUSPEND, checks the exact count, and checks that it survives a resize and a destroy_queue()/init_queue() cycle.
auplugin uses the same queue but only exposes the overflow flag. I left it out to keep this small, happy to add it in a follow-up if you want.