Is it a bug in etrap.S srmmu stack check routine?

Is it a bug in etrap.S srmmu stack check routine?

[Eldar Abusalimov]

Hi, David!

I've been looking into traps-related routines for some time, and was
confused with srmmu user stack checking code.

Here is the corresponding part of etrap.S file:

278 	        .globl  tsetup_srmmu_stackchk
279 	tsetup_srmmu_stackchk:
280 	        /* Check results of callers andcc %sp, 0x7, %g0 */
281 	        bne     trap_setup_user_stack_is_bolixed
282 	         sethi   %hi(PAGE_OFFSET), %glob_tmp
283 	
284 	        cmp     %glob_tmp, %sp
285 	        bleu,a  1f
286 	         lda    [%g0] ASI_M_MMUREGS, %glob_tmp          ! read MMU control
287 	
288 	trap_setup_user_stack_is_bolixed:
289 	        /* From user/kernel into invalid window w/bad user
290 	         * stack. Save bad user stack, and return to caller.
291 	         */
292 	        SAVE_BOLIXED_USER_STACK(curptr, g3)
293 	        restore %g0, %g0, %g0
294 	
295 	        jmpl    %t_retpc + 0x8, %g0
296 	         mov    %t_kstack, %sp
297 	
298 	1:
299 	        /* Clear the fault status and turn on the no_fault bit. */
300 	        or      %glob_tmp, 0x2, %glob_tmp               ! or in
no_fault bit
301 	        sta     %glob_tmp, [%g0] ASI_M_MMUREGS          ! set it
302 	
303 	        /* Dump the registers and cross fingers. */
304 	        STORE_WINDOW(sp)

On the line 284 we compare user stack pointer with PAGE_OFFSET, and if
it is greater than PAGE_OFFSET, then we consider user stack is still
ok and continue checking. On the contrary, if %sp is small enough,
bollixed stack handler is entered at once. Is it right?

The same part of wof.S looks more logical - if %sp is greater than
kernel base, then branch into a spwin_user_stack_is_bolixed.

398 	         sethi   %hi(PAGE_OFFSET), %glob_tmp
399 	        cmp     %glob_tmp, %sp
400 	        bleu    spwin_user_stack_is_bolixed

I am perplexed especially because of the fact that this part of logic
has not been changed since 1999, and nevertheless Linux works well.
(but trap_setup_user_stack_is_bolixed is reached in fact, I've just
tested on the hardware with grmon).
Linux version is 2.6.21.1.

Please let me know if this is really a bug or where am I wrong else.
Thanks in advance.

-- 
Best regards,
Eldar Sh. Abusalimov

Re: Is it a bug in etrap.S srmmu stack check routine?

[David Miller]

From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
Date: Mon, 3 Aug 2009 21:36:31 +0400

> Hi, David!
> 
> I've been looking into traps-related routines for some time, and was
> confused with srmmu user stack checking code.
> 
> Here is the corresponding part of etrap.S file:
> 
> 278 	        .globl  tsetup_srmmu_stackchk
> 279 	tsetup_srmmu_stackchk:
> 280 	        /* Check results of callers andcc %sp, 0x7, %g0 */
> 281 	        bne     trap_setup_user_stack_is_bolixed
> 282 	         sethi   %hi(PAGE_OFFSET), %glob_tmp
> 283 	
> 284 	        cmp     %glob_tmp, %sp
> 285 	        bleu,a  1f
> 286 	         lda    [%g0] ASI_M_MMUREGS, %glob_tmp          ! read MMU control
> 287 	
> 288 	trap_setup_user_stack_is_bolixed:
> 289 	        /* From user/kernel into invalid window w/bad user
> 290 	         * stack. Save bad user stack, and return to caller.
> 291 	         */
> 292 	        SAVE_BOLIXED_USER_STACK(curptr, g3)
> 293 	        restore %g0, %g0, %g0
> 294 	
> 295 	        jmpl    %t_retpc + 0x8, %g0
> 296 	         mov    %t_kstack, %sp
> 297 	
> 298 	1:
> 299 	        /* Clear the fault status and turn on the no_fault bit. */
> 300 	        or      %glob_tmp, 0x2, %glob_tmp               ! or in
> no_fault bit
> 301 	        sta     %glob_tmp, [%g0] ASI_M_MMUREGS          ! set it
> 302 	
> 303 	        /* Dump the registers and cross fingers. */
> 304 	        STORE_WINDOW(sp)
> 
> On the line 284 we compare user stack pointer with PAGE_OFFSET, and if
> it is greater than PAGE_OFFSET, then we consider user stack is still
> ok and continue checking. On the contrary, if %sp is small enough,
> bollixed stack handler is entered at once. Is it right?

We want to fall through to line 288 if PAGE_OFFSET > %sp and that's
what we do.

Your reading of the logic seems to be reversed, either that or you're
missing the fallthrough there for when the branch isn't taken. :-)

Re: Is it a bug in etrap.S srmmu stack check routine?

[Eldar Abusalimov]

Forwarding it here.


---------- Forwarded message ----------
From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
Date: 2009/8/4
Subject: Re: Is it a bug in etrap.S srmmu stack check routine?
To: David Miller <davem@davemloft.net>


