Opened 6 years ago

Last modified 11 months ago

#10169 assigned bug

Double Fault Exception on usb boot x86_64

Reported by: korli Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: System/Kernel Version: R1/Development
Keywords: boot-failure Cc:
Blocked By: Blocking:
Has a Patch: no Platform: x86-64

Description

I reproduced on real hardware and QEmu 1.4.0

Here is the QEmu command line (090c:1000 is the vendor_id:device_id for the usb key)

qemu-system-x86_64 -m 512 -usb -serial stdio  --enable-kvm -usbdevice host:090c:1000 -boot menu=on

then press F12, then select the USB device.

As you may understand this bug is a bit blocking to test x86_64 on real hardware :)

Attachments (1)

log_usb_boot_hrev46281_x86_64.txt (37.7 KB) - added by korli 6 years ago.
panic serial log

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by korli

panic serial log

comment:1 Changed 6 years ago by korli

It seems the kernel stack is the same size on 32-bit and 64-bit, which might explain the behavior difference between architectures. The following patch fixes the problem by increasing the kernel stack size by another page.

Is it OK as is or would it be better to differentiate with B_HAIKU_64_BIT?

diff --git a/headers/private/kernel/kernel.h b/headers/private/kernel/kernel.h
index a5c01e3..d068a48 100644
--- a/headers/private/kernel/kernel.h
+++ b/headers/private/kernel/kernel.h
@@ -47,7 +47,7 @@
        // At least, you then know that the stack overflows in this case :)
 
 /** Size of the kernel stack */
-#define KERNEL_STACK_SIZE              (B_PAGE_SIZE * 3)       // 12 kB
+#define KERNEL_STACK_SIZE              (B_PAGE_SIZE * 4)       // 16 kB
 
 #ifdef DEBUG_KERNEL_STACKS
 #      define KERNEL_STACK_GUARD_PAGES 1

comment:2 Changed 6 years ago by axeld

IIRC it's only 3 pages because of the guard pages -- at least IIRC it used to be 8 kB. Anyway, it should be a 64-bit only change. The kernel stacks can take a lot of (unpageable) memory when you have many threads around.

comment:3 Changed 6 years ago by korli

Fine. But isn't the 2832 bytes stack usage in vm_page_write_modified_page_range() a bit high anyway?

ffffffff811a8010 (+2832) ffffffff8010f3b4   <kernel_x86_64> vm_page_write_modified_page_range() + 0x4cb

comment:4 in reply to:  3 Changed 6 years ago by bonefish

Replying to korli:

Fine. But isn't the 2832 bytes stack usage in vm_page_write_modified_page_range() a bit high anyway?

Yes, that's way too much. The main issue is

PageWriteWrapper* wrappers[maxPages];

With maxPages being up to 256, this means 2048 bytes on a 64 bit architecture. It should be allocated in the same way wrapperPool is allocated (i.e. on the heap with small stack array fallback).

Another issue is the PageWriteTransfer object allocated on the stack. The class contains the following attribute:

generic_io_vec      fVecs[32]; // TODO: make dynamic/configurable

That's another 512 bytes.

One can see several heavy stack users in the stack trace. The file cache's read_into_cache() being the most problematic, since there's no limit for nested mounting (e.g. an image file in an image file in an image file ...), with certain calls going through as many file cache layers.

I suppose many functions could use a review wrt stack usage. Even 200 bytes is quite a bit. And all code that isn't performance critical and is allowed to fail could allocate variables on the heap.

comment:5 Changed 6 years ago by korli

Applied a working patch in hrev46314.

comment:6 Changed 6 years ago by korli

Here is a patch for the wrappers stack usage issue. I have no idea better than stackWrappers2 for the variable name ATM.

Testing with a 128MB VM seemed OK. I encountered "PageWriteWrapper: Failed to write page" messages in the syslog on low memory condition with or without this patch.

