Invalid Opcode when scheduling multiple threads

[shizi]

I'm developing my x86_64 kernel and encountering an Invalid Opcode exception when scheduling multiple threads.
When I don't use the test module, only the idle thread runs and everything works fine—interrupts return correctly. But when I enable multi‑threading tests, an Invalid Opcode exception occurs. At the time of the exception, I see that the `rip` register points to a linear‑mapped address (physical address + offset), which is an area I use for kernel data.

Here is a partial log when running on a single core:

Code: Select all

[TASK]task data init success
[SMP] smp init succeed
[TASK]task init success
[TASK_TEST]Thread A started
[TASK_TEST]Thread B started
[TASK_TEST]Thread C started
[TASK_TEST]Thread D started
[TASK_TEST]Thread E started
[TASK_TEST]Thread F started
Invalid Opcode
rdi=0x0xFFFF80807F003000
rsi=0x0xFFFF80807E064000
rdx=0x0x0000000000000000
rcx=0x0x0000000000000837
rax=0x0x0000000000000028
r8=0x0x0000000000000282
r9=0x0xFFFF80807F007FAF
r10=0x0xFFFF80807F007FAF
r11=0x0xCCCCCCCCCCCCCCCD
rbx=0x0x0000000000000028
rbp=0x0xCCCCCCCCCCCCCCCD
r12=0x0xFFFF80807E063F20
r13=0x0xFFFF80807E07E000
r14=0x0x0000000000000041
r15=0x0x0000000000000001
vector=6
error_code=0x0x0000000000000000
rip=0x0xFFFF80807E063FFC
cs=0x0x0000000000000008
rflags=0x0x0000000000000046
rsp=0x0xFFFF80807E063F18
ss=0x0x0000000000000010
Invalid Opcode

The test code:

Code: Select all

static void test_thread(void *arg) {
    char id = (char)(uintptr_t)arg;
    uint64_t cnt = 0;

    TASK_TEST_PRINT("Thread %c started\n", id);

    while (1) {
        cnt++;
        if (cnt % 5 == 0) {
            TASK_TEST_PRINT("[%c] count=%llu\n", id, cnt);
        }
        cpu_halt();
    }
}

static void test(void) {
    if (get_logical_id() == bootboot->bspid) {
        task_create_kernel_thread(test_thread, (void*)'A');
        task_create_kernel_thread(test_thread, (void*)'B');
        task_create_kernel_thread(test_thread, (void*)'C');
        task_create_kernel_thread(test_thread, (void*)'D');
        task_create_kernel_thread(test_thread, (void*)'E');
        task_create_kernel_thread(test_thread, (void*)'F');
    }
}

From my debugging, the exception always occurs near the interrupt return path, and each test kernel thread runs normally the first time it is scheduled. I compiled with `-fstack-protector-all`, but no stack overflow was detected. It's strange: if every task can run once and the idle thread works fine, the interrupt handling and `switch_to` seem correct, yet the error still happens.

Below are the core parts of my interrupt handling and `switch_to`:

Common interrupt handler:

Code: Select all

.section .text
.globl common_interrupt
.type common_interrupt, @function
.align 8
common_interrupt:
    pushq %r15
    pushq %r14
    pushq %r13
    pushq %r12
    pushq %rbp
    pushq %rbx
    pushq %r11
    pushq %r10
    pushq %r9
    pushq %r8
    pushq %rax
    pushq %rcx
    pushq %rdx
    pushq %rsi
    pushq %rdi

    testb $3, 144(%rsp)
    jz 1f
    swapgs
1:
    movq %rsp, %r12
    andq $-16, %rsp
    movq %r12, %rdi
    call irq_entry
    movq %r12, %rsp

    testb $3, 144(%rsp)
    jz 2f
    swapgs
2:
    popq %rdi
    popq %rsi
    popq %rdx
    popq %rcx
    popq %rax
    popq %r8
    popq %r9
    popq %r10
    popq %r11
    popq %rbx
    popq %rbp
    popq %r12
    popq %r13
    popq %r14
    popq %r15

    addq $16, %rsp
    iretq

Before jumping here, I already unify the stack layout as:
[rsp] error code (or 0)
[rsp+8] vector number
[rsp+16] hardware‑pushed RIP
[rsp+24] hardware‑pushed CS
[rsp+32] hardware‑pushed RFLAGS
[rsp+40] hardware‑pushed RSP (if from user mode)
[rsp+48] hardware‑pushed SS (if from user mode)

