Fix a panic when userland attempts to flush an invalid addres

[andrew]

Summary

Without this I get:

Fatal kernel mode data abort: 'Instruction cache maintenance' on write

when attempting to flush the page starting at 0. This change checks if the
page is mapped in the applications pmap, and if so only then flushes it.

[• ian]

I think ignoring umpapped pages is wrong. If a userlang process tries to do something to an unmapped virtual address, that's a SIGBUS. If it tries to do something incompatible with the way a page is mapped (persmissions, icache sync on non-exec page), that's a SIGSEGV.

An interesting question that pops into my head... does this routine need to do something to ensure the mappings don't change during the operation (say, on another processor)? For example, what if one userland thread is doing this at the same time that another thread in the process does something that kills the whole app?

sys/arm/arm/sys_machdep.c

78

There is no need to round to a cacheline boundary, that's handled by the lower levels (by the hardware for armv7).

Also, hardcoded 32 is evil and not a cacheline boundary on all chips.

82

Shouldn't this just use td, since it's passed to the function already?

84

I can't see any need to do this partial-page / full page stuff. pmap_extract() returns the physical page given any va within the page.

[andrew]

Lock the pmap before accessing it and send a SIGBUS when a page is not mapped

[mmel]

I still have a problem with it. Not all valid user pages must be mapped at the time of the sync_icache () call. Unlimited/unchecked address/size is open gate to effective DoS from userland.
I think that right solution for this problem is:

• check and limit arguments for cpu_icache_sync_range().
• use pcb_onfault in arm32_sync_icache() for handle aborts(errors - access to unmapped pages).
• change cache maintenance abort type from abort_fatal to NULL.

[• ian]

After looking into this some more I think a complete correct solution is going to be pretty complex compared to the current code. I think pcb_onfault can only be used from asm code. The onfault handler has to undo any function-entry stuff done by the faulting routine (unwind the stack, basically). It's not like a real setjmp/longjmp system, it's just the longjmp part.

If we think about how this function is used in the real world, I think we can live with a partial solution for a while. It's used by things like JIT compilers, and also by debuggers to insert/remove breakpoints. In both cases, the memory being sync'd was just written to and is very unlikely to be paged out between the write and the call to this function (because by definition the page is dirty, and dirty pages are the last thing to be reclaimed when memory gets low).

I think for now we can leave the code as it is (kill the process on unmapped/un-present page), maybe with a comment block that mentions that vm_fault() handling is needed eventually. Maybe we should even enter a PR for it so we remember to fix it some day.

[mmel]

I agree. Then, can you move loop code into function in pmap-v6.c, please? Exposing pmap internals to outside world makes our new pmap job little difficult. Thanks.

[sbruno]

Is this going to get a structural reworking? I don't think that the current reviews make it likely that this is going to be accepted as is.

[skra]

It's solved correctly in https://reviews.freebsd.org/D2035.

[skra]

D2035 was committed in r283947. Can this be closed/abandoned?

[emaste]

Ping