Paths

Table of Contentst

Errors caused by DTrace are not correctly handled by ARMv6 abort_handler()
AcceptedPublic
Actions

Authored by graeme.jenkinson_cl.cam.ac.uk on Feb 21 2017, 11:54 AM.

Details

Reviewers

andrew
gnn
mmel

Group Reviewers

ARM

Summary

The DTrace script:

fbt::tcP_do_segment:entry{

tdatalen = (((struct ip *)args[0]->m_data)->ip_hl << 2) + (args[1]->th_off << 2);

}

resulted in a Translation (L1) fault on ARMv6 (BeagleBone Black). The translation fault caused by the script was not being correctly handled, the following issues where identified:

dtrace_trap - didn't handle FAULT_TRAN_1 or FAULT_TRAN_2 (only FAULT_ALIGN).
abort_handler - didn't call the dtrace_trap() function early enough to handle faults caused by DTrace
dtrace_trap - was called with the fault address register rather than the fault status

I have move the call to dtrace_trap to the earliest available point in abort_handler(), rather than create a separate function as in the amd64 handler to keep changes to a minimum.

Something similar should be done for < ARMv6. I need to further understand the errors that need to be handled in that case. I don't have a board to test the changes so will be cautious before making those changes,

Test Plan

Check that the original script no longer results in a Translation Fault:

dtrace: error on enabled probe ID 1 (ID 12949: fbt:kernel:tcp_do_segment:entry): invalid address (0x0) in action #1 at DIF offset 44

Also, checked that valid fbt providers still function correctly

fbt::tcp_do_segment:entry
{

printf("%d", notes(args[0]]->m_len);

}

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

graeme.jenkinson_cl.cam.ac.uk updated this revision to Diff 25462.Feb 21 2017, 11:54 AM

graeme.jenkinson_cl.cam.ac.uk retitled this revision from to Errors caused by DTrace are not correctly handled by ARMv6 abort_handler().

graeme.jenkinson_cl.cam.ac.uk updated this object.

graeme.jenkinson_cl.cam.ac.uk edited the test plan for this revision. (Show Details)

graeme.jenkinson_cl.cam.ac.uk added a reviewer: andrew.

graeme.jenkinson_cl.cam.ac.uk set the repository for this revision to rS FreeBSD src repository - subversion.

graeme.jenkinson_cl.cam.ac.uk added projects: DTrace, ARM.

Herald added a reviewer: gnn. · View Herald TranscriptFeb 21 2017, 11:54 AM

Herald added a subscriber: imp. · View Herald Transcript

andrew added a reviewer: ARM.Feb 21 2017, 12:19 PM

andrew added inline comments.Feb 21 2017, 2:00 PM

sys/arm/arm/trap-v6.c
318	This should be `!= 0`
sys/cddl/dev/dtrace/arm/dtrace_subr.c
204	No need for a `FALLTHROUGH` comment when there is no code.

Can you be, please, little more specific for
"abort_handler - didn't call the dtrace_trap() function early enough to handle faults caused by DTrace " ?

Which faults are not handled if KDTRACE_HOOKS is on original location?
I'm only curious here, and my knowledge of DTRACE are very limited. But are you sure that DTRACE wants to see faults, possibly handled by vm_fault()? Or by pcb_onfault()... ?

mmel added inline comments.Feb 23 2017, 6:31 AM

sys/arm/arm/trap-v6.c
311	Also, i don't think that this is right place for KDTRACE_HOOKS. Everything before line 353 (in original file, " if (__predict_false((td->td_pflags & TDP_NOFAULTING) != 0)) {" is extreme hot path. pmap_fault() emulates MODIFY and ACCESSED bits in PTE, and I'm sure that these traps are not subject for dtrace. pmap_fault() also protect us from endless recursive loop - if faulted address is in mapped page table region, then any access to it ends with trap recursion and stack overflow. FAULT_EA_IMPREC is asynchronous, its not executed in context of faulting instruction. There is no way to recovery from it.

In D9701#201533, @meloun-miracle-cz wrote:

Can you be, please, little more specific for
"abort_handler - didn't call the dtrace_trap() function early enough to handle faults caused by DTrace " ?

Which faults are not handled if KDTRACE_HOOKS is on original location?
I'm only curious here, and my knowledge of DTRACE are very limited. But are you sure that DTRACE wants to see faults, possibly handled by vm_fault()? Or by pcb_onfault()... ?

DTrace is designed for safe in-production analysis, that means that DTrace should not bring the system down. When DTrace probe's fire they execute in a restricted environment call "probe context". For these purposes, the important point is that probe context occurs in kernel and only accesses wired pages.

In my example a malformed script results in a memory access to 0x02. The resulting translation fault brings the system down. This should not happen.

dtrace_trap() tests whether the system is in probe context, by checking the flag cpu_core[curcpu].cpuc_dtrace_flags & CPU_DTRACE_NOFAULT. If this is set the translation fault should not be handled by the normal code path, instead the DTrace error probe should fire and the DTrace script resume processing.