C interrupt entry:

Code: Select all

void irq_entry(struct pt_regs *regs) {
    // Get current timestamp
    uint64_t now = smp_get_timestamp();
    
    /**
     * Update time
     * The updated period is from the last kernel exit to this kernel entry,
     * i.e., the time spent in user mode.
     */
    time_update(now);

    // Account user mode time to the current task
    uint64_t user_delta = time_delta();
    task_add_current_tick(user_delta);   

    uint64_t vector = regs->vector;
    bool is_vector = (vector < 32);

    if (irq_table[vector]) {
        void (*irq)(struct pt_regs *regs) = (void (*)(struct pt_regs *regs))irq_table[vector];
        irq(regs);
    } else {
        if (is_vector) {
            printp("CPU ERROR\n");
        } else {
            IRQ_WARN("NO HANDLER FOR VECTOR\n");
        }
    }

    // Update timestamp again
    now = smp_get_timestamp();
    time_update(now);

    if (is_vector) {
        // Exception finished, accumulate kernel time for the current task
        uint64_t kernel_delta = time_delta();
        task_add_current_tick(kernel_delta);
    } else {
        // External interrupt finished, only update timestamp, no accumulation
        apic_eoi();
    }

    if (smp_check_need_sched()) task_sched();
}

`switch_to` code:

Code: Select all

__attribute__((naked, noinline))
void switch_to(struct thread_struct *prev, struct thread_struct *next) {
    __asm__ volatile (
        // Save prev registers to prev->thread
        "movq %%rbx,  %c[thr_rbx](%%rdi)\n\t"
        "movq %%rbp,  %c[thr_rbp](%%rdi)\n\t"
        "movq %%r12,  %c[thr_r12](%%rdi)\n\t"
        "movq %%r13,  %c[thr_r13](%%rdi)\n\t"
        "movq %%r14,  %c[thr_r14](%%rdi)\n\t"
        "movq %%r15,  %c[thr_r15](%%rdi)\n\t"
        "movq %%rsp,  %c[thr_rsp](%%rdi)\n\t"

        // Save return address
        "movq (%%rsp), %%rax\n\t"
        "movq %%rax,  %c[thr_rip](%%rdi)\n\t"

        // Save fs_base
        "rdfsbase %%rax\n\t"
        "movq %%rax,  %c[thr_fs](%%rdi)\n\t"

        // Save cr3
        "movq %%cr3, %%rax\n\t"
        "movq %%rax,  %c[thr_cr3](%%rdi)\n\t"

        // Load next registers
        "movq %c[thr_rsp](%%rsi), %%rsp\n\t"
        "movq %c[thr_rbx](%%rsi), %%rbx\n\t"
        "movq %c[thr_rbp](%%rsi), %%rbp\n\t"
        "movq %c[thr_r12](%%rsi), %%r12\n\t"
        "movq %c[thr_r13](%%rsi), %%r13\n\t"
        "movq %c[thr_r14](%%rsi), %%r14\n\t"
        "movq %c[thr_r15](%%rsi), %%r15\n\t"

        // Restore fs_base
        "movq %c[thr_fs](%%rsi), %%rax\n\t"
        "wrfsbase %%rax\n\t"

        // Load page table
        "movq %c[thr_cr3](%%rsi), %%rax\n\t"
        "movq %%rax, %%cr3\n\t"

        // Jump to next->rip
        "pushq %c[thr_rip](%%rsi)\n\t"
        "ret\n"
        :
        : [thr_rip]  "i" (THR_RIP),
          [thr_cr3]  "i" (THR_CR3),
          [thr_rsp]  "i" (THR_RSP),
          [thr_fs]   "i" (THR_FS),
          [thr_rbx]  "i" (THR_RBX),
          [thr_rbp]  "i" (THR_RBP),
          [thr_r12]  "i" (THR_R12),
          [thr_r13]  "i" (THR_R13),
          [thr_r14]  "i" (THR_R14),
          [thr_r15]  "i" (THR_R15)
        : "rax", "memory"
    );
}

The macros like `THR_RIP` are simply `offsetof(struct thread_struct, rip)`.

The exception seems to occur after `task_sched` returns

Code: Select all