diff --git a/src/system/kernel/vm/vm_page.cpp b/src/system/kernel/vm/vm_page.cpp
index ee20257..34d7370 100644
--- a/src/system/kernel/vm/vm_page.cpp
+++ b/src/system/kernel/vm/vm_page.cpp
@@ -3081,7 +3081,14 @@ vm_page_write_modified_page_range(struct VMCache* cache, uint32 firstPage,
 
        int32 nextWrapper = 0;
 
-       PageWriteWrapper* wrappers[maxPages];
+       PageWriteWrapper* stackWrappers2[1];
+       PageWriteWrapper** wrappers
+               = new(malloc_flags(allocationFlags)) PageWriteWrapper*[maxPages];
+       if (wrappers == NULL) {
+               // don't fail, just limit our capabilities
+               wrappers = stackWrappers2;
+               maxPages = 1;
+       }
        int32 usedWrappers = 0;
 
        PageWriteTransfer transfer;
@@ -3153,6 +3160,8 @@ vm_page_write_modified_page_range(struct VMCache* cache, uint32 firstPage,
 
        if (wrapperPool != stackWrappers)
                delete[] wrapperPool;
+       if (wrappers != stackWrappers2)
+               delete[] wrappers;
 
        return B_OK;
 }

comment:7 Changed 6 years ago by bonefish

Regarding the variable naming, I would rename the old stackWrappers to stackWrapperPool (to match the naming of the actual variable) and go with stackWrappers for the new variable.

I'd also merge the two allocation checks into a single if, like this:

PageWriteWrapper* wrapperPool = ... allocate ...
PageWriteWrapper** wrappers = ... allocate ...
if (wrapperPool == NULL || wrappers == NULL) {
  delete both
  use stack
}

comment:8 Changed 6 years ago by korli

Thanks for the feedback. Applied with your ideas in hrev46354.

read_into_cache() has already a TODO about the stack usage. It would be interesting if there is any static analysis tool to provide stack usage information, but I doubt any exists.

comment:9 in reply to:  8 ; Changed 6 years ago by umccullough

Replying to korli:

It would be interesting if there is any static analysis tool to provide stack usage information, but I doubt any exists.

Not sure what you're looking for specifically. Coverity does have a "stack depth" checker but I have little clue how to configure it. I found this snippet in the documentation:

A.1.5.1.11.  __coverity_stack_depth__(<max_memory>)

Indicates to the STACK_USE checker that the function and its callees should not use more memory (in bytes) than specified by the constant integer <max_memory>. This feature is useful for situations where threads are created with different stack sizes. The primitive should be used in the thread entry-point function.

It seems perhaps you can create a "model" to declare this primitive and it will check the stack below any given function that is modeled.

comment:10 in reply to:  9 ; Changed 6 years ago by korli

Replying to umccullough:

It seems perhaps you can create a "model" to declare this primitive and it will check the stack below any given function that is modeled.

I suppose Coverity should fit our use case then. You can try with 8KB or less for kernel threads. We probably have to insert coverity_stack_depth in our code. Kernel entry points would be kernel threads and syscall entry points.

comment:11 in reply to:  10 Changed 6 years ago by umccullough

Replying to korli:

We probably have to insert coverity_stack_depth in our code. Kernel entry points would be kernel threads and syscall entry points.

I think they also provide a "model file" submission for analysis purposes, to direct Coverity how to treat certain functions so you don't have to do anything with the code.

I admit, I have only a vague idea of how it is supposed to work, but I have made you an admin if you'd like to investigate and mess with it. You can login and then visit this page: https://scan.coverity.com/projects/82?tab=Analysis+Settings

Further documentation is available here: https://scan.coverity.com/tune and here: https://scan.coverity.com/models

comment:12 Changed 5 years ago by luroh

Blocking: 7665 added

comment:13 Changed 5 years ago by luroh

Milestone: R1Unscheduled

Moving non-x86 related tickets out of R1 milestone.

comment:14 Changed 2 years ago by axeld

Owner: changed from axeld to nobody
Status: newassigned

comment:15 Changed 11 months ago by waddlesplash

Keywords: boot-failure added

comment:16 Changed 11 months ago by waddlesplash

Blocking: 7665 removed
Note: See TracTickets for help on using tickets.