GPF on interrupt (stub calls another entry after handler)

[cart]

Hello everyone,
I was trying not to ask for help since breaking interrupts is a quite common issue, but I'm not having much success in tracking this bug. I apologize if this is a very basic issue, but maybe someone more experienced can give some tips.
Don't mind the code, I messed up the abstraction and have been hacking it together for testing purposes for a while. I still need to clean it up.

In https://github.com/cartTest/TestOS/blob ... l/boot.cpp, I initialize gdt, idt, and pic. Then I set irq0 in index 32 to be handled and generate the respective interrupt.

At first I was trying to make the handlers with inline asm, but since I was getting similar issues (interrupt only getting called once), I decided to try the stub implementation that appears in some tutorials. This can be found in https://github.com/cartTest/TestOS/blob ... nel/irqs.S, and the handler implementation can be found in https://github.com/cartTest/TestOS/blob ... elpers.cpp.

The interrupt setup can be found in https://github.com/cartTest/TestOS/blob ... rrupts.cpp. It basically creates and loads the idt, and sets all function to point to TEST. Newer additions will overwrite this pointer to different functions.

When running this code, the handler will get called, but when the iret instruction gets executed, the code will end up in TEST (default idt handler). Either that, or the stub forces kernel_init to be called again, leading to errors.
My guess is that either the stub assembly code is wrong, or the idt has an issue.

Qemu monitor outputs the following:

Code: Select all

check_exception old: 0xffffffff new 0xd
     1: v=0d e=6fb8 i=0 cpl=0 IP=0008:00100094 pc=00100094 SP=0018:00106fb8 env->regs[R_EAX]=00000020
EAX=00000020 EBX=00106fb8 ECX=00000020 EDX=00000020
ESI=00000000 EDI=00000000 EBP=00107018 ESP=00106fb8
EIP=00100094 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0010 00000000 ffffffff 00cf9300 DPL=0 DS   [-WA]
CS =0008 00000000 ffffffff 00cf9a00 DPL=0 CS32 [-R-]
SS =0018 00000000 ffffffff 00cf9300 DPL=0 DS   [-WA]
DS =0010 00000000 ffffffff 00cf9300 DPL=0 DS   [-WA]
FS =0010 00000000 ffffffff 00cf9300 DPL=0 DS   [-WA]
GS =0010 00000000 ffffffff 00cf9300 DPL=0 DS   [-WA]
LDT=0000 00000000 0000ffff 00008200 DPL=0 LDT
TR =0000 00000000 0000ffff 00008b00 DPL=0 TSS32-busy
GDT=     00107040 0000003f
IDT=     0010708e 000007ff
CR0=00000011 CR2=00000000 CR3=00000000 CR4=00000000
DR0=00000000 DR1=00000000 DR2=00000000 DR3=00000000 
DR6=ffff0ff0 DR7=00000400
CCS=00000010 CCD=00106fa4 CCO=ADDL    
EFER=0000000000000000

Full github code can be found in: https://github.com/cartTest/TestOS

Thank you for the help.

[Octocontrabass]

Code: Select all

1: v=0d e=6fb8
EBX=00106fb8

You're not following the ABI correctly. The caller needs to clean up the parameter-passing space on the stack. As it is now, you're popping garbage into EBX, and the CPU throws a fault on the instruction that tries to put that garbage into a segment register.

If you don't like the default calling conventions, you can override it for individual functions or for the whole program. Remember that libgcc must also be built with any options that apply to the whole program. (Multilib can help sort out different libgcc versions.) The Linux kernel uses "-mregparm=3".

[cart]

I apologize beforehand for my ignorance, as my assembly is very weak.

If I understood you correctly, I am messing up the stack cleanup after calling my handler.
I assume this happens because I push more onto the stack than I pop, so I am currently placing the previous esp onto ebx instead.

With that in mind I tried a few things without success:
First one was to

Code: Select all

add esp, 4

