Clear TDB_USERWR in ast()

[trasz]

Summary

Rework r287870 to clear TDB_USERWR in ast instead of syscallenter(),
to move it out of the syscall fast path.

[kib]

1. You are replacing simple 'and' with a lot of work.
2. You do it absolutely wrong. For start, why do you schedule ast it PT_DETACH ?

PT_USERWR is a temporal flag, its operational scope is limited by syscall_enter() fragment from the first ptracestop() due to PTE_SCE, to the second one. It is done for each PTE_SCE, and it must be cleared before call to the first ptracestop/PTE_SCE in syscall.

Also note that ptrace(2) ops that modify target state do not know if they operate on the temporal scope where the PT_USERWR is handled, so they set PT_USERWR always.

[trasz]

In D27132#605694, @kib wrote:

1. You are replacing simple 'and' with a lot of work.

I'm replacing a memory fetch with some work in place where it doesn't matter.

2. You do it absolutely wrong. For start, why do you schedule ast it PT_DETACH ?

The point of this patch is to move clearing the flag off the fast path. It cannot be cleared at PT_DETACH because, quoting r287870, "the flag cannot be cleared at PT_DETACH time in case one of the threads in the process is currently stopped in syscallenter() and the debugger has modified the arguments for that pending system call before detaching." This - detaching a debugger - is a rather infrequent event, thus IMO it matches the purpose of AST quite well.

PT_USERWR is a temporal flag, its operational scope is limited by syscall_enter() fragment from the first ptracestop() due to PTE_SCE, to the second one. It is done for each PTE_SCE, and it must be cleared before call to the first ptracestop/PTE_SCE in syscall.

Also note that ptrace(2) ops that modify target state do not know if they operate on the temporal scope where the PT_USERWR is handled, so they set PT_USERWR always.

Why does it need to be cleared? From my understanding the only thing it does is refetching the syscall args.

[kib]

In D27132#605777, @trasz wrote:

In D27132#605694, @kib wrote:

1. You are replacing simple 'and' with a lot of work.

I'm replacing a memory fetch with some work in place where it doesn't matter.

2. You do it absolutely wrong. For start, why do you schedule ast it PT_DETACH ?

The point of this patch is to move clearing the flag off the fast path. It cannot be cleared at PT_DETACH because, quoting r287870, "the flag cannot be cleared at PT_DETACH time in case one of the threads in the process is currently stopped in syscallenter() and the debugger has modified the arguments for that pending system call before detaching." This - detaching a debugger - is a rather infrequent event, thus IMO it matches the purpose of AST quite well.

PT_USERWR is a temporal flag, its operational scope is limited by syscall_enter() fragment from the first ptracestop() due to PTE_SCE, to the second one. It is done for each PTE_SCE, and it must be cleared before call to the first ptracestop/PTE_SCE in syscall.

Also note that ptrace(2) ops that modify target state do not know if they operate on the temporal scope where the PT_USERWR is handled, so they set PT_USERWR always.

Why does it need to be cleared? From my understanding the only thing it does is refetching the syscall args.

Because we do not want to re-read syscall args if not needed. It has potentially undesirable side effects. For instance, in ktrace you will see two syscall enters for each syscall after debugger modified some syscall args.

[trasz]

Also clear TDB_USERWR if traced.

[trasz]

In D27132#605793, @kib wrote:

In D27132#605777, @trasz wrote:

In D27132#605694, @kib wrote:

1. You are replacing simple 'and' with a lot of work.

I'm replacing a memory fetch with some work in place where it doesn't matter.

2. You do it absolutely wrong. For start, why do you schedule ast it PT_DETACH ?

The point of this patch is to move clearing the flag off the fast path. It cannot be cleared at PT_DETACH because, quoting r287870, "the flag cannot be cleared at PT_DETACH time in case one of the threads in the process is currently stopped in syscallenter() and the debugger has modified the arguments for that pending system call before detaching." This - detaching a debugger - is a rather infrequent event, thus IMO it matches the purpose of AST quite well.

PT_USERWR is a temporal flag, its operational scope is limited by syscall_enter() fragment from the first ptracestop() due to PTE_SCE, to the second one. It is done for each PTE_SCE, and it must be cleared before call to the first ptracestop/PTE_SCE in syscall.

Also note that ptrace(2) ops that modify target state do not know if they operate on the temporal scope where the PT_USERWR is handled, so they set PT_USERWR always.

Why does it need to be cleared? From my understanding the only thing it does is refetching the syscall args.

Because we do not want to re-read syscall args if not needed. It has potentially undesirable side effects. For instance, in ktrace you will see two syscall enters for each syscall after debugger modified some syscall args.

Thanks, I see your point. How about the new patch? It still clears TDB_USERWR on entry if traced, and also does it in ast() to "clean up" after detaching.

[kib]

Now you allow some syscall to enter wit TDB_USERWR be set.

Please drop this review, it is not going to work.

[trasz]

In D27132#605928, @kib wrote:

Now you allow some syscall to enter wit TDB_USERWR be set.

If the process is being traced, upon entering TDB_USERWR will get cleared, just like it is now. If it's not being traced, the flag will be ignored - again, just like now. The only thing this patch changes is how the "deferred clearing" works, ie what happens when we detach. What am I missing here?

Please drop this review, it is not going to work.

Assuming I _am_ missing something, which is of course quite possible, how does TDB_USERWR being set hurt anything? The only thing this flag affects is syscall argument refetching. If duplicate ktrace record is really a problem, we can simply move the call to ktrsyscall() to after the TDB_USERWR check.

Also note that it's not just TDB_USERWR - TDB_FORK and TDB_EXEC handling needs to be put under 'if (debug)' too; there's a followup review for those.

