Closed Bug 1687164 Opened 5 years ago Closed 4 years ago

freebl: Unconditional use of "-mcrypto" and "-mvsx" breaks older POWER systems

Categories

(NSS :: Libraries, defect, P5)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: glaubitz, Assigned: beurdouche)

References

Details

(Whiteboard: [nss-nofx])

Attachments

(1 file, 1 obsolete file)

Bug 1687164 - Introduce NSS_DISABLE_CRYPTO_VSX and disable_crypto_vsx r=bbeurdouche
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

The fix in #1642174 is incomplete since the Makefile still unconditionally passes "-mcrypto" and "-mvsx" during build which results in freebl being compiled with instructions incompatible with older POWER systems as reported in [1].

[1] https://lists.debian.org/debian-powerpc/2021/01/msg00037.html

I added -mvsx, because I was informed that nss actually can't make use of altivec and it can only use vsx, which NSS_DISABLE_ALTIVEC=1 can disable.

Is this not true?

(In reply to Piotr Kubaj from comment #1)

I added -mvsx, because I was informed that nss actually can't make use of altivec and it can only use vsx, which NSS_DISABLE_ALTIVEC=1 can disable.

Is this not true?

I would suggest then to rename the configure flag to NSS_DISABLE_VSX then and only enable it when LITTLE_ENDIAN is defined.

See the output of "echo | gcc -E -dM -"

Hi Piotr, Hi John,
Unfortunately we don't have a machine to test this, but we would welcome a patch if you have a good solution... : )
B.

Severity: -- → S4
Priority: -- → P5
Whiteboard: [nss-nofx]
Version: 3.60 → trunk

The GCC compile farm has big-endian PowerPC machines available running Debian unstable, see: https://gcc.gnu.org/wiki/CompileFarm Anyone can request access to these machines.

I maintain the machine gcc203, so I can install additional packages if necessary. But I can also try to provide a patch the next days if I find the time. Shouldn't be too difficult.

Here is an example of a patch that I use to still run Thunderbird and Firefox on my G5:

--- ./nss/lib/freebl/Makefile.orig 2020-12-11 16:32:40.000000000 +0100
+++ ./nss/lib/freebl/Makefile 2021-01-15 23:04:21.000000000 +0100
@@ -757,10 +757,10 @@

ifeq ($(CPU_ARCH),ppc)
ifndef NSS_DISABLE_ALTIVEC
-$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec -mvsx
-$(OBJDIR)/$(PROG_PREFIX)gcm$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec -mvsx
-$(OBJDIR)/$(PROG_PREFIX)rijndael$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec -mvsx
-$(OBJDIR)/$(PROG_PREFIX)sha512$(OBJ_SUFFIX): CFLAGS += -mcrypto -maltivec -mvsx
+$(OBJDIR)/$(PROG_PREFIX)gcm-ppc$(OBJ_SUFFIX): CFLAGS += -maltivec
+$(OBJDIR)/$(PROG_PREFIX)gcm$(OBJ_SUFFIX): CFLAGS += -maltivec
+$(OBJDIR)/$(PROG_PREFIX)rijndael$(OBJ_SUFFIX): CFLAGS += -maltivec
+$(OBJDIR)/$(PROG_PREFIX)sha512$(OBJ_SUFFIX): CFLAGS += -maltivec
-funroll-loops -fpeel-loops
endif
endif

And there should indeed be a distinction between ALTIVEC, VSX and CRYPTO as those features were introduced with different levels of the Power-ISA (Altivec is already available for good old G4/G5 Power-Macs whereas for VSX you'll need Power-7 and CRYPTO requires an even higher level (Power-8)).

It shouldn't be too difficult to fix this.

Basically, we want to split AltiVec off from the Crypto/VSX stuff because the latter is available on POWER8 and newer only.

I would suggest to have something like disable_vsx or disable_crypto which are set on target_arch="ppc64" but not on target_arch="ppc64le".

While VSX and the Crypto extensions can be used on big-endian ppc64 as well, the majority of users with a big-endian target will be on an older baseline which doesn't support these.

I can create a patch once I have figured out how the build system works. I haven't dealt with GYP before.

Hi John(In reply to John Paul Adrian Glaubitz from comment #6)

It shouldn't be too difficult to fix this.

Basically, we want to split AltiVec off from the Crypto/VSX stuff because the latter is available on POWER8 and newer only.

I would suggest to have something like disable_vsx or disable_crypto which are set on target_arch="ppc64" but not on target_arch="ppc64le".

While VSX and the Crypto extensions can be used on big-endian ppc64 as well, the majority of users with a big-endian target will be on an older baseline which doesn't support these.

I can create a patch once I have figured out how the build system works. I haven't dealt with GYP before.

Hi John,

for gyp you can take a look at my patch where I’ve added NSS_DISABLE_ALTIVEC:
https://hg.mozilla.org/projects/nss/rev/f2d9478178505d24da34f2e694bdd05e0e6395c6

Thanks, I will be sending a PR shortly once I remember how the review system works ;-).

Btw, it seems your change enables Altivec for 64-bit PowerPC only but 32-bit PowerPC targets support Altivec as well - such as PowerMac G4.

(In reply to John Paul Adrian Glaubitz from comment #8)

Thanks, I will be sending a PR shortly once I remember how the review system works ;-).

Btw, it seems your change enables Altivec for 64-bit PowerPC only but 32-bit PowerPC targets support Altivec as well - such as PowerMac G4.

Yes, I had to handle every case for every ppc extension but that was enough at the time :-)
Look forward for you patch, with Buildroot then every case should be built so if any problem exists it comes out.
Thank you!

Gotcha. I'm currently build-testing (powerpc, ppc64, ppc64le) everything to make sure I don't break anything.

(In reply to Giulio Benetti from comment #9)

Look forward for you patch, with Buildroot then every case should be built so if any problem exists it comes out.

One question: Do you know whether any config defines such as NSS_DISABLE_ALTIVEC are automatically translated into Gyp defines such as "disable_altivec"?

(In reply to John Paul Adrian Glaubitz from comment #11)

(In reply to Giulio Benetti from comment #9)

Look forward for you patch, with Buildroot then every case should be built so if any problem exists it comes out.

One question: Do you know whether any config defines such as NSS_DISABLE_ALTIVEC are automatically translated into Gyp defines such as "disable_altivec"?

No it's automatic, you have to take care of lib/freebl/freebl.gyp. There you check gyp variables and then define NSS_DISABLE_ALTIVEC or something
like that. Try to imitate what I've done in this patch lines:
https://hg.mozilla.org/projects/nss/rev/f2d9478178505d24da34f2e694bdd05e0e6395c6#l4.12

Currently, NSS assumes that every PowerPC target supports the crypto
and VSX extensions of the PowerPC ABI. However, VSX was only introduced
with ISA version 2.06 and the crypto extensions with ISA version 2.07
and enabling them on older PowerPC targets will result in a SIGILL. Thus,
make their use configurable and enable them by default on ppc64le only.

Just submitted it. To my surprise, neither jcj nor kjacobs were available as reviewers, so I picked bbeurdouche. Let me know if I shall pick someone else.

I have verified the changes on ppc64 and ppc64le. I can verify them on 32-bit PowerPC tomorrow as it's late here already.

(In reply to John Paul Adrian Glaubitz from comment #14)

Just submitted it. To my surprise, neither jcj nor kjacobs were available as reviewers, so I picked bbeurdouche. Let me know if I shall pick someone else.

I have verified the changes on ppc64 and ppc64le. I can verify them on 32-bit PowerPC tomorrow as it's late here already.

I’ve taken a look at the patch, is there any ppc that has crypto but not VSX?
If yes then I would introduce a variable for VSX too.
Also the reviewer I think you should leave it undefined and then Mozilla crew set it, at least I’ve done that way every time.

(In reply to John Paul Adrian Glaubitz from comment #14)

Just submitted it. To my surprise, neither jcj nor kjacobs were available as reviewers, so I picked bbeurdouche. Let me know if I shall pick someone else.

I have verified the changes on ppc64 and ppc64le. I can verify them on 32-bit PowerPC tomorrow as it's late here already.

Hi all, thanks for submitting the patch,
You were correct to pick me as the reviewer. JC and Kevin left for new adventures which leaves only me on the Mozilla side.

As Giulio noted, it is better not to assign a reviewer yourself, but that's fine in this case.
I've pushed your patch to our CI and it seem to be failing right now. (I've put the link in the test plan of the phabricator patch.)
Feel free to needinfo me here, when you have updated the patch and you think it is ready for review... : )

Flags: needinfo?(glaubitz)
Assignee: nobody → bbeurdouche
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Thanks Benjamin for the helpful input. I'm calling it a day for today and will get back to this tomorrow.

Flags: needinfo?(glaubitz)

(In reply to Giulio Benetti from comment #15)

I’ve taken a look at the patch, is there any ppc that has crypto but not VSX?

Yes, there are although those systems are not commonly used as a baseline.

See: https://en.wikipedia.org/wiki/Power_ISA

VSX was introduced with ISA version 2.06, the crypto instructions with ISA version 2.07.

If yes then I would introduce a variable for VSX too.

I first wanted to avoid that because I thought it would become too complex. But I think I have a rough idea now on how to construct the CFLAGS and will hopefully push a much better patch the next days.

Also the reviewer I think you should leave it undefined and then Mozilla crew set it, at least I’ve done that way every time.

Seems that Benjamin is fine with being picked as the reviewer.

(In reply to John Paul Adrian Glaubitz from comment #18)

(In reply to Giulio Benetti from comment #15)

I’ve taken a look at the patch, is there any ppc that has crypto but not VSX?

Yes, there are although those systems are not commonly used as a baseline.

See: https://en.wikipedia.org/wiki/Power_ISA

VSX was introduced with ISA version 2.06, the crypto instructions with ISA version 2.07.

IMHO this is worth having 2 patches, 1 for disabling VSX and 1 for disabling CRYPTO.

If yes then I would introduce a variable for VSX too.

I first wanted to avoid that because I thought it would become too complex. But I think I have a rough idea now on how to construct the CFLAGS and will hopefully push a much better patch the next days.

Also the reviewer I think you should leave it undefined and then Mozilla crew set it, at least I’ve done that way every time.

Seems that Benjamin is fine with being picked as the reviewer.

I think I won't make two separate patches, but we'll see. I'm busy this week but I will be on vacation from next week, so plenty of time to work on this :-).

There is even more breakage now with the recent changes:

[566/1215] SOLINK /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so
FAILED: /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC
if [ ! -e /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -o ! -e /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC ]; then cc -shared -Wl,-Bsymbolic -Wl,--version-script,obj/lib/freebl/freeblpriv3.gen/out.freebl_hash_vector.def -Wl,--gc-sections -Wl,-z,defs -z noexecstack -o /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -Wl,-soname=libfreeblpriv3.so @/home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.rsp && { readelf -d /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | grep SONAME ; nm -gD -f p /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | cut -f1-2 -d' '; } > /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC; else cc -shared -Wl,-Bsymbolic -Wl,--version-script,obj/lib/freebl/freeblpriv3.gen/out.freebl_hash_vector.def -Wl,--gc-sections -Wl,-z,defs -z noexecstack -o /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -Wl,-soname=libfreeblpriv3.so @/home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.rsp && { readelf -d /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | grep SONAME ; nm -gD -f p /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | cut -f1-2 -d' '; } > /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp && if ! cmp -s /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC; then mv /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC ; fi; fi
/usr/bin/ld: libgcm-aes-ppc_c_lib.a(gcm-aes-ppc_c_lib.sha512-p8.o): ABI version 2 is not compatible with ABI version 1 output
/usr/bin/ld: failed to merge target specific data of file libgcm-aes-ppc_c_lib.a(gcm-aes-ppc_c_lib.sha512-p8.o)
collect2: error: ld returned 1 exit status
[583/1215] CC obj/lib/sqlite/sqlite.sqlite3.o
ninja: build stopped: subcommand failed.[566/1215] SOLINK /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so
FAILED: /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC
if [ ! -e /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -o ! -e /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC ]; then cc -shared -Wl,-Bsymbolic -Wl,--version-script,obj/lib/freebl/freeblpriv3.gen/out.freebl_hash_vector.def -Wl,--gc-sections -Wl,-z,defs -z noexecstack -o /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -Wl,-soname=libfreeblpriv3.so @/home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.rsp && { readelf -d /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | grep SONAME ; nm -gD -f p /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | cut -f1-2 -d' '; } > /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC; else cc -shared -Wl,-Bsymbolic -Wl,--version-script,obj/lib/freebl/freeblpriv3.gen/out.freebl_hash_vector.def -Wl,--gc-sections -Wl,-z,defs -z noexecstack -o /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so -Wl,-soname=libfreeblpriv3.so @/home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.rsp && { readelf -d /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | grep SONAME ; nm -gD -f p /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so | cut -f1-2 -d' '; } > /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp && if ! cmp -s /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC; then mv /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.tmp /home/glaubitz/dist/Debug/lib/libfreeblpriv3.so.TOC ; fi; fi
/usr/bin/ld: libgcm-aes-ppc_c_lib.a(gcm-aes-ppc_c_lib.sha512-p8.o): ABI version 2 is not compatible with ABI version 1 output
/usr/bin/ld: failed to merge target specific data of file libgcm-aes-ppc_c_lib.a(gcm-aes-ppc_c_lib.sha512-p8.o)
collect2: error: ld returned 1 exit status
[583/1215] CC obj/lib/sqlite/sqlite.sqlite3.o
ninja: build stopped: subcommand failed.

Frustrating.

(In reply to John Paul Adrian Glaubitz from comment #21)

There is even more breakage now with the recent changes:
(...)
Frustrating.

OK, I managed to get it building again. Will try to push an updated revision of the patch later today.

Currently, NSS assumes that every PowerPC target supports the crypto
and VSX extensions of the PowerPC ABI. However, VSX was only introduced
with ISA version 2.06 and the crypto extensions with ISA version 2.07
and enabling them on older PowerPC targets will result in a SIGILL. Thus,
make their use configurable and enable them by default on ppc64le only.

Attachment #9213088 - Attachment is obsolete: true

(In reply to John Paul Adrian Glaubitz from comment #22)

(In reply to John Paul Adrian Glaubitz from comment #21)

There is even more breakage now with the recent changes:
(...)
Frustrating.

OK, I managed to get it building again. Will try to push an updated revision of the patch later today.

Hi John,

this patch looks good, but it should add 2 different NSS_DISABLE'S_
NSS_DISABLE_CRYPTO and NSS_DISABLE_VSX, since as you've written there are ISA 2.06 where only VSX is present and
ISA 2.07 where also CRYPTO is present.

I'm adding those 2 options to Buildroot:
BR2_POWERPC_CPU_HAS_VSX
BR2_POWERPC_CPU_HAS_CRYPTO

But I need 2 different patches with such macros to disable them.
Basically this unique patch is partly wrong based. Since you're doing a great job, let's do it until the end!
Otherwise I can take over it, no problem.

Thank you very much!

(In reply to Giulio Benetti from comment #24)

this patch looks good, but it should add 2 different NSS_DISABLE'S_
NSS_DISABLE_CRYPTO and NSS_DISABLE_VSX, since as you've written there are ISA 2.06 where only VSX is present and
ISA 2.07 where also CRYPTO is present.

I'm adding those 2 options to Buildroot:
BR2_POWERPC_CPU_HAS_VSX
BR2_POWERPC_CPU_HAS_CRYPTO

But I need 2 different patches with such macros to disable them.
Basically this unique patch is partly wrong based. Since you're doing a great job, let's do it until the end!
Otherwise I can take over it, no problem.

Well, I don't think anything without VSX and with CRYPTO exists in the wild. So I don't see a reason to separate those.

I mean, currently have all under the Altivec conditional, so that's definitely an improvement.

Users have either POWER8 or greater or POWER5 on ppc64, the latter with or without Altivec.

(In reply to John Paul Adrian Glaubitz from comment #25)

(In reply to Giulio Benetti from comment #24)

this patch looks good, but it should add 2 different NSS_DISABLE'S_
NSS_DISABLE_CRYPTO and NSS_DISABLE_VSX, since as you've written there are ISA 2.06 where only VSX is present and
ISA 2.07 where also CRYPTO is present.

I'm adding those 2 options to Buildroot:
BR2_POWERPC_CPU_HAS_VSX
BR2_POWERPC_CPU_HAS_CRYPTO

But I need 2 different patches with such macros to disable them.
Basically this unique patch is partly wrong based. Since you're doing a great job, let's do it until the end!
Otherwise I can take over it, no problem.

Well, I don't think anything without VSX and with CRYPTO exists in the wild. So I don't see a reason to separate those.

I mean, currently have all under the Altivec conditional, so that's definitely an improvement.

Users have either POWER8 or greater or POWER5 on ppc64, the latter with or without Altivec.

Under Buildroot we still have POWER4/5/6/7 and 4/5/6 has not the VSX nor CRYPTO, while POWER7 has VSX only.
And we also have e500-mc and e5500 that are in the same state, this is why. You can check it here:
https://git.buildroot.net/buildroot/tree/arch/Config.in.powerpc
And they are part of the autobuilders. At this point I think it would make sense to complete this using the 2 DISABLES_VSX/CRYPTO.

(In reply to Giulio Benetti from comment #26)

Under Buildroot we still have POWER4/5/6/7 and 4/5/6 has not the VSX nor CRYPTO, while POWER7 has VSX only.
And we also have e500-mc and e5500 that are in the same state, this is why. You can check it here:
https://git.buildroot.net/buildroot/tree/arch/Config.in.powerpc
And they are part of the autobuilders. At this point I think it would make sense to complete this using the 2 DISABLES_VSX/CRYPTO.

The code already uses NSS_DISABLE_CRYPTO_VSX:

https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/chacha20poly1305.c#l222

I see no reason to change that at the moment as POWER5 with or without Altivec and POWER8 or greater (which supports all) covers 99% of all usecases.

If someone reports they are actually using a combination where VSX is enabled but CRYPTO not or vice versa, we can still change that. For now, I just want to unbreak nss on Debian/ppc64 because users have reported that Firefox currently crashes with SIGILL for them because of this bug.

(In reply to John Paul Adrian Glaubitz from comment #27)

(In reply to Giulio Benetti from comment #26)

Under Buildroot we still have POWER4/5/6/7 and 4/5/6 has not the VSX nor CRYPTO, while POWER7 has VSX only.
And we also have e500-mc and e5500 that are in the same state, this is why. You can check it here:
https://git.buildroot.net/buildroot/tree/arch/Config.in.powerpc
And they are part of the autobuilders. At this point I think it would make sense to complete this using the 2 DISABLES_VSX/CRYPTO.

The code already uses NSS_DISABLE_CRYPTO_VSX:

https://hg.mozilla.org/projects/nss/file/tip/lib/freebl/chacha20poly1305.c#l222

I see no reason to change that at the moment as POWER5 with or without Altivec and POWER8 or greater (which supports all) covers 99% of all usecases.

If someone reports they are actually using a combination where VSX is enabled but CRYPTO not or vice versa, we can still change that. For now, I just want to unbreak nss on Debian/ppc64 because users have reported that Firefox currently crashes with SIGILL for them because of this bug.

Ok I see, if I'll find that combination I'll submit another patch then. Thank you very much!

https://hg.mozilla.org/projects/nss/rev/a66c71152314e591106680e6de618c3c8d044373

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Thanks @glaubitz !! : )

Duplicate of this bug: 1821347
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: