Remove superfluous {get|put}_task_struct() calls 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).

[Adam-pi3]

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.

Seems reasonable. Nevertheless, I'm curious if we should add read_{un}lock(&tasklist_lock) protection during p_iterate_process() when we build the database on initialization. @solardiz what do you think?

[kerneltoast]

Nevertheless, I'm curious if we should add read_{un}lock(&tasklist_lock) protection during p_iterate_process() when we build the database on initialization.

Could you elaborate on this? I'm not seeing any race in the database creation that would be fixed by read_{un}lock(&tasklist_lock) protection.

[Adam-pi3]

Could you elaborate on this? I'm not seeing any race in the database creation that would be fixed by read_{un}lock(&tasklist_lock) protection.

Sure. RCU does not prevent the tasklist from being modified (it guarantees to be consistent) and during tasklist traverse we can either "dump" the old or the new structure of tasklist. In theory, it could be possible that some process is being in the middle of being removed (or added) to the tasklist while LKRG will be in the middle of building the database (in that case we may see the old list with/without those processes). In case of exit, I think we will detect it (through task flag that it is dying process), while when new one is created we can miss it and do not track it. Acquiring tasklist_lock will prevent anyone from modifying it during the database build.

[kerneltoast]

Sure. RCU does not prevent the tasklist from being modified (it guarantees to be consistent) and during tasklist traverse we can either "dump" the old or the new structure of tasklist. In theory, it could be possible that some process is being in the middle of being removed (or added) to the tasklist while LKRG will be in the middle of building the database (in that case we may see the old list with/without those processes). In case of exit, I think we will detect it (through task flag that it is dying process), while when new one is created we can miss it and do not track it. Acquiring tasklist_lock will prevent anyone from modifying it during the database build.

If you're referring to p_create_database(), I don't see any tasklist traverse there. 🫤

If you're referring to p_iterate_processes() which does the initial tasklist traversal and registers tasks with the ED tracker, I don't see any issues there because p_iterate_processes() is called with user tasks frozen and the p_is_ed_task() check filters out any kthreads. So the only tasks that can be added/removed during that traversal are kthreads.

I think RCU protection is sufficient, and tasklist_lock isn't needed.

[solardiz]

Thank you Adam and Sultan for this additional review/discussion, and I'm sorry I didn't get around to looking into that.

I think this PR is ready to merge, correct?

[Adam-pi3]

@kerneltoast when I refer to database creation in that context I meant ED database through p_iterate_processes(). I believe that the problem which I described can still be there. Even when we freeze the tasks, it doesn't mean that process creation/exiting were not started.
@solardiz I think we should probably add to this PR tasklist_lock unless we are fine with the risk which I described.

[solardiz]

I think we should probably add to this PR tasklist_lock unless we are fine with the risk which I described.

If we do this, it'd need to be a separate commit anyhow, so not necessarily in this PR.

[kerneltoast]

@kerneltoast when I refer to database creation in that context I meant ED database through p_iterate_processes(). I believe that the problem which I described can still be there. Even when we freeze the tasks, it doesn't mean that process creation/exiting were not started.

@Adam-pi3 freezing is a cooperative process—it is not possible for a task to be preempted and then frozen while executing arbitrary code in the kernel. It's similarly not possible for a task to be frozen while it is sleeping somewhere inside the kernel in a non-freezable state; e.g., if a task is in the middle of msleep(100) then it cannot be frozen.

Frozen tasks are parked in the aptly-named __refrigerator() which must be entered explicitly via a call to try_to_freeze(). For user tasks, try_to_freeze() is called from the signal-handling code.

The process creation/exit code in the kernel doesn't call into the refrigerator anywhere, so there is no possibility for a task to be frozen midway through process creation/exit.

[Adam-pi3]

I think I found the answer. @kerneltoast thanks for more details, although the same problem which I described may appear during process freeze. However, the logic of the freeze_processes function does take this into account and does acquire tasklist_lock inside of try_to_freeze_tasks function during task list RCU traversing:
https://elixir.bootlin.com/linux/v6.13.5/source/kernel/power/process.c#L52

		read_lock(&tasklist_lock);
		for_each_process_thread(g, p) {
			if (p == current || !freeze_task(p))
				continue;

			todo++;
		}
		read_unlock(&tasklist_lock);

Because of that, the problem does not apply for freezing and since we are executing our logic after, we're safe (otherwise we would need to acquire tasklist_lock).

[Adam-pi3]

@solardiz That's true, although I don't think is needed (please take a look at my comment #365 (comment)). I just merged the PR.