[LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Cyril Hrubis]

While integer division by zero does trap on x86_64 and causes the SIGFPE
signal to be delivered it's not the case on all architecutes. At least
on ARM and PPC64LE division by zero simply returns undefined result
instead.

This patch adds raise(SIGFPE) at the end of the child as a fallback to
make sure the process is killed with the right signal on all
architectures.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
index 869ef18bd..8c351d120 100644
--- a/testcases/kernel/syscalls/waitid/waitid10.c
+++ b/testcases/kernel/syscalls/waitid/waitid10.c
@@ -28,7 +28,10 @@ static void run(void)
 		volatile int a, zero = 0;
 
 		a = 1 / zero;
-		exit(a);
+
+		tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");
+
+		raise(SIGFPE);
 	}
 
 	TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED));
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Cyril Hrubis]

Hi!
> While integer division by zero does trap on x86_64 and causes the SIGFPE
> signal to be delivered it's not the case on all architecutes. At least
> on ARM and PPC64LE division by zero simply returns undefined result
> instead.
> 
> This patch adds raise(SIGFPE) at the end of the child as a fallback to
> make sure the process is killed with the right signal on all
> architectures.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
> index 869ef18bd..8c351d120 100644
> --- a/testcases/kernel/syscalls/waitid/waitid10.c
> +++ b/testcases/kernel/syscalls/waitid/waitid10.c
> @@ -28,7 +28,10 @@ static void run(void)
>  		volatile int a, zero = 0;
>  
>  		a = 1 / zero;
> -		exit(a);
> +
> +		tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");

This patch inroduces 'set but not used' warning for the a variable so
maybe the message should look like:

		tst_res(TINFO, "1/0 = %i raising SIGFPE", a);

> +		raise(SIGFPE);
>  	}
>  
>  	TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED));
> -- 
> 2.34.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> While integer division by zero does trap on x86_64 and causes the SIGFPE
>> signal to be delivered it's not the case on all architecutes. At least
>> on ARM and PPC64LE division by zero simply returns undefined result
>> instead.

Nit picking: even with this patch we are still testing undefined
behaviour.

   There are six signals that can be delivered as a consequence of a
   hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and
   SIGTRAP.  Which of these signals is delivered, for any given
   hard- ware exception, is not documented and does not always make
   sense.

If dividing by zero produces SIGEMT then it's still valid according to
the specification. FPE does stand for floating point exception, but we
are dividing integers.

>> 
>> This patch adds raise(SIGFPE) at the end of the child as a fallback to
>> make sure the process is killed with the right signal on all
>> architectures.
>> 
>> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
>> ---
>>  testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
>> index 869ef18bd..8c351d120 100644
>> --- a/testcases/kernel/syscalls/waitid/waitid10.c
>> +++ b/testcases/kernel/syscalls/waitid/waitid10.c
>> @@ -28,7 +28,10 @@ static void run(void)
>>  		volatile int a, zero = 0;
>>  
>>  		a = 1 / zero;
>> -		exit(a);
>> +
>> +		tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");
>
> This patch inroduces 'set but not used' warning for the a variable so
> maybe the message should look like:
>
> 		tst_res(TINFO, "1/0 = %i raising SIGFPE", a);
>
>> +		raise(SIGFPE);

I'm wondering if we should branch on the architecture. If it's x86[_64]
then we only do divide by zero as it's reasonable to think that if the
signal is not raised then this is a bug.

>>  	}
>>  
>>  	TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED));
>> -- 
>> 2.34.1
>> 
>> 
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>
> -- 
> Cyril Hrubis
> chrubis@suse.cz


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Cyril Hrubis]

Hi!
> >> While integer division by zero does trap on x86_64 and causes the SIGFPE
> >> signal to be delivered it's not the case on all architecutes. At least
> >> on ARM and PPC64LE division by zero simply returns undefined result
> >> instead.
> 
> Nit picking: even with this patch we are still testing undefined
> behaviour.
> 
>    There are six signals that can be delivered as a consequence of a
>    hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and
>    SIGTRAP.  Which of these signals is delivered, for any given
>    hard- ware exception, is not documented and does not always make
>    sense.
> 
> If dividing by zero produces SIGEMT then it's still valid according to
> the specification. FPE does stand for floating point exception, but we
> are dividing integers.