[kib]

In D27132#605947, @trasz wrote:

In D27132#605928, @kib wrote:

Now you allow some syscall to enter wit TDB_USERWR be set.

If the process is being traced, upon entering TDB_USERWR will get cleared, just like it is now. If it's not being traced, the flag will be ignored - again, just like now. The only thing this patch changes is how the "deferred clearing" works, ie what happens when we detach. What am I missing here?

Upon entering *to syscall handler*. I.e. chunk for subr_syscall.c along makes some sense. Everything else does not.

Please drop this review, it is not going to work.

Assuming I _am_ missing something, which is of course quite possible, how does TDB_USERWR being set hurt anything? The only thing this flag affects is syscall argument refetching. If duplicate ktrace record is really a problem, we can simply move the call to ktrsyscall() to after the TDB_USERWR check.

Also note that it's not just TDB_USERWR - TDB_FORK and TDB_EXEC handling needs to be put under 'if (debug)' too; there's a followup review for those.

[trasz]

In D27132#606537, @kib wrote:

In D27132#605947, @trasz wrote:

In D27132#605928, @kib wrote:

Now you allow some syscall to enter wit TDB_USERWR be set.

If the process is being traced, upon entering TDB_USERWR will get cleared, just like it is now. If it's not being traced, the flag will be ignored - again, just like now. The only thing this patch changes is how the "deferred clearing" works, ie what happens when we detach. What am I missing here?

Upon entering *to syscall handler*. I.e. chunk for subr_syscall.c along makes some sense. Everything else does not.

Okay, so the chunk for subr_syscall.c basically undoes r287870, to move the TDB_USERWR check out of the fast path. The rest of the patch deals with the same problem as r287870, but in a different way.

The point is that we cannot clear TDB_USERWR when detaching, because if debugger changed the syscall arguments and then detached, we would fail to fetch the updated arguments; thus, we need to clear it someplace else. The patch below moves clearing it into ast(). What's wrong with that approach?

[kib]

In D27132#608095, @trasz wrote:

In D27132#606537, @kib wrote:

In D27132#605947, @trasz wrote:

In D27132#605928, @kib wrote:

Now you allow some syscall to enter wit TDB_USERWR be set.

If the process is being traced, upon entering TDB_USERWR will get cleared, just like it is now. If it's not being traced, the flag will be ignored - again, just like now. The only thing this patch changes is how the "deferred clearing" works, ie what happens when we detach. What am I missing here?

Upon entering *to syscall handler*. I.e. chunk for subr_syscall.c along makes some sense. Everything else does not.

Okay, so the chunk for subr_syscall.c basically undoes r287870, to move the TDB_USERWR check out of the fast path. The rest of the patch deals with the same problem as r287870, but in a different way.

The point is that we cannot clear TDB_USERWR when detaching, because if debugger changed the syscall arguments and then detached, we would fail to fetch the updated arguments; thus, we need to clear it someplace else. The patch below moves clearing it into ast(). What's wrong with that approach?

AST is asynchronous (A is for it). It will clear (corrupt) state of unrelated thread that could already get a different instance of debugger attached.

[trasz]

In D27132#608106, @kib wrote:

In D27132#608095, @trasz wrote:

In D27132#606537, @kib wrote:

In D27132#605947, @trasz wrote:

In D27132#605928, @kib wrote:

Now you allow some syscall to enter wit TDB_USERWR be set.

If the process is being traced, upon entering TDB_USERWR will get cleared, just like it is now. If it's not being traced, the flag will be ignored - again, just like now. The only thing this patch changes is how the "deferred clearing" works, ie what happens when we detach. What am I missing here?

Upon entering *to syscall handler*. I.e. chunk for subr_syscall.c along makes some sense. Everything else does not.

Okay, so the chunk for subr_syscall.c basically undoes r287870, to move the TDB_USERWR check out of the fast path. The rest of the patch deals with the same problem as r287870, but in a different way.

The point is that we cannot clear TDB_USERWR when detaching, because if debugger changed the syscall arguments and then detached, we would fail to fetch the updated arguments; thus, we need to clear it someplace else. The patch below moves clearing it into ast(). What's wrong with that approach?

AST is asynchronous (A is for it). It will clear (corrupt) state of unrelated thread that could already get a different instance of debugger attached.

For a given thread, ast() will only run when returning to userspace, which is when TDB_USERWR being set does no longer matter - in fact, we later (in subsequent syscall) explicitly clear it early in syscallenter(). Within the time window when TDB_USERWR does matter, ast() cannot run for that thread (in other words, for any thread, ast() cannot run in parallel with syscallenter()), right?

Still, to be on the safe side, how about replacing the code in ast() with something like this:

+       if ((td->td_dbgflags & TDB_USERWR) != 0) {
+               PROC_LOCK(p);
+               if ((p->p_flag & P_TRACED) == 0) {
+                       thread_lock(td);
+                       td->td_dbgflags &= ~TDB_USERWR;
+                       thread_unlock(td);
+               }
+               PROC_UNLOCK(p);
+       }

This would guarantee we only clear TDB_USERWR if not being traced anymore. Although I prefer the current version (if correct), not only because it's simpler, but also because it hints the reader that TDB_USERWR only matters within the syscall.

[kib]

Until you set ast flag for other threads, they can run arbitrary amount of code, including syscalls.

[trasz]

In D27132#610070, @kib wrote:

Until you set ast flag for other threads, they can run arbitrary amount of code, including syscalls.

I'm sorry, I still don't follow.

Yes, other threads can do whatever they want, but 1. "their" ast() will never run in the middle of their syscallenter(), and 2. ast() only clears TDB_USERWR for that thread, it doesn't touch others.