On Wednesday, July 31, 2024 02:18 EEST, Linus Torvalds [off-list ref] wrote:

On Tue, 30 Jul 2024 at 16:09, Jeff Xu [off-list ref] wrote:

quoted

quoted

+               task = get_proc_task(file_inode(file));
+               if (task) {
+                       ptrace_active = task->ptrace && task->mm == mm && task->parent == current;

Do we need to call "read_lock(&tasklist_lock);" ?
see comments in ptrace_check_attach() of  kernel/ptrace.c

Well, technically I guess the tasklist_lock should be taken.

Practically speaking, maybe just using READ_ONCE() for these fields
would really be sufficient.

Yes, it could "race" with the task exiting or just detaching, but the
logic would basically be "at one point we were tracing it", and since
this fundamentally a "one-point" situation (with the actual _accesses_
happening later anyway), logically that should be sufficient.

I mean - none of this is about "permissions" per se. We actually did
the proper *permission* check at open() time regardless of all this
code. This is more of a further tightening of the rules (ie it has
gone from "are we allowed to ptrace" to "are we actually actively
ptracing".

I suspect that the main difference between the two situations is
probably (a) one extra step required and (b) whatever extra system
call security things people might have which may disable an actual
ptrace() or whatever..

Either approach is fine with me.

Will leave v4 a few days longer in case others have a stronger
opinion or to gather & address more feedback.

If no one objects by then, I'll send v5 with READ_ONCE().