void task_sched(void) {
    uint64_t flags = get_cpu_flags();
    irq_off();

    struct task_struct *prev = smp_get_task_current();
    if (!prev) {
        goto out;
    }

    if (prev->state == TASK_RUNNING && prev != smp_get_idle()) {
        sched_class_ptr->enqueue(prev);
    }

    struct task_struct *next = sched_class_ptr->pick_next();
    if (!next) {
        goto out;
    }

    // If the same task, no need to switch; just set the timer and return
    if (prev == next) {
        prev->sched.exec_ns = 0;
        task_set_next_timer();
        goto out;
    }

    fpu_save(prev->thread);
    smp_set_task_current(next);
    fpu_restore(next->thread);

    // Set the next timer interrupt
    task_set_next_timer();

    switch_to(prev->thread, next->thread);

out:
    write_cpu_flags(flags);
}

Everything looks correct, yet `rip` becomes that data‑area address. I'm puzzled.

Here is my GitHub repository: https://github.com/shizi297/ShiziOS

[Octocontrabass]

[rsp+40] hardware‑pushed RSP (if from user mode)
[rsp+48] hardware‑pushed SS (if from user mode)

The CPU always pushes SS:RSP to the stack.

`switch_to` code:

Why aren't you switching stacks when you switch tasks? Also, extended asm is not allowed in naked functions.

[shizi]

The CPU always pushes SS:RSP to the stack.

Okay, thanks for pointing that out, I'll keep it in mind.

Why aren't you switching stacks when you switch tasks? Also, extended asm is not allowed in naked functions.

I do switch the stack with `movq %c[thr_rsp](%%rsi), %%rsp\n\t`. About naked functions, I looked it up and it might indeed be problematic. I disassembled the code and confirmed the generated machine code is correct and no extra binary was produced. However, to be safe, I also tried replacing `switch_to` with a basic inline assembly using hardcoded offsets, like this:

Code: Select all

__attribute__((naked, noinline))
void switch_to(struct thread_struct *prev, struct thread_struct *next) {
    asm volatile (
        // Save prev registers into prev->thread
        "movq %%rbx, 0x20(%%rdi)\n\t"
        "movq %%rbp, 0x28(%%rdi)\n\t"
        "movq %%r12, 0x30(%%rdi)\n\t"
        "movq %%r13, 0x38(%%rdi)\n\t"
        "movq %%r14, 0x40(%%rdi)\n\t"
        "movq %%r15, 0x48(%%rdi)\n\t"
        "movq %%rsp, 0x10(%%rdi)\n\t"

        // Save return address
        "movq (%%rsp), %%rax\n\t"
        "movq %%rax, 0x00(%%rdi)\n\t"

        // Save fs_base
        "rdfsbase %%rax\n\t"
        "movq %%rax, 0x18(%%rdi)\n\t"

        // Save cr3
        "movq %%cr3, %%rax\n\t"
        "movq %%rax, 0x08(%%rdi)\n\t"

        // Load next registers
        "movq 0x10(%%rsi), %%rsp\n\t"
        "movq 0x20(%%rsi), %%rbx\n\t"
        "movq 0x28(%%rsi), %%rbp\n\t"
        "movq 0x30(%%rsi), %%r12\n\t"
        "movq 0x38(%%rsi), %%r13\n\t"
        "movq 0x40(%%rsi), %%r14\n\t"
        "movq 0x48(%%rsi), %%r15\n\t"

        // Restore fs_base
        "movq 0x18(%%rsi), %%rax\n\t"
        "wrfsbase %%rax\n\t"

        // Load page table
        "movq 0x08(%%rsi), %%rax\n\t"
        "movq %%rax, %%cr3\n\t"

        // Jump to next->rip
        "pushq 0x00(%%rsi)\n\t"
        "ret\n\t"
        : : : "rax", "memory"
    );
}

The result still shows the same issue. So the problem probably isn’t in this function.

[Octocontrabass]

I do switch the stack with `movq %c[thr_rsp](%%rsi), %%rsp\n\t`.

Why aren't you saving the GPRs on the stack? You're never going to examine or modify them. And since you are switching the stack, you don't need to save the return address because the return address is already on the stack.

Code: Select all

        : : : "rax", "memory"

This is still extended asm.

The result still shows the same issue. So the problem probably isn’t in this function.

The problem probably is that you're pushing the return address onto the stack when the return address is already on the stack.

