2017-12-12 22:49:42

by Sudip Mukherjee

[permalink] [raw]
Subject: [PATCH v2] arch: define weak abort

gcc toggle -fisolate-erroneous-paths-dereference (default at -O2
onwards) isolates faulty code paths such as null pointer access, divide
by zero etc. If gcc port doesnt implement __builtin_trap, an abort() is
generated which causes kernel link error.

In this case, gcc is generating abort due to 'divide by zero' in
lib/mpi/mpih-div.c.

Currently 'frv' and 'arc' are failing. Previously other arch was also
broken like m32r was fixed by d22e3d69ee1a ("m32r: fix build failure").

Lets define this weak function which is common for all arch and fix the
problem permanently. We can even remove the arch specific 'abort' after
this is done.

Cc: Alexey Brodkin <[email protected]>
Signed-off-by: Sudip Mukherjee <[email protected]>
---

Hi Alexey,
I was thinking of sending the m32r revert patch in few days. My m32r
builds are having a little problem and should be fixed by this weekend.
I can not test m32r before that. We can also send a patch to remove the
same code in arm and unicore32.

kernel/exit.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/kernel/exit.c b/kernel/exit.c
index af6c245..90c6869 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1759,3 +1759,11 @@ long kernel_wait4(pid_t upid, int __user *stat_addr, int options,
return -EFAULT;
}
#endif
+
+__weak void abort(void)
+{
+ BUG();
+
+ /* if that doesn't kill us, halt */
+ panic("Oops failed to kill thread");
+}
--
1.9.1


2017-12-13 21:47:20

by Alexey Brodkin

[permalink] [raw]
Subject: Re: [PATCH v2] arch: define weak abort

Hi Sudip,

On Tue, 2017-12-12 at 22:49 +0000, Sudip Mukherjee wrote:
> gcc toggle -fisolate-erroneous-paths-dereference (default at -O2
> onwards) isolates faulty code paths such as null pointer access, divide
> by zero etc. If gcc port doesnt implement __builtin_trap, an abort() is
> generated which causes kernel link error.
>
> In this case, gcc is generating abort due to 'divide by zero' in
> lib/mpi/mpih-div.c.
>
> Currently 'frv' and 'arc' are failing. Previously other arch was also
> broken like m32r was fixed by d22e3d69ee1a ("m32r: fix build failure").
>
> Lets define this weak function which is common for all arch and fix the
> problem permanently. We can even remove the arch specific 'abort' after
> this is done.
>
> Cc: Alexey Brodkin <[email protected]>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> Hi Alexey,
> I was thinking of sending the m32r revert patch in few days. My m32r
> builds are having a little problem and should be fixed by this weekend.
> I can not test m32r before that.

Understood, that's fine by me.

> We can also send a patch to remove the
> same code in arm and unicore32.

Sure that would be really great!

-Alexey

2017-12-19 21:39:39

by Vineet Gupta

[permalink] [raw]
Subject: Re: [PATCH v2] arch: define weak abort

On 12/12/2017 02:49 PM, Sudip Mukherjee wrote:
> gcc toggle -fisolate-erroneous-paths-dereference (default at -O2
> onwards) isolates faulty code paths such as null pointer access, divide
> by zero etc. If gcc port doesnt implement __builtin_trap, an abort() is
> generated which causes kernel link error.
>
> In this case, gcc is generating abort due to 'divide by zero' in
> lib/mpi/mpih-div.c.
>
> Currently 'frv' and 'arc' are failing. Previously other arch was also
> broken like m32r was fixed by d22e3d69ee1a ("m32r: fix build failure").
>
> Lets define this weak function which is common for all arch and fix the
> problem permanently. We can even remove the arch specific 'abort' after
> this is done.
>
> Cc: Alexey Brodkin <[email protected]>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> Hi Alexey,
> I was thinking of sending the m32r revert patch in few days. My m32r
> builds are having a little problem and should be fixed by this weekend.
> I can not test m32r before that. We can also send a patch to remove the
> same code in arm and unicore32.
>
> kernel/exit.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index af6c245..90c6869 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1759,3 +1759,11 @@ long kernel_wait4(pid_t upid, int __user *stat_addr, int options,
> return -EFAULT;
> }
> #endif
> +
> +__weak void abort(void)
> +{
> + BUG();
> +
> + /* if that doesn't kill us, halt */
> + panic("Oops failed to kill thread");
> +}

Hmm, I realize there's a small gotcha with this, when testing with a different
patch from Arnd. This needs an EXPORT_SYMBOL() as well !

-Vineet

2017-12-19 21:56:04

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v2] arch: define weak abort

On Tue, 19 Dec 2017 13:39:22 -0800 Vineet Gupta <[email protected]> wrote:

> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -1759,3 +1759,11 @@ long kernel_wait4(pid_t upid, int __user *stat_addr, int options,
> > return -EFAULT;
> > }
> > #endif
> > +
> > +__weak void abort(void)
> > +{
> > + BUG();
> > +
> > + /* if that doesn't kill us, halt */
> > + panic("Oops failed to kill thread");
> > +}
>
> Hmm, I realize there's a small gotcha with this, when testing with a different
> patch from Arnd. This needs an EXPORT_SYMBOL() as well !

Yup. This?

From: Andrew Morton <[email protected]>
Subject: kernel/exit.c: export abort() to modules

gcc -fisolate-erroneous-paths-dereference can generate calls to abort()
from modular code too.

Reported-by: Vineet Gupta <[email protected]>
Cc: Sudip Mukherjee <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: "Alexey Brodkin" <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

kernel/exit.c | 1 +
1 file changed, 1 insertion(+)

diff -puN kernel/exit.c~a kernel/exit.c
--- a/kernel/exit.c~a
+++ a/kernel/exit.c
@@ -1763,3 +1763,4 @@ __weak void abort(void)
/* if that doesn't kill us, halt */
panic("Oops failed to kill thread");
}
+EXPORT_SYMBOL(abort);
_

2017-12-21 23:20:43

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH v2] arch: define weak abort

On Tue, Dec 19, 2017 at 01:55:59PM -0800, Andrew Morton wrote:
> On Tue, 19 Dec 2017 13:39:22 -0800 Vineet Gupta <[email protected]> wrote:
>

<snip>

> > Hmm, I realize there's a small gotcha with this, when testing with a different
> > patch from Arnd. This needs an EXPORT_SYMBOL() as well !
>
> Yup. This?

Yes, just faced this when I was testing with the reverted patch on m32r.

>
> From: Andrew Morton <[email protected]>
> Subject: kernel/exit.c: export abort() to modules
>
> gcc -fisolate-erroneous-paths-dereference can generate calls to abort()
> from modular code too.
>
> Reported-by: Vineet Gupta <[email protected]>
> Cc: Sudip Mukherjee <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: "Alexey Brodkin" <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---

Acked-by: Sudip Mukherjee <[email protected]>

--
Regards
Sudip