ED: Fix potential task_struct double-frees when iterating over tasks

[kerneltoast]

No description provided.

[Adam-pi3]

Thanks @kerneltoast for that PR. I looked at the code and I think that we can drop get/put_task_struct because we are under RCU lock. p_iterate_process() is only executed when we do initialization, and we do it when all the processes are frozen (we call freeze_processes()).
With seccomp is different story. That being said, we know that task_struct can't be 0 while we are intercepting the seccomp() syscall, but the other threads' task struct when SECCOMP_FILTER_FLAG_TSYNC is used probably (unlikely?) can.

I'm wondering, maybe it is worth adding read_{un}lock(&tasklist_lock) for p_iterate_processes (and paranoid)? What do you @kerneltoast and @solardiz think?

Thanks,
Adam

[kerneltoast]

I looked at the code and I think that we can drop get/put_task_struct because we are under RCU lock. p_iterate_process() is only executed when we do initialization, and we do it when all the processes are frozen (we call freeze_processes()).

Kthreads aren't frozen though, and all tasks in the system are iterated upon, so there is still a double-free possibility from p_iterate_processes()... or so I thought, but there's no double-free at all (see below).

With seccomp is different story. That being said, we know that task_struct can't be 0 while we are intercepting the seccomp() syscall,

This is only true for current, not any other task including threads part of the same thread group as current... or so I thought again.

I'm wondering, maybe it is worth adding read_{un}lock(&tasklist_lock) for p_iterate_processes (and paranoid)? What do you @kerneltoast and @solardiz think?

Turns out I'm wrong about the potential double-free because a task_struct pointer which is obtained under RCU read lock is guaranteed to have a refcount of >=1 within that same RCU read-side critical section. This guarantee was introduced by this old commit from 2006.

So the commit message can be simplified to say that it's just removing superfluous {get|put}_task_struct() calls.

[solardiz]

A bikeshed: regarding the commit message, we no longer use the CI/ED designation since LKRG 0.8 (released years ago). This change was described as follows:

*) Redesign LKRG's presentation of its feature set to the user (sysadmin), no
   longer presenting it as having separate Code Integrity and Exploit Detection
   components, but instead LKRG as a whole working to detect various integrity
   violations (not only of code, and possibly caused by exploits) and attacks

We've more recently started to reuse the CI acronym to mean Continuous Integration.

We still have plenty of _ed_ in source code identifiers, but I think that's just legacy and we should eventually get rid of them in a bigger source code cleanup (which may also include source tree re-structuring to avoid the unnecessarily deep directories, and switching to Linux kernel coding style).