Actually as far as I can tell the POSIX says that for integer division
by zero you shall get SIGFPE (and si_code in siginfo se tto FPE_INTDIV)
if the operation traps. It seems to be pretty well defined:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html

> >> 
> >> This patch adds raise(SIGFPE) at the end of the child as a fallback to
> >> make sure the process is killed with the right signal on all
> >> architectures.
> >> 
> >> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> >> ---
> >>  testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
> >> index 869ef18bd..8c351d120 100644
> >> --- a/testcases/kernel/syscalls/waitid/waitid10.c
> >> +++ b/testcases/kernel/syscalls/waitid/waitid10.c
> >> @@ -28,7 +28,10 @@ static void run(void)
> >>  		volatile int a, zero = 0;
> >>  
> >>  		a = 1 / zero;
> >> -		exit(a);
> >> +
> >> +		tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");
> >
> > This patch inroduces 'set but not used' warning for the a variable so
> > maybe the message should look like:
> >
> > 		tst_res(TINFO, "1/0 = %i raising SIGFPE", a);
> >
> >> +		raise(SIGFPE);
> 
> I'm wondering if we should branch on the architecture. If it's x86[_64]
> then we only do divide by zero as it's reasonable to think that if the
> signal is not raised then this is a bug.

That would work too I guess.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> >> While integer division by zero does trap on x86_64 and causes the SIGFPE
>> >> signal to be delivered it's not the case on all architecutes. At least
>> >> on ARM and PPC64LE division by zero simply returns undefined result
>> >> instead.
>> 
>> Nit picking: even with this patch we are still testing undefined
>> behaviour.
>> 
>>    There are six signals that can be delivered as a consequence of a
>>    hardware exception: SIGBUS, SIGEMT, SIGFPE, SIGILL, SIGSEGV, and
>>    SIGTRAP.  Which of these signals is delivered, for any given
>>    hard- ware exception, is not documented and does not always make
>>    sense.
>> 
>> If dividing by zero produces SIGEMT then it's still valid according to
>> the specification. FPE does stand for floating point exception, but we
>> are dividing integers.
>
> Actually as far as I can tell the POSIX says that for integer division
> by zero you shall get SIGFPE (and si_code in siginfo se tto FPE_INTDIV)
> if the operation traps. It seems to be pretty well defined:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html
>
>> >> 
>> >> This patch adds raise(SIGFPE) at the end of the child as a fallback to
>> >> make sure the process is killed with the right signal on all
>> >> architectures.
>> >> 
>> >> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
>> >> ---
>> >>  testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
>> >> index 869ef18bd..8c351d120 100644
>> >> --- a/testcases/kernel/syscalls/waitid/waitid10.c
>> >> +++ b/testcases/kernel/syscalls/waitid/waitid10.c
>> >> @@ -28,7 +28,10 @@ static void run(void)
>> >>  		volatile int a, zero = 0;
>> >>  
>> >>  		a = 1 / zero;
>> >> -		exit(a);
>> >> +
>> >> +		tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");
>> >
>> > This patch inroduces 'set but not used' warning for the a variable so
>> > maybe the message should look like:
>> >
>> > 		tst_res(TINFO, "1/0 = %i raising SIGFPE", a);
>> >
>> >> +		raise(SIGFPE);
>> 
>> I'm wondering if we should branch on the architecture. If it's x86[_64]
>> then we only do divide by zero as it's reasonable to think that if the
>> signal is not raised then this is a bug.
>
> That would work too I guess.

I would still use #ifdef to remove raise(SIGFPE) on x86. I think this
makes it a more solid test on x86. As it is defined in the spec I guess
you could do the divide by zero on other arches and we can review the
results to see if any also raise SIGFPE.

With that:
Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Martin Doucha]

On 21. 03. 22 16:48, Richard Palethorpe wrote:
> I'm wondering if we should branch on the architecture. If it's x86[_64]
> then we only do divide by zero as it's reasonable to think that if the
> signal is not raised then this is a bug.

