Re: Call for review && test: linux_kdump-1.6

On Sat, Apr 12, 2008 at 06:49:58PM +0400, Chagin Dmitry
wrote:
> 
> Hello All,
> 
> it here: http://81.
200.6.196/linux_kdump-1.6.tar.gz
> 
> That new:
> 1) Functionality from kdump(1) except for ktr_user is
transferred
> 2) Rewrited ktr_sysret for correct Linux errno
processing (look below)
> 3) Added mksubr
> 
> Known bugs and limitations:
> 1) Some code are ugly and i mark it "XXX"
> 2) I can't test all syscalls and in kdump.c i mark it
"NOT TESTED"
> 3) Probably, we have incorrect errno processing in
ktr_sysret for syscalls
>    that return EJUSTRETURN, like linux_rt_sigreturn?
 
thnx for the great work!

> And question: whether i can add to linuxolator some
ktr_struct
>    functionality?

sure... please provide a patch and I'll take care about it.

thnx!

roman
_______________________________________________
freebsd-emulationfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-emu
lation
To unsubscribe, send any mail to
"freebsd-emulation-unsubscribefreebsd.org"

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).

-- 
Have fun!
chd
_______________________________________________
freebsd-emulationfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-emu
lation
To unsubscribe, send any mail to
"freebsd-emulation-unsubscribefreebsd.org"

On Mon, Apr 14, 2008 at 08:18:22PM +0400, Chagin Dmitry
wrote:
> On Mon, 14 Apr 2008, Kostik Belousov wrote:
> 
> >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.
> >>
> 
> I do not absolutely agree with this statement. If the
emulated syscalls 
> should return native FreeBSD errno, that why to not
write them to a 
> ktrace file without conversion? Current linux_kdump
port uses strerror 
> because expects it. At least, it is convenient 
Again, could you, please, elaborate ? ABI emulation shall
return the
translated errors. And, the current behaviour is to dump
translated
error codes, so linux_kdump must cope with it already.

> 
> Your patch is correct for my version of linux_kdump,
but does not solve 
> a problem with the current port version.
I think we could commit the trap.c patch simultaneously
with
the new linux_kdump. Even better, two versions of the
linux_kdump
could coexist in the ports, with the right one being
selected based
on the __FreeBSD_version. But I would leave this to the
emulation.

> 
> If it's possible, explain please, that is not correct
in my last patch?
Your previous patch (cited above) prevents the dumping of
the
EJUSTRETURN for the native FreeBSD syscalls, that is also
wrong, IMHO.

Assuming that your last patch is the one that moved the
ktrsysret()
before the switch (error), I see two problems:
1. It dumps the error before the ABI compat has translated
the error.
   This is definitely huge deviation with the present
behaviour, see
   above.
2. It missed the amd64 trap.c.
#1 is corrected in my version. #2 is a trivial overlook.

> 
> thnx
> 
> >>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.
^^^^^^^^This statement is about _my_ first patch.

> >
> >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:
> >
> 
> -- 
> Have fun!
> chd

On Mon, 14 Apr 2008, Kostik Belousov wrote:

> On Mon, Apr 14, 2008 at 08:18:22PM +0400, Chagin Dmitry
wrote:
>> On Mon, 14 Apr 2008, Kostik Belousov wrote:
>>
>>> On Mon, Apr 14, 2008 at 03:31:37PM +0300,
Kostik Belousov wrote:
>>>>
>>>> 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.
>>>>
>>
>> I do not absolutely agree with this statement. If
the emulated syscalls
>> should return native FreeBSD errno, that why to not
write them to a
>> ktrace file without conversion? Current linux_kdump
port uses strerror
>> because expects it. At least, it is convenient 
> Again, could you, please, elaborate ? ABI emulation
shall return the
> translated errors. And, the current behaviour is to
dump translated
> error codes, so linux_kdump must cope with it already.
>

ABI emulation shall return the translated errors to
user-space, but not 
to kernel-space where dump written. But i have understood
all, thanks! I 
simply thought, that while linux_kdump unique easier to
change rules for 
all following.

>>
>> Your patch is correct for my version of
linux_kdump, but does not solve
>> a problem with the current port version.
> I think we could commit the trap.c patch simultaneously
with
> the new linux_kdump. Even better, two versions of the
linux_kdump
> could coexist in the ports, with the right one being
selected based
> on the __FreeBSD_version. But I would leave this to the
emulation.
>

ok

>>
>> If it's possible, explain please, that is not
correct in my last patch?
> Your previous patch (cited above) prevents the dumping
of the
> EJUSTRETURN for the native FreeBSD syscalls, that is
also wrong, IMHO.
>
> Assuming that your last patch is the one that moved the
ktrsysret()
> before the switch (error), I see two problems:
> 1. It dumps the error before the ABI compat has
translated the error.
>   This is definitely huge deviation with the present
behaviour, see
>   above.
> 2. It missed the amd64 trap.c.
> #1 is corrected in my version. #2 is a trivial
overlook.
>
>>
>> thnx
>>
>>>> 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.
> ^^^^^^^^This statement is about _my_ first patch.
>
>>>
>>> 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:
>>>
>>
>> --
>> Have fun!
>> chd
>

-- 
Have fun!
chd
_______________________________________________
freebsd-emulationfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-emu
lation
To unsubscribe, send any mail to
"freebsd-emulation-unsubscribefreebsd.org"