Call for review && test: linux_kdump-1.6

[Chagin Dmitry]

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

Your patch is correct for my version of linux_kdump, but does not solve 
a problem with the current port version.

If it's possible, explain please, that is not correct in my last patch?

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