2009/8/4 David Miller <davem@davemloft.net>:
> From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
> Date: Mon, 3 Aug 2009 21:36:31 +0400
>
>> Hi, David!
>>
>> I've been looking into traps-related routines for some time, and was
>> confused with srmmu user stack checking code.
>>
>> Here is the corresponding part of etrap.S file:
>>
>> 278           .globl  tsetup_srmmu_stackchk
>> 279   tsetup_srmmu_stackchk:
>> 280           /* Check results of callers andcc %sp, 0x7, %g0 */
>> 281           bne     trap_setup_user_stack_is_bolixed
>> 282            sethi   %hi(PAGE_OFFSET), %glob_tmp
>> 283
>> 284           cmp     %glob_tmp, %sp
>> 285           bleu,a  1f
>> 286            lda    [%g0] ASI_M_MMUREGS, %glob_tmp          ! read MMU control
>> 287
>> 288   trap_setup_user_stack_is_bolixed:
>> 289           /* From user/kernel into invalid window w/bad user
>> 290            * stack. Save bad user stack, and return to caller.
>> 291            */
>> 292           SAVE_BOLIXED_USER_STACK(curptr, g3)
>> 293           restore %g0, %g0, %g0
>> 294
>> 295           jmpl    %t_retpc + 0x8, %g0
>> 296            mov    %t_kstack, %sp
>> 297
>> 298   1:
>> 299           /* Clear the fault status and turn on the no_fault bit. */
>> 300           or      %glob_tmp, 0x2, %glob_tmp               ! or in
>> no_fault bit
>> 301           sta     %glob_tmp, [%g0] ASI_M_MMUREGS          ! set it
>> 302
>> 303           /* Dump the registers and cross fingers. */
>> 304           STORE_WINDOW(sp)
>>
>> On the line 284 we compare user stack pointer with PAGE_OFFSET, and if
>> it is greater than PAGE_OFFSET, then we consider user stack is still
>> ok and continue checking. On the contrary, if %sp is small enough,
>> bollixed stack handler is entered at once. Is it right?
>
> We want to fall through to line 288 if PAGE_OFFSET > %sp and that's
> what we do.
>

Thank you for the reply.

That is PAGE_OFFSET > %sp means a corrupt user stack condition, isn't
it? Then why we do the same check in wof.S (see my first post), but
the stack is considered to be corrupt under the opposite condition
(when PAGE_OFFSET <= %sp)? What's the difference between these two
cases?

My questions may look silly, but I still have not understood what's
wrong in my chain of reasoning. =)

--
Best regards,
Eldar Sh. Abusalimov



-- 
Best regards,
Eldar Sh. Abusalimov

Re: Is it a bug in etrap.S srmmu stack check routine?

[David Miller]

From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
Date: Tue, 4 Aug 2009 13:41:02 +0400

> Forwarding it here.

I sent a reply to the private email, if you can't keep this discussion
properly on the list I'm not going to repeat myself here.

Re: Is it a bug in etrap.S srmmu stack check routine?

[Eldar Abusalimov]

2009/8/4 David Miller <davem@davemloft.net>:
> No, a bad user stack would be if %sp >= PAGE_OFFSET, the user stack
> must be below the lowest kernel address which is PAGE_OFFSET.

Yes, I agree, but you've just written that

>>> We want to fall through to line 288 if PAGE_OFFSET > %sp and that's
>>> what we do.

and line 288 is trap_setup_user_stack_is_bolixed branch.

-- 
Best regards,
Eldar Sh. Abusalimov

Re: Is it a bug in etrap.S srmmu stack check routine?

[David Miller]

From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
Date: Tue, 4 Aug 2009 17:07:32 +0400

> 2009/8/4 David Miller <davem@davemloft.net>:
>> No, a bad user stack would be if %sp >= PAGE_OFFSET, the user stack
>> must be below the lowest kernel address which is PAGE_OFFSET.
> 
> Yes, I agree, but you've just written that
> 
>>>> We want to fall through to line 288 if PAGE_OFFSET > %sp and that's
>>>> what we do.
> 
> and line 288 is trap_setup_user_stack_is_bolixed branch.

Sorry, logic reversed.

Re: Is it a bug in etrap.S srmmu stack check routine?

[Eldar Abusalimov]

2009/8/4 David Miller <davem@davemloft.net>:
> From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
> Date: Tue, 4 Aug 2009 17:09:42 +0400
>
>> 2009/8/4 David Miller <davem@davemloft.net>:
>>> From: Eldar Abusalimov <eldar.abusalimov@gmail.com>
>>> Date: Tue, 4 Aug 2009 17:07:32 +0400
>>>
>>>> 2009/8/4 David Miller <davem@davemloft.net>:
>>>>> No, a bad user stack would be if %sp >= PAGE_OFFSET, the user stack
>>>>> must be below the lowest kernel address which is PAGE_OFFSET.
>>>>
>>>> Yes, I agree, but you've just written that
>>>>
>>>>>>> We want to fall through to line 288 if PAGE_OFFSET > %sp and that's
>>>>>>> what we do.
>>>>
>>>> and line 288 is trap_setup_user_stack_is_bolixed branch.
>>>
>>> Sorry, logic reversed.
>>>
>>
>> So, is it a bug in etrap.S?
>
> Seems that way, and it's harmless because the real fault handler will
> find that things are actually fine and it all works out, albeit
> slowly.
>
> Feel free to test the obvious fix :)
>

Ok, thanks. =)

-- 
Best regards,
Eldar Sh. Abusalimov