right after my call, to bypass the argument.

The second was to stop using eax/ebx, and just push/pop the segment registers directly, while still bypassing esp:

Code: Select all

irq_common_stub:
  pusha

  push ds
  push es
  push fs
  push gs

  mov ax, 0x10
  mov ds, ax
  mov es, ax
  mov fs, ax
  mov gs, ax
  
  push esp
  call _irq_handler
  add esp, 0x4

  pop gs
  pop fs
  pop es
  pop ds

  popa
  add esp, 0x8
  iret

It also didn't work, but it wouldn't make sense anyway, as it would mess up the regs struct passed to my handler.
This actually led me to a question: shouldn't my regs_t in https://github.com/cartTest/TestOS/blob ... terrupts.h, have "ds" after the general purpose registers?

Regardless, I also tried a few variations of both attempts, but ebx seems to always end up with garbage.

It might be something very simple, but I am failing to grasp it. Am I misunderstanding your reply?

[Octocontrabass]

I assume this happens because I push more onto the stack than I pop, so I am currently placing the previous esp onto ebx instead.

It's not the previous ESP, it's garbage. The parameter-passing space on the stack can be modified by the called function, so you can never be sure what values are there after it returns.

With that in mind I tried a few things without success:

There can be other issues besides the one I pointed out. Now that you're no longer trying to put garbage from the parameter-passing space into your segment registers, the bad segment selectors must be coming from somewhere else. Perhaps an interrupt happened while the segment registers still held selectors that reference the bootloader's GDT?

This actually led me to a question: shouldn't my regs_t in https://github.com/cartTest/TestOS/blob ... terrupts.h, have "ds" after the general purpose registers?

The stack grows down. The last item you push will be at the lowest address.

[cart]

Now that you're no longer trying to put garbage from the parameter-passing space into your segment registers, the bad segment selectors must be coming from somewhere else.

With this reply, I assume that bypassing the argument was the correct thing to do, and the assembly should now be correct. That's great to know as that is the part I am least comfortable with.

In that case, I probably have some other issue(s) in my gdt/idt/pic implementation, so I'll look into those. If anyone has any idea of the bug, or even feedback for my code, I'm happy to learn.

If I end up finding the problem, I'll update the post.
Thank you for the help!

[Octocontrabass]

If anyone has any idea of the bug, or even feedback for my code, I'm happy to learn.

Stale segment selectors? If an interrupt happens after you load the GDTR but before you load the segment registers, you'll push the bootloader's segment selectors onto the stack. When you try to pop them back into the segment registers, you'll get a fault because your GDT doesn't have descriptors that will work for those selectors.

[cart]

Hello again,

I've spent the last month trying to find the issue with my code, but unfortunately I had no luck.
I currently have contradictory beliefs as I don't think the problem is with my code, but something in the setup. This might be wrong, but I say this because I found a small project online that I could compile and run, and that project continued working after I swapped my code in (gdt/idt/pic/pit). In reverse, my code continued to fail after I swapped their code in.
On the other side, I followed the setup from the meaty skeleton, so if there was a problem there, it would've been found by now.

As I was out of ideas, I just noticed your comment:

Stale segment selectors? If an interrupt happens after you load the GDTR but before you load the segment registers, you'll push the bootloader's segment selectors onto the stack. When you try to pop them back into the segment registers, you'll get a fault because your GDT doesn't have descriptors that will work for those selectors.

How can I check if that was the case, and if so, how should I fix that?

Thank you once again.

[Octocontrabass]

How can I check if that was the case, and if so, how should I fix that?

It's easy enough to check, just look for all the places where you put your kernel's segment selectors into segment registers. If the only place you do that is in your interrupt stub, then you're enabling interrupts while there are stale selectors in the segment registers.

You can fix it by adding code right after you load the GDTR to also load CS, DS, ES, SS, and probably FS and GS. (Eventually you might handle FS and GS differently.)

