Move TDB_USERWR check under 'if (traced)'

[trasz]

Summary

Move TDB_USERWR check under 'if (traced)'.

If we hadn't been traced in the first place when syscallenter() started executing, we can ignore TDB_USERWR. TDB_USERWR can get set, sure, but if it does, it's because the debugger raced with the syscall, and it cannot depend on winning that race.

[trasz]

No need to __predict_false() there.

[kib]

If we hadn't been traced in the first place when syscallenter() started executing, then the TDP_USERWR, cleared few lines above, should still be clear.

This is in fact false.

Did you tested that changing syscall args still works ? I do not remember if I put a test into the tree.

[trasz]

In D26585#592163, @kib wrote:

If we hadn't been traced in the first place when syscallenter() started executing, then the TDP_USERWR, cleared few lines above, should still be clear.

This is in fact false.

Can you tell me the scenario where this is not true?

Did you tested that changing syscall args still works ? I do not remember if I put a test into the tree.

Not yet; I've run the kyua ptrace tests, but now that I look at them, I don't see anything that would change the syscall arguments. Can you tell me how to test it?

[kib]

In D26585#592543, @trasz wrote:

In D26585#592163, @kib wrote:

If we hadn't been traced in the first place when syscallenter() started executing, then the TDP_USERWR, cleared few lines above, should still be clear.

This is in fact false.

Can you tell me the scenario where this is not true?

P_TRACED is protected by the process lock. As far as lock not held (or dropped), parallel attach or detach can occur.

Did you tested that changing syscall args still works ? I do not remember if I put a test into the tree.

Not yet; I've run the kyua ptrace tests, but now that I look at them, I don't see anything that would change the syscall arguments. Can you tell me how to test it?

Track SCE (syscall enter) with ptrace. When stopped due to enter, modify args and after continue, observe immediate SCE (without SCX) with updated args/syscall number.
I believe I had some test for this, but if it survived to the present time, it should be on a w/s that is currently not powered on. I will look later.
Meantime, tools/test/ptrace/scescx.c provides idiomatic example of tracing syscalls (both SCE and SCX) which should be much easier to follow than truss(1) code.

[trasz]

In D26585#592596, @kib wrote:

In D26585#592543, @trasz wrote:

In D26585#592163, @kib wrote:

If we hadn't been traced in the first place when syscallenter() started executing, then the TDP_USERWR, cleared few lines above, should still be clear.

This is in fact false.

Can you tell me the scenario where this is not true?

P_TRACED is protected by the process lock. As far as lock not held (or dropped), parallel attach or detach can occur.

True, but that's not quite what I meant.

First, P_TRACED can be cleared, but we're checking the 'traced' variable, not P_TRACED. If P_TRACED was set when it was checked at the beginning of the function and cleared just a moment later, 'traced' will still be true and thus we'll check TDB_USERWR. If I understand it right, this should fix the case of parallel detach.

Second, with the patch the parallel attach and setting the TDB_USRWR would indeed get ignored, but - again, if my mental model is correct - the debugger cannot depend on it _not_ being ignored, because it's racing with us anyway.

Did you tested that changing syscall args still works ? I do not remember if I put a test into the tree.

Not yet; I've run the kyua ptrace tests, but now that I look at them, I don't see anything that would change the syscall arguments. Can you tell me how to test it?

Track SCE (syscall enter) with ptrace. When stopped due to enter, modify args and after continue, observe immediate SCE (without SCX) with updated args/syscall number.
I believe I had some test for this, but if it survived to the present time, it should be on a w/s that is currently not powered on. I will look later.
Meantime, tools/test/ptrace/scescx.c provides idiomatic example of tracing syscalls (both SCE and SCX) which should be much easier to follow than truss(1) code.

Thanks; I'll take a look.

[kib]

In D26585#592648, @trasz wrote:

In D26585#592596, @kib wrote:

In D26585#592543, @trasz wrote:

In D26585#592163, @kib wrote:

If we hadn't been traced in the first place when syscallenter() started executing, then the TDP_USERWR, cleared few lines above, should still be clear.

This is in fact false.

Can you tell me the scenario where this is not true?

P_TRACED is protected by the process lock. As far as lock not held (or dropped), parallel attach or detach can occur.

True, but that's not quite what I meant.

First, P_TRACED can be cleared, but we're checking the 'traced' variable, not P_TRACED. If P_TRACED was set when it was checked at the beginning of the function and cleared just a moment later, 'traced' will still be true and thus we'll check TDB_USERWR. If I understand it right, this should fix the case of parallel detach.

Second, with the patch the parallel attach and setting the TDB_USRWR would indeed get ignored, but - again, if my mental model is correct - the debugger cannot depend on it _not_ being ignored, because it's racing with us anyway.

Re-read the statement at the very beginning of the quoted text.

That said, I do not see the race as problematic, it is just there. What make me more worried is your statement (at the beginning) about the modified code that is not true.

[trasz]

I gave myself a break from this diff for a while, and upon revisiting it - sure, you're right. Let me restate what I meant:

The fundamental assumption in this patch is that if P_TRACED was not set when sysenter() started executing, we can ignore TDB_USERWR. TDB_USERWR can get set, sure, but if it does, it's because the debugger raced with the syscall, and it cannot depend on winning that race.

Does that sound correct?

[kib]

At least I expect an update for the review summary/commit message.

[trasz]

Sure, done.

[trasz]

Regen.