Call for review && test: linux_kdump-1.6

[Kostik Belousov]

On Mon, Apr 14, 2008 at 12:42:29AM +0400, Chagin Dmitry wrote:
> On Sun, 13 Apr 2008, Kostik Belousov wrote:
> 
> >On Sun, Apr 13, 2008 at 11:11:55PM +0400, Chagin Dmitry wrote:
> >>On Sun, 13 Apr 2008, Kostik Belousov wrote:
> >>
> >>>On Sun, Apr 13, 2008 at 08:32:48PM +0200, Roman Divacky wrote:
> >>>>On Sun, Apr 13, 2008 at 09:58:08PM +0400, Chagin Dmitry wrote:
> >>>>>On Sat, 12 Apr 2008, Roman Divacky wrote:
> >>>>>
> >>>>>>>And question: whether i can add to linuxolator some ktr_struct
> >>>>>>>  functionality?
> >>>>>>
> >>>>>>sure... please provide a patch and I'll take care about it.
> >>>>>
> >>>>>ok, thnx :)
> >>>>>what about EJUSTRETURN?
> >>>>>i attached simple patch for demo only (not tested).
> >>>>
> >>>>uh... can you provide diff -u ? I dont understand the diff at all ;)
> >>>
> >>>Also, please note that the ML software strips your attachments. Either
> >>>inline the patch, or use the plain-text content-type for it.
> >>>
> >>
> >>ups... ah google ))
> >>i have understood, sorry and thnx.
> >>Speech about that in linux_kdump it is impossible to distinguish
> >>EJUSTRETURN from a real error. look:
> >>
> >>--- sys/i386/i386/trap.c.orig	2008-04-13 21:39:18.000000000 +0400
> >>+++ sys/i386/i386/trap.c	2008-04-13 22:35:25.000000000 +0400
> >>@@ -1091,8 +1091,12 @@
> >>  	    td->td_proc->p_pid, td->td_name, code);
> >>
> >>  #ifdef KTRACE
> >>-	if (KTRPOINT(td, KTR_SYSRET))
> >>-		ktrsysret(code, error, td->td_retval[0]);
> >>+	if (KTRPOINT(td, KTR_SYSRET)) {
> >>+		if (error == EJUSTRETURN)
> >>+			ktrsysret(code, 0, td->td_retval[0]);
> >>+		else
> >>+			ktrsysret(code, error, td->td_retval[0]);
> >>+	}
> >>  #endif
> >>
> >>  	/*
> >>@@ -1104,4 +1108,3 @@
> >>
> >>  	PTRACESTOP_SC(p, td, S_PT_SCX);
> >>  }
> >>-
> >
> >I do not quite understand the intent of this change.
> >
> >EJUSTRETURN is used for two different purposes in the kernel.
> >1. The sigreturn family of the syscalls use it after the interrupted
> >frame is restored to avoid the normal syscall return sequence to modify
> >the machine state.
> >2. It is used by the kernel to notify the in-kernel caller code about
> >some special condition, that nonetheless shall not be returned to the
> >userspace.
> >
> >Only the first case is applicable to the kdump, and IMHO you actually
> >destroy some information, since error == EJUSTRETURN is reported as 0.
> >
> >Could you, please, provide some more arguments in the support of your
> >proposed change ?
> >
> 
> Thanks for you informative reply Kostya.
> The problem arises only in linux_kdump. Because linux error
> codes negative and EJUSTRETURN coincides with ENOENT.
> Before a call ktr_sysret we decode return codes of emulators syscalls.

Ah, I see. Then, we shall never dump the ERESTART and EJUSTRETURN
for the emulated ABIs. At least, this is true for Linux, I am not
so sure about iBCS2 and SVR4.

Could you test the patch below, instead ?

diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index b6a454d..9ccbd4b 100644
--- a/sys/amd64/amd64/trap.c
+++ b/sys/amd64/amd64/trap.c
@@ -912,8 +912,23 @@ syscall(struct trapframe *frame)
 	    td->td_proc->p_pid, td->td_name, code);
 
 #ifdef KTRACE
-	if (KTRPOINT(td, KTR_SYSRET))
-		ktrsysret(code, error, td->td_retval[0]);
+	if (KTRPOINT(td, KTR_SYSRET)) {
+		int error1;
+
+		/*
+		 * The ABIs that use the negative error codes, like
+		 * Linux, would confuse the in-kernel errno values
+		 * with proper userspace errno. Clean these values to
+		 * avoid a confusion in the kdump.
+		 */
+		if (p->p_sysent->sv_errsize &&
+		    (error == EJUSTRETURN || error == ERESTART))
+			error1 = 0;
+		else
+			error1 = error;
+
+		ktrsysret(code, error1, td->td_retval[0]);
+	}
 #endif
 
 	/*
diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
index e7de579..55642d1 100644
--- a/sys/i386/i386/trap.c
+++ b/sys/i386/i386/trap.c
@@ -1091,8 +1091,22 @@ syscall(struct trapframe *frame)
 	    td->td_proc->p_pid, td->td_name, code);
 
 #ifdef KTRACE
-	if (KTRPOINT(td, KTR_SYSRET))
-		ktrsysret(code, error, td->td_retval[0]);
+	if (KTRPOINT(td, KTR_SYSRET)) {
+		int error1;
+
+		/*
+		 * The ABIs that use the negative error codes, like
+		 * Linux, would confuse the in-kernel errno values
+		 * with proper userspace errno. Clean these values to
+		 * avoid a confusion in the kdump.
+		 */
+		if (p->p_sysent->sv_errsize &&
+		    (error == EJUSTRETURN || error == ERESTART))
+			error1 = 0;
+		else
+			error1 = error;
+		ktrsysret(code, error1, td->td_retval[0]);
+	}
 #endif
 
 	/*
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-emulation/attachments/20080414/fc5b52ea/attachment.pgp