It's more likely to be a hardware bug/missing feature though. Do we
really care? I'd argue that removing the division altogether and just
calling raise(SIGFPE) in the child process is all we need in this
particular test.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Richard Palethorpe]

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> On 21. 03. 22 16:48, Richard Palethorpe wrote:
>> I'm wondering if we should branch on the architecture. If it's x86[_64]
>> then we only do divide by zero as it's reasonable to think that if the
>> signal is not raised then this is a bug.
>
> It's more likely to be a hardware bug/missing feature though. Do we
> really care? I'd argue that removing the division altogether and just
> calling raise(SIGFPE) in the child process is all we need in this
> particular test.

I suppose it depends on if there is a substantial difference in how the
signal is raised between div by zero and raise. I guess there is some
configuration to trap the faulting instruction and raise a
signal.

I don't have a strong opinion as, by definintion, testing undefined
behaviour has uncertain results.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Cyril Hrubis]

Hi!
> >> I'm wondering if we should branch on the architecture. If it's x86[_64]
> >> then we only do divide by zero as it's reasonable to think that if the
> >> signal is not raised then this is a bug.
> >
> > It's more likely to be a hardware bug/missing feature though. Do we
> > really care? I'd argue that removing the division altogether and just
> > calling raise(SIGFPE) in the child process is all we need in this
> > particular test.
> 
> I suppose it depends on if there is a substantial difference in how the
> signal is raised between div by zero and raise. I guess there is some
> configuration to trap the faulting instruction and raise a
> signal.

I guess that in the case of division by zero we end up in the kernel
interrupt handler where the kernel looks up the process that was running
when the interrupt has raised then it queues the signal delivery and so
on.

In the case of raise() we just do sysenter instruction which triggers
different interrupt handler and the rest would be the same we queue the
signal and so on.

Which is why I think that there is some value in triggering the divison
by zero on architectures that enable it by default because we execute
kernel interrupt handler that is rarely being executed.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/waitid10: Fix on ARM, PPC and possibly others

[Jan Stancek]

On Thu, Mar 10, 2022 at 11:53 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> While integer division by zero does trap on x86_64 and causes the SIGFPE
> signal to be delivered it's not the case on all architecutes.

With gcc version 12.0.1 I don't get SIGFPE even on x86:
int main(void)
{
        volatile int a, zero = 0;
        a = 1 / zero;
}

   0x0000000000401020 <+0>:     movl   $0x0,-0x4(%rsp)
   0x0000000000401028 <+8>:     mov    -0x4(%rsp),%eax
   0x000000000040102c <+12>:    lea    0x1(%rax),%edx
   0x000000000040102f <+15>:    cmp    $0x2,%edx
   0x0000000000401032 <+18>:    mov    $0x0,%edx
   0x0000000000401037 <+23>:    cmova  %edx,%eax
   0x000000000040103a <+26>:    mov    %eax,-0x8(%rsp)
   0x000000000040103e <+30>:    xor    %eax,%eax
   0x0000000000401040 <+32>:    ret

It does trigger with a small modification:

@@ -25,9 +25,9 @@ static void run(void)

        pidchild = SAFE_FORK();
        if (!pidchild) {
-               volatile int a, zero = 0;
+               volatile int a = 1, zero = 0;

-               a = 1 / zero;
+               a = a / zero;
                exit(a);
        }


> At least
> on ARM and PPC64LE division by zero simply returns undefined result
> instead.
>
> This patch adds raise(SIGFPE) at the end of the child as a fallback to
> make sure the process is killed with the right signal on all
> architectures.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/syscalls/waitid/waitid10.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/waitid/waitid10.c b/testcases/kernel/syscalls/waitid/waitid10.c
> index 869ef18bd..8c351d120 100644
> --- a/testcases/kernel/syscalls/waitid/waitid10.c
> +++ b/testcases/kernel/syscalls/waitid/waitid10.c
> @@ -28,7 +28,10 @@ static void run(void)
>                 volatile int a, zero = 0;
>
>                 a = 1 / zero;
> -               exit(a);
> +
> +               tst_res(TINFO, "Division by zero didn't trap, raising SIGFPE");
> +
> +               raise(SIGFPE);

I agree with Petr, that raise(SIGFPE) would be sufficient for this test.

>         }
>
>         TST_EXP_PASS(waitid(P_ALL, 0, infop, WEXITED));
> --
> 2.34.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp