Skip to content

Conversation

@minipli-oss
Copy link
Contributor

This PR fixes the extable handling as in not triggering a panic or handling NULL pointer dereferences in an endless loop. E.g. when booting on a VM with too little memory we'll now fail gracefully instead of looping in exception handling:

minipli@ex:~/src/ktf (extables)$ qemu-system-x86_64 -enable-kvm -cpu host -cdrom boot.iso -serial stdio 
COM1: 3f8, COM2: 2f8
CPU: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
Frequency: 1900 MHz
Initialize Physical Memory Manager
Avail memory frames: (total size: 126 MB)
  4 KB: 2177
  2048 KB: 59
Initialize Per CPU structures
Initialize SLAB
Initializing ACPI support
ACPI: RSDP 0x00000000000F5B20 000014 (v00 BOCHS )
ACPI: RSDT 0x0000000007FE1905 000034 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
ACPI: FACP 0x0000000007FE17B9 000074 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
ACPI: DSDT 0x0000000007FE0040 001779 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
RAX=0x0000000000000000  R8=0xffffffff807e6240
RBX=0x0000000007fe0000  R9=0x0000000007fe0000
RCX=0x0000000000000000 R10=0x0000000000007fe1
RDX=0xffffffff87fe0000 R11=0x00000000000003fd
RSI=0x0000000000000005 R12=0x0000000000000000
RDI=0xffffffff807e6260 R13=0x0000000000000001
RBP=0xffffffff8015d89c R14=0x0000000000000000
RSP=0xffffffff809ffe68 R15=0x0000000000000000

RIP=0xffffffff8013d270

CURRENT:
CS=0x0018 DS=0x0000 SS=0x0000
ES=0x0000 FS=0x0000 GS=0x0048
EXCEPTION:
CS=0x0018 SS=0x0000

CR0=0x0000000080010011 CR2=0xffffffff87fe0004
CR3=0x00000000007fb000 CR4=0x0000000000000030
CR8=0x0000000000000000

RFLAGS=0x0000000000010246

STACK[ffffffff809ffe68]:
0x0000: ffffffff8013f88c 0000000000000000 000000028013d4d1 0000000007fe0000 
0x0020: ffffffff87fe0000 5343414600000040 0000000000050000 0000000000000004 
0x0040: ffffffff807e6200 0000000000000074 ffffffff87fe1929 0000000000000034 
0x0060: ffffffff8013ee78 0000000000000004 ffffffff87fe17b9 0000000000000004 
0x0080: ffffffff87fe1905 0000000000000004 ffffffff801420fb ffffffff809fff48 
0x00a0: 0000000400000000 ffffffffffffffff 0000000000000000 0000000000000001 
0x00c0: 0000000000000000 0000000000000001 0000000000000000 ffffffffffffffff 
0x00e0: 000000003b9aca00 0000000000000000 ffffffff801403fb 0000000000000000 
0x0100: 0000000000000000 0000000000000000 ffffffff80154501 0000000000000067 
0x0120: 0000000000000000 0000000000000000 000000000001d000 ffffffffffffffff 
0x0140: 000000003b9aca00 0000000000000000 ffffffff80158f89 0000000000000063 
0x0160: 00000000000000ff 0000000000000000 000000000001d000 ffffffffffffffff 
0x0180: 000000003b9aca00 0000000000000000 000000000000d762 

CALLSTACK:
0xffffffff8013d270: AcpiTbInitTableDescriptor + <0x20> [0x35]
0xffffffff8013f88c: AcpiTbInstallStandardTable + <0x13c> [0x2ff]
0xffffffff8013ee78: AcpiTbParseFadt + <0xd8> [0xda]
0xffffffff801420fb: AcpiTbParseRootTable + <0x37b> [0x380]
0xffffffff801403fb: AcpiInitializeTables + <0x7b> [0x12b]
0xffffffff80154501: init_acpi + <0x51> [0x55e]
0xffffffff80158f89: kmap + <0x29> [0x2e]
0x000000000000d762: kernel_start + <0x2a2> [0xa89]

******************************
CPU[0] PANIC: #PF -RS-- at IP: 0x18:0xffffffff8013d270 SP: 0x00:0xffffffff809ffe68
******************************

Boilerplate disclamer follows:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

wipawel
wipawel previously approved these changes Oct 15, 2021
{
__start_extables = .;
*(.extables)
__stop_extables = .;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice catch. Thanks!

#include <mm/regions.h>

void init_extables(void) {
for (extable_entry_t *cur = __start_extables; cur < __stop_extables; ++cur) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to have them sanity checked. Thanks!

Next step could be to sort them and use bsearch to speed up lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we've only like 8 entries, so a linear search should still be fast enough. ;) But yeah, when we get more users and especially ones in hot code, speeding up the search might make sense.

Comment on lines -314 to +312
use_extables(regs);
if (extables_fixup(regs))
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... Thanks!

82marbag
82marbag previously approved these changes Oct 15, 2021
@82marbag 82marbag enabled auto-merge (rebase) October 15, 2021 14:16
Change the type of the __{start,end}_* markers to unsigned char so
pointer diff does "The Right Thing"™, i.e. give the byte difference,
not the object count difference.

Signed-off-by: Mathias Krause <[email protected]>
The extables handling is broken as in not being able to handle
exceptions gracefully. They'll all end up in a panic, which is probably
not intended.

Make use_extables() return a boolean that tells if the exception was
handled or not. If it was, we can simply return from the exception
handler and continue execution.

Also don't support doing both, a fixup IP and a callback. Only do one
and prefer the fixup IP which is currently the only use case.

Last, but not least, use the oppurtunity to rename the function to
extables_fixup(), as that's what it does: fixing up the exception.

Signed-off-by: Mathias Krause <[email protected]>
We round up the extables array to a multiple of 4kB. However, this leads to
empty extable entries, i.e. entries for NULL ptr faults without a fixup IP
nor a callback.

To avoid bogus handling of these, introduce a new symbol __stop_extables[]
that points to the end of legitimate extable entries. Leave __end_extables[]
as is to lower the amount of code churn. However, change its type to
unsigned char to avoid and flag accidental misuse.

Signed-off-by: Mathias Krause <[email protected]>
Some headers lack explicit includes for #defines / types they use. Add
these to avoid these implicit include dependencies.

Signed-off-by: Mathias Krause <[email protected]>
Ensure all entries have a fixup IP or a callback. Panic if an entry
lacks both.

Signed-off-by: Mathias Krause <[email protected]>
auto-merge was automatically disabled October 15, 2021 14:25

Head branch was pushed to by a user without write access

@minipli-oss minipli-oss dismissed stale reviews from 82marbag and wipawel via bb72da4 October 15, 2021 14:25
@82marbag 82marbag enabled auto-merge (rebase) October 15, 2021 14:26
@82marbag 82marbag merged commit c46bc09 into KernelTestFramework:mainline Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants