amd64: Avoid copying td_frame from kernel procs

[markj]

Summary

When creating a new thread, we unconditionally copy td_frame from the
creating thread. For threads which never return to user mode, this is
unnecessary since td_frame just points to the base of the stack or a
random interrupt frame.

If KASAN is configured this copying can trigger false positives since
the td_frame region may contain poisoned stack regions. It was not
noticed before since thread0 used a dummy proc0_tf trapframe, and kernel
procs are generally created by thread0. Since commit
df8dd6025af88a99d34f549fa9591a9b8f9b75b1, though, we call
cpu_thread_alloc(&thread0), which sets thread0.td_frame. In particular,
I think proc0_tf serves no purpose now.

Work around the problem by not copying the frame unless the copying
thread came from user mode. While here, de-duplicate the copying and
remove redundant re(initialization) of td_frame.

Reported by: [e-mail]

[kib]

May be indeed remove amd64' proc0_tf now?
For instance, you might initialize thread0.td_frame with some signalling value, which could be also used to initialize other kernel threads td_frame. Hm, I think it is better than checking P_KPROC.

sys/amd64/amd64/vm_machdep.c

613

Should this line also become conditional?

[markj]

In D32057#723494, @kib wrote:

May be indeed remove amd64' proc0_tf now?

Yes, I think so.

For instance, you might initialize thread0.td_frame with some signalling value, which could be also used to initialize other kernel threads td_frame. Hm, I think it is better than checking P_KPROC.

Isn't it possible for some interrupt handlers to set td_frame, even for kernel threads? The default value is effectively useless for kernel threads, but the field is still used, I think.

[kib]

In D32057#723598, @markj wrote:

In D32057#723494, @kib wrote:

May be indeed remove amd64' proc0_tf now?

Yes, I think so.

For instance, you might initialize thread0.td_frame with some signalling value, which could be also used to initialize other kernel threads td_frame. Hm, I think it is better than checking P_KPROC.

Isn't it possible for some interrupt handlers to set td_frame, even for kernel threads? The default value is effectively useless for kernel threads, but the field is still used, I think.

I inspected all places where td_frame is assigned for amd64 and generic kernel code (ast), and I do not think it happens.

[jhb]

sys/amd64/amd64/vm_machdep.c

613

It could be, and it could be moved into copy_thread() I think.

[markj]

In D32057#723623, @kib wrote:

In D32057#723598, @markj wrote:

In D32057#723494, @kib wrote:

May be indeed remove amd64' proc0_tf now?

Yes, I think so.

For instance, you might initialize thread0.td_frame with some signalling value, which could be also used to initialize other kernel threads td_frame. Hm, I think it is better than checking P_KPROC.

Isn't it possible for some interrupt handlers to set td_frame, even for kernel threads? The default value is effectively useless for kernel threads, but the field is still used, I think.

I inspected all places where td_frame is assigned for amd64 and generic kernel code (ast), and I do not think it happens.

The suggested approach is still awkward: when creating proc1, i.e., init, the new thread of course needs a valid td_frame pointer, so there is a case where the new thread should not inherit the sentinel value. So we can avoid the P_KPROC check in copy_thread(), but some similar check is still required somewhere in cpu_fork(), I believe.

[markj]

• Remove proc0_tf. thread0's td_frame is NULL up until fpuinitstate() calls cpu_thread_alloc().
• Conditionally clear PSL_T, move the clear into copy_thread().

[kib]

In D32057#724696, @markj wrote:

The suggested approach is still awkward: when creating proc1, i.e., init, the new thread of course needs a valid td_frame pointer, so there is a case where the new thread should not inherit the sentinel value. So we can avoid the P_KPROC check in copy_thread(), but some similar check is still required somewhere in cpu_fork(), I believe.

We can do something special when creating pid 1, or rather, when setting up init for first return to usermode. But I agree that the scope of this review should be limited to proposed patch.