Call for review && test: linux_kdump-1.6

[Kostik Belousov]

On Mon, Apr 14, 2008 at 03:31:37PM +0300, Kostik Belousov wrote:
> 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/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

The patch is obviously wrong, it just prevents the Linux ENOENT to be
dumped. Please, try this one instead.

diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index b6a454d..3b1368a 100644
--- a/sys/amd64/amd64/trap.c
+++ b/sys/amd64/amd64/trap.c
@@ -861,9 +861,18 @@ syscall(struct trapframe *frame)
 		frame->tf_rip -= frame->tf_err;
 		frame->tf_r10 = frame->tf_rcx;
 		td->td_pcb->pcb_flags |= PCB_FULLCTX;
-		break;
-
+		/* FALLTHROUGH */
 	case EJUSTRETURN:
+#ifdef KTRACE
+		/*
+		 * 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 = 0;
+#endif
 		break;
 
 	default:
diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
index e7de579..6ec04b0 100644
--- a/sys/i386/i386/trap.c
+++ b/sys/i386/i386/trap.c
@@ -1040,9 +1040,18 @@ syscall(struct trapframe *frame)
 		 * int 0x80 is 2 bytes. We saved this in tf_err.
 		 */
 		frame->tf_eip -= frame->tf_err;
-		break;
-
+		/* FALLTHROUGH */
 	case EJUSTRETURN:
+#ifdef KTRACE
+		/*
+		 * 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 = 0;
+#endif
 		break;
 
 	default:
-------------- 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/eac3db95/attachment.pgp