In the original code dtrace_trap() was called from abort_fatal(). abort_fatal() is never called for translation faults (or alignment faults). It would be possible to populate the aborts array with handlers that call dtrace_trap() infor L1 and L2 translation faults and alignment faults, but I think that this is a bit messy. Other architectures handling dtrace_trap() immediately on entering the trap handler (I'll expand on this point when replying to the other comment).

sys/arm/arm/trap-v6.c
318	It's a bit ambiguous. But I can agree that dtrace_trap returns 0 if the trap should be handled in the usual way, so I'll change.
sys/cddl/dev/dtrace/arm/dtrace_subr.c
204	OK. The style is a bit unclear on this.

In my example a malformed script results in a memory access to 0x02. The resulting translation fault brings the system down. This should not happen.

Yes, due to missing FAULT_TRAN_L[1][2]: case in dtrace_trap().

dtrace_trap() tests whether the system is in probe context, by checking the flag cpu_core[curcpu].cpuc_dtrace_flags & CPU_DTRACE_NOFAULT. If this is set the translation fault should not be handled by the normal code path, instead the DTrace error probe should fire and the DTrace script resume processing.

In the original code dtrace_trap() was called from abort_fatal(). abort_fatal() is never called for translation faults (or alignment faults). It would be possible to populate the aborts array with handlers that call dtrace_trap() infor L1 and L2 translation faults and alignment faults, but I think that this is a bit messy.

I don't agree here. All fatal traps ends in abort_fatal(), including all translation or alignment faults.
Kernel mode FAULT_TRAN_L[1][2] traps calls abort_fatal() at line 501.

Other architectures handling dtrace_trap() immediately on entering the trap handler (I'll expand on this point when replying to the other comment).

But other architectures doesn't emulates access or modify bits in SW. In peaks, quadcore ARM can easily get over 100k/sec of traps, filtered down (by pmap_fault()) to 3~5 k of 'real' ones.

But again, are you're really sure that calling dtrace probe for trap caused by swapped out page(for example) and thus fully covered by vm_fault() is right one?
In other words, can dtrace probe handle these 'false alarms' ?
In my opinion no, at least not on ARM.

In any case, I haven't a single problem if you put dtrace hook anywhere behind the line 352.

In D9701#201579, @meloun-miracle-cz wrote:

In my example a malformed script results in a memory access to 0x02. The resulting translation fault brings the system down. This should not happen.

Yes, due to missing FAULT_TRAN_L[1][2]: case in dtrace_trap().

dtrace_trap() tests whether the system is in probe context, by checking the flag cpu_core[curcpu].cpuc_dtrace_flags & CPU_DTRACE_NOFAULT. If this is set the translation fault should not be handled by the normal code path, instead the DTrace error probe should fire and the DTrace script resume processing.

In the original code dtrace_trap() was called from abort_fatal(). abort_fatal() is never called for translation faults (or alignment faults). It would be possible to populate the aborts array with handlers that call dtrace_trap() infor L1 and L2 translation faults and alignment faults, but I think that this is a bit messy.

I don't agree here. All fatal traps ends in abort_fatal(), including all translation or alignment faults.
Kernel mode FAULT_TRAN_L[1][2] traps calls abort_fatal() at line 501.

Other architectures handling dtrace_trap() immediately on entering the trap handler (I'll expand on this point when replying to the other comment).

But other architectures doesn't emulates access or modify bits in SW. In peaks, quadcore ARM can easily get over 100k/sec of traps, filtered down (by pmap_fault()) to 3~5 k of 'real' ones.

But again, are you're really sure that calling dtrace probe for trap caused by swapped out page(for example) and thus fully covered by vm_fault() is right one?
In other words, can dtrace probe handle these 'false alarms' ?
In my opinion no, at least not on ARM.

In any case, I haven't a single problem if you put dtrace hook anywhere behind the line 352.

I'll double check, but I'm fairly sure dtrace_trap isn't invoked when it's in the abort handler. What about the pmap_fault range check? (I confess to being not 100% familiar with ARM or the fine details of what is being done here.) Hmmm, that's call nogo: which should all abort_fatal(). I'll double check...

sys/arm/arm/trap-v6.c
311	amd64 does the following: void trap_check(struct trapframe frame) { #ifdef KDTRACE_HOOKS if (dtrace_trap_func != NULL && (dtrace_trap_func)(frame, frame->tf_trapno) != 0) return; #endif trap(frame); } I have attempted to provide a solution that is as close to this, whilst minimizing changes. If I place the `dtrace_trap_func` call after `pmap_fault()` wouldn't the range checking mean that the kernel panics in my example malformed DTrace script? Whether `dtrace_trap_func` is called before or after `FAULT_EA_IMPREC` seems pretty mute to me.

Confirmed that calling dtrace_trap_func from abort_fatal() is correct (my mistake).

Herald edited edge metadata. · View Herald TranscriptFeb 23 2017, 1:33 PM

Perfect :)
By my opinion, kernel should call dtrace_trap_func () only in unresolvable kernel trap case, as alternative to panic.
Something like: oups, dtrace probe causes panic, recovery me.

I'm fine with rest of this review.

mmel accepted this revision.Feb 23 2017, 2:14 PM

mmel added a reviewer: mmel.

gnn accepted this revision.Feb 23 2017, 6:08 PM

gnn edited edge metadata.

This revision is now accepted and ready to land.Feb 23 2017, 6:08 PM

Revision Contents
Changeset List

Path

Size

sys/

arm/

trap-v6.c

21 lines

cddl/

dev/

dtrace/

arm/

dtrace_subr.c

6 lines

Diff 25462

View Options

sys/arm/arm/trap-v6.c