Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> This patch addresses the recently discovered issue that I-pipe actually
>>>>> need to deal with faults over non-root domain in which the current
>>>>> domain shows no interest in. Such faults could be triggered inside
>>>>> copy_*_user, thus can cleanly be handled by Linux - if we only allow for
>>>>> this. Currently, if debugging is on, we warn about a potential bug, and
>>>>> corrupt the pipeline states otherwise.
>>>>>
>>>>> The new approach is to unconditionally drop to root domain in such
>>>>> cases, but - for debugging purposes of non-fixable faults - keep track
>>>>> of the original domain and report it on oops.
>>>>>
>>>>> Similar patches are required for other archs. Maybe I can look into
>>>>> x86_64 later.
>>>>>
>>> Nak, this patch would not work as wanted. Again, what you need is to
>>> always fixup, and conditionally send a bug report to the kernel log if
>>> CONFIG_IPIPE_DEBUG is enabled, nothing more.
>>>
>>> This patch assumes that die() is always going to be fired for any
>>> in-kernel fault, so that all reports only need to go through this
>>> routine, which is wrong. Kernel fixups through exception tables may fix
>>> the fault early and silently, and this is particularly the case for
>>> copy_to_user helpers, which do include kernel fixup code.  By being
>>> silent when fixing up things in __ipipe_handle_exception() like your
>>> patch currently is, we would be left with no trace at all that some
>>> unhandled fault just happened, except by looking at /proc/xenomai/faults.
>> As you are still remain vague on the actual problematic scenarios, I
>> will try to go through them, and maybe you can add/correct what I miss:
>>
>>  - faults in user land => can be silently handled by Linux after
>>    dropping to root domain. This lowering is perfectly fine as the
>>    higher domains showed no interest in the fault, thus are currently
>>    running in domain-agnostic code paths anyway.
>>
>>  - faults on fixable kernel addresses => same as above. If the high
>>    domains fail to evaluate the fix-up result, it's not I-pipe's fault.
>>
>>  - minor faults on kernel addresses (more precisely: in the I-pipe core
>>    or some I-pipe user) => those would now went unnoticed and need
>>    further thoughts, granted.
>> +
>>  - major faults on kernel addresses => still generate major oopses and
>>    will thus be visible.
>>
>> Did I missed something? If not, I would now start addressing the
>> remaining problematic scenario directly instead of throwing all into the
>> same pot.
>>
>>> By sending the report immediately when fixing up in the latter routine,
>>> you also avoid the ugly ipipe_orig_domain stuff.
>> It's not nice, but it is at least as ugly as reporting a kernel BUG when
>> there is only a gracefully fixable bug in user code. I definitely do not
>> agree with your approach as well, and I'm convinced we need to find a
>> third way here.
> 
> OK, I think this should be acceptable for everyone. It's basically your
> approach, reworked to avoid false positives. Moreover, it
> unconditionally sets the root domain - should be faster.

Attached the equivalent x86_64 version. Works fine here as well.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
---
 arch/x86_64/kernel/ipipe.c |   29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Index: linux-2.6.23.1-xeno_64/arch/x86_64/kernel/ipipe.c
===================================================================
--- linux-2.6.23.1-xeno_64.orig/arch/x86_64/kernel/ipipe.c
+++ linux-2.6.23.1-xeno_64/arch/x86_64/kernel/ipipe.c
@@ -612,17 +612,32 @@ asmlinkage int __ipipe_handle_exception(
 	}
 #endif /* CONFIG_KGDB */
 
-	if (!ipipe_trap_notify(vector, regs)) {
-		__ipipe_exptr handler = __ipipe_std_extable[vector];
-		handler(regs,error_code);
+	if (unlikely(ipipe_trap_notify(vector, regs))) {
 		local_irq_restore(flags);
-		__fixup_if(regs);
-		return 0;
+		return 1;
 	}
 
-	local_irq_restore(flags);
+#ifdef CONFIG_IPIPE_DEBUG
+	/* Warn about unhandled faults over non-root domains in kernel space
+	 * unless they occured at fixable locations. */
+	if (unlikely(!ipipe_root_domain_p) && !(error_code & 4) &&
+	    !search_exception_tables(instruction_pointer(regs))) {
+		struct ipipe_domain *ipd = ipipe_current_domain;
+		ipipe_current_domain = ipipe_root_domain;
+		ipipe_trace_panic_freeze();
+		printk(KERN_ERR "BUG: Unhandled exception over domain"
+				" %s - switching to ROOT\n", ipd->name);
+		dump_stack();
+	}
+#endif /* CONFIG_IPIPE_DEBUG */
 
-	return 1;
+	/* Always switch to root so that Linux can handle it cleanly. */
+	ipipe_current_domain = ipipe_root_domain;
+
+	__ipipe_std_extable[vector](regs, error_code);
+	local_irq_restore(flags);
+	__fixup_if(regs);
+	return 0;
 }
 
 asmlinkage int __ipipe_divert_exception(struct pt_regs *regs, int vector)
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to