tetragon: Hook exit sensor on acct_process

[olsajiri]

Djalal and Anastasios found another way we could race in exit
event hook, so we could receive multiple exit events with same
pid value.

Anastasios suggested to hook acct_process instead, which is
called only for the last task in the thread group.

The acct_process depends on CONFIG_BSD_PROCESS_ACCT config
option but it seems to be present on all supported kernels.

[jrfastab]

would also be nice to get a test for this failing case if possible.

[tixxdz]

Hi @olsajiri being testing this on various kernels seems to work, here is a simple test that works at bpf side we don't mix with userspace , maybe add it too 05b2a94 up to you

[tixxdz]

Lgtm Jiri thanks, can you please add later as a follow same test but that runs on the perf ring buffer.

In case we change some ref count logic that may not export the grpc exit event anymore, then we are still covered at bpf side.

[tixxdz]

@olsajiri just for the record:

If we don't want to depend on that config, then an alternative could be:

+++ b/bpf/process/bpf_exit.c
@@ -46,6 +46,8 @@ char _license[] __attribute__((section("license"), used)) = "GPL";
  * won't race with another clone, because there's no other thread to call
  * it (current thread is in do_exit).
  */
+#define SIGNAL_GROUP_EXIT      0x00000004
+
 __attribute__((section("kprobe/do_task_dead"), used)) int
 event_exit(struct pt_regs *ctx)
 {
@@ -53,11 +55,30 @@ event_exit(struct pt_regs *ctx)
        __u64 pid_tgid = get_current_pid_tgid();
        struct signal_struct *signal;
        atomic_t live;
+       __u32 flags, tgid;
 
        probe_read(&signal, sizeof(signal), _(&task->signal));
        probe_read(&live, sizeof(live), _(&signal->live));
+       probe_read(&flags, sizeof(flags), _(&signal->flags));
+
+       tgid = pid_tgid >> 32;
+       /* If it is a group thread exit then check SIGNAL_GROUP_EXIT
+        * as it is set early to sync all threads where signal is locked.
+        */
+       if (flags & SIGNAL_GROUP_EXIT) {
+               if (tgid == (__u32)pid_tgid) {
+                       DEBUG("sending bpf exit:  pid=%d   tid=%d", pid_tgid >> 32, (u32)pid_tgid);
+                       event_exit_send(ctx, pid_tgid >> 32);
+               }
+               return 0;
+       }
 
-       if (live.counter == 0)
-               event_exit_send(ctx, pid_tgid >> 32);
+       /* If single thread exit then send exit on counter zero, still separated threads
+        * can race and have the live.counter zero.
+        */
+       if (live.counter == 0) {
+               DEBUG("sending bpf exit:  pid=%d   tid=%d", pid_tgid >> 32, (u32)pid_tgid);
+               event_exit_send(ctx, tgid);
+       }
        return 0;
 }

This will close the current group exit issue, but we still have a smaller window where separate threads exit on their own and could race.

For that case the sensor handler or grpc one where the event parsing happens, plus the gc need to be bullet proof to ensure we collect process entries when we receive one single exit event (as live.counter == 0) is good indication anyway, and ignore following bpf exit event if ever they happen.

However I identified some other issues on the gc logic: #1517 where we underflow the refcount and won't collect entries... I will try to fix that separately anyway as this is another bug.

Maybe then if we feel we are fine we could have that code and switch off from CONFIG_BSD_PROCESS_ACCT.

For the time being I think current solution is ok