[cart]

You can fix it by adding code right after you load the GDTR to also load CS, DS, ES, SS, and probably FS and GS.

This worked, thank you so much for the help!

I should've noticed the reply and asked about it earlier, as I was about to quit after one month of no progress.
At least I got to fix a few minor issues, change stubs, and experiment with different iterations. Just need to clean it up and finally move forward.

For anyone having a similar issue, I will leave the fix below. Keep in mind that the code changed a bit and has a different stub and setup. This change fixes the main issue in the github version, for the test in boot.cpp. Keep in mind that in the github version, Pit was not using the stub yet so it requires more changes for that to work.

The fix is the following:

1 - Change stub code to bypass registers parameter on return, and push/pop segment registers correctly (posted previously).
2 - Change the gdt load to a function in assembly that load gdtr, and loads the registers:

Code: Select all

.global gdt_load
gdt_load:
    mov 4(%esp), %eax
    lgdt (%eax)
    mov $0x10, %ax
    mov %ax, %ds
    mov %ax, %es
    mov %ax, %fs
    mov %ax, %gs
    mov %ax, %ss
    ljmp $0x8, $1f
    1: ret

3 - Call this new function in my class:

Code: Select all

extern "C" void gdt_load(uintptr_t p);

void GDT::Setup()
{
    ... // setup descriptors
    gdt_load((uintptr_t)&m_gdtr);
}

In theory, the assembly function can be converted to inline assembly. However, and I don't know why, it wasn't working when called inside a member function. It worked fine in a normal free function, though.

Thank you so much once again. I wouldn't have found it without your help.

[Octocontrabass]

In theory, the assembly function can be converted to inline assembly. However, and I don't know why, it wasn't working when called inside a member function.

GCC's inline assembly is notoriously difficult to get right. I can think of a few problems you could've had here.

[cart]

GCC's inline assembly is notoriously difficult to get right. I can think of a few problems you could've had here.

Nevermind, I found the issue. I was re-checking it to post here for learning purposes, when I noticed the problem while converting the code into inline asm goto.
I mistakenly left the "ret" inside the label, which obviously shouldn't be there in inline asm.

Not that it matters as the asm worked, but for completeness and if anyone is interested, here it is:

Code: Select all

asm volatile("lgdt %0;\n\t"
    "mov $0x10, %%ax;\n\t"
    "mov %%ax, %%ds;\n\t"
    "mov %%ax, %%es;\n\t"
    "mov %%ax, %%fs;\n\t"
    "mov %%ax, %%gs;\n\t"
    "mov %%ax, %%ss;\n\t"
    "ljmp $0x8, $1f;\n\t"
    "1: ;"
    :
    : "m" (m_gdtr)
    : "memory");

Thank you once again for all the help!

[Octocontrabass]

Not that it matters as the asm worked, but for completeness and if anyone is interested, here it is:

You're not telling the compiler that you're clobbering the value in AX. That might blow up in your face in the future.

The label you're using for the far jump could in theory be not unique. I'm not sure if it would ever be a problem, but there's a way to generate unique labels for inline assembly.

[cart]

You're not telling the compiler that you're clobbering the value in AX.

Good point, AX should definitely be in the clobbered list.

The label you're using for the far jump could in theory be not unique.

I may be mistaken/misusing but I thought numeric labels, with "f/b", were supposed to be local and unique.

[Octocontrabass]

I may be mistaken/misusing but I thought numeric labels, with "f/b", were supposed to be local and unique.

If the compiler uses the same number for a label that's supposed to go across your inline assembly, there might be a conflict. I'm pretty sure GCC doesn't use that kind of label, which is why I said "in theory".

[cart]

If the compiler uses the same number for a label that's supposed to go across your inline assembly, there might be a conflict. I'm pretty sure GCC doesn't use that kind of label, which is why I said "in theory".

Fair point, thanks for pointing it out. It's probably safe, but that's good to know.