[shizi]

Why aren't you saving the GPRs on the stack? You're never going to examine or modify them. And since you are switching the stack, you don't need to save the return address because the return address is already on the stack.

We use switch_to to jump directly to a kernel thread (on its first run) or to the interrupt return path of a kernel thread (when switch_to is called, the return address pushed by hardware is used). For the first run of a kernel thread, we don't need to save anything, because it doesn't go through the interrupt return path; the first time it gets interrupted, our interrupt handler will save the context. Our plan is to only use the interrupt return path for the first run of user threads.

The problem probably is that you're pushing the return address onto the stack when the return address is already on the stack.

Thank you so much! I didn't realize I had an extra return address. By directly overwriting the pushed return address instead of pushing another one, I solved the problem. However, for now I still plan to use switch_to for the first scheduling of kernel threads to directly jump to the kernel thread entry, and only user threads will go through the interrupt return path on their first schedule.

[Octocontrabass]

We use switch_to to jump directly to a kernel thread (on its first run) or to the interrupt return path of a kernel thread (when switch_to is called, the return address pushed by hardware is used).

I don't understand. Don't you use switch_to for all task switches?

By directly overwriting the pushed return address

Why are you overwriting the return address? The correct return address should already be on the stack.

However, for now I still plan to use switch_to for the first scheduling of kernel threads to directly jump to the kernel thread entry, and only user threads will go through the interrupt return path on their first schedule.

What does the interrupt return path have to do with anything? Task switching doesn't care about interrupts, it just switches to a new stack and the new task resumes exactly where it last left off.

[shizi]

I don't understand. Don't you use switch_to for all task switches?

All task switches are indeed done through switch_to, no exceptions.

Why are you overwriting the return address? The correct return address should already be on the stack.

When a kernel thread runs for the first time, its stack has no pt_regs, so it jumps directly to ret_from_kernel_thread. Later, when it gets interrupted, the hardware will push pt_regs and the interrupt return path will use them correctly. The change I made to overwrite the return address in switch_to was only to eliminate the extra return address pushed by the call instruction; it does not affect the normal switching logic.

What does the interrupt return path have to do with anything? Task switching doesn't care about interrupts, it just switches to a new stack and the new task resumes exactly where it last left off.

Distinguishing between kernel threads and user threads means that in the future, the first run of a user thread may need to construct a pt_regs and return via iretq, while kernel threads will still use direct jumps.

[Octocontrabass]

The change I made to overwrite the return address in switch_to was only to eliminate the extra return address pushed by the call instruction; it does not affect the normal switching logic.

There is no return address pushed by the call instruction the first time a thread runs. When you allocate the new stack for the new thread, you put a return address on it, and then switch_to doesn't need to overwrite anything because the return address is already correct. (If switch_to expects anything else on the stack, you fill in those values too.)

[shizi]

There is no return address pushed by the call instruction the first time a thread runs. When you allocate the new stack for the new thread, you put a return address on it, and then switch_to doesn't need to overwrite anything because the return address is already correct. (If switch_to expects anything else on the stack, you fill in those values too.)

When creating a kernel thread, we allocate and set up a `thread_struct` on the heap (not on the stack). Its `thread.rip` is set to `ret_from_kernel_thread`, `thread.rbx` holds the kernel thread entry function, and `thread.rbp` holds the argument. When `switch_to` is called, the `call` instruction automatically pushes a return address onto the current stack. For the first run of a kernel thread, its `thread.rip` is not the return address pushed by that `call`. Therefore we need to load `thread.rip` and overwrite the pushed return address (while saving the old one). `switch_to` does not distinguish between first and subsequent runs; it always performs this overwrite.

[Octocontrabass]

When `switch_to` is called, the `call` instruction automatically pushes a return address onto the current stack.

But switch_to will switch to a different stack with a different return address. Why doesn't the new thread's stack already contain the correct return address?

[shizi]

But switch_to will switch to a different stack with a different return address. Why doesn't the new thread's stack already contain the correct return address?

Oh! I didn't realize that the return address pushed by the call switch_to doesn't exist on the new stack,it's on the old stack. Thank you for keeping asking me about this, it helped me understand the issue. I plan to remove the rip field from the thread_struct, and change the kernel thread creation code to place a return address on its stack in advance, so that I no longer need to load or save rip.