Bug 248221 - skein1024 broken
Summary: skein1024 broken
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Ed Maste
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-23 18:53 UTC by Ed Maste
Modified: 2020-10-27 13:17 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Maste freebsd_committer 2020-07-23 18:53:45 UTC
Running skein1024's built in test suite reports incorrect results:

universe13a% skein1024 -x
Skein1024 test suite:
Skein1024 ("") = da8908b01a1bb35bb15d11648c7c61fcc1cc602d93c95bb3b55411200ddca99851bc85513827eb26ca5b13680c10cfb81388f04870d78d7caf8ba93237f97da7ce60de44422ff97c21a24e90524593c007966934e9b4cff1abb8c820d398b01fe7b60481b6f2d36b9491904ec3b4be40c66b61d2a627074ac96a334a798a159e - INCORRECT RESULT!
...

Reported by pizzamig
Comment 1 commit-hook freebsd_committer 2020-07-23 18:56:18 UTC
A commit references this bug:

Author: emaste
Date: Thu Jul 23 18:55:48 UTC 2020
New revision: 363454
URL: https://svnweb.freebsd.org/changeset/base/363454

Log:
  libmd: temporarily disable optimized assembly skein1024 implementation

  It is apparently broken when assembled by contemporary GNU as as well as
  Clang IAS (which is used in the default configuration).

  PR:		248221
  Reported by:	pizzamig
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/lib/libmd/Makefile
Comment 2 Ed Maste freebsd_committer 2020-07-23 19:03:16 UTC
If I assemble skein_block_asm.s with GNU as 2.17.50 then skein1024 -x produces "verified correct".

If I assemble with GNU as 2.33.1 it produces "INCORRECT RESULT!".
Comment 3 Ed Maste freebsd_committer 2020-07-23 19:13:48 UTC
Comparing the disassembly:

$ llvm-objdump -S skein_block_asm.2.17.50.pico | cut -c10- > disass.2.17.50
llvm-objdump: warning: 'skein_block_asm.2.17.50.pico': failed to parse debug information for skein_block_asm.2.17.50.pico
$ llvm-objdump -S skein_block_asm.2.33.1.pico | cut -c10- > disass.2.33.1
llvm-objdump: warning: 'skein_block_asm.2.33.1.pico': failed to parse debug information for skein_block_asm.2.33.1.pico
$ diff -u3 disass.2.33.1 disass.2.17.50

shows:

--- disass.2.33.1       2020-07-23 15:11:19.795112000 -0400
+++ disass.2.17.50      2020-07-23 15:11:11.603490000 -0400
@@ -1,5 +1,5 @@

-ck_asm.2.33.1.pico:    file format ELF64-x86-64
+ck_asm.2.17.50.pico:   file format ELF64-x86-64


 ly of section .text:
@@ -1876,8 +1876,8 @@
  48 89 8d c0 00 00 00          movq    %rcx, 192(%rbp)
  4c 8b 4f 18                   movq    24(%rdi), %r9
  eb 0e                         jmp     14 <Skein1024_block_loop>
- 66 66 2e 0f 1f 84 00 00 00 00 00      nopw    %cs:(%rax,%rax)
- 0f 1f 00                      nopl    (%rax)
+ 0f 1f 80 00 00 00 00          nopl    (%rax)
+ 0f 1f 80 00 00 00 00          nopl    (%rax)

 0001bc0 Skein1024_block_loop:
  4c 8b 47 10                   movq    16(%rdi), %r8
@@ -2083,29 +2083,48 @@
  49 8d 3c 3f                   leaq    (%r15,%rdi), %rdi
  49 c1 c7 05                   rolq    $5, %r15
  49 31 ff                      xorq    %rdi, %r15
+ 48 03 bc 24 a0 00 00 00       addq    160(%rsp), %rdi
+ 4c 03 bc 24 18 01 00 00       addq    280(%rsp), %r15
+ 49 83 c7 01                   addq    $1, %r15
  49 8d 2c 2b                   leaq    (%r11,%rbp), %rbp
  49 c1 c3 14                   rolq    $20, %r11
  49 31 eb                      xorq    %rbp, %r11
+ 48 03 ac 24 b0 00 00 00       addq    176(%rsp), %rbp
+ 4c 03 9c 24 f8 00 00 00       addq    248(%rsp), %r11
  49 8d 4c 0d 00                leaq    (%r13,%rcx), %rcx
  49 c1 c5 30                   rolq    $48, %r13
  49 31 cd                      xorq    %rcx, %r13
+ 48 03 8c 24 d0 00 00 00       addq    208(%rsp), %rcx
+ 4c 03 ac 24 08 01 00 00       addq    264(%rsp), %r13
+ 4c 03 ac 24 88 00 00 00       addq    136(%rsp), %r13
  48 89 4c 24 30                movq    %rcx, 48(%rsp)
  4e 8d 34 36                   leaq    (%rsi,%r14), %r14
  48 c1 c6 2f                   rolq    $47, %rsi
  4c 31 f6                      xorq    %r14, %rsi
+ 4c 03 b4 24 10 01 00 00       addq    272(%rsp), %r14
+ 48 03 b4 24 a8 00 00 00       addq    168(%rsp), %rsi
+ 4c 03 b4 24 90 00 00 00       addq    144(%rsp), %r14
  48 8b 4c 24 20                movq    32(%rsp), %rcx
  4e 8d 04 03                   leaq    (%rbx,%r8), %r8
  48 c1 c3 1c                   rolq    $28, %rbx
  4c 31 c3                      xorq    %r8, %rbx
+ 4c 03 84 24 e0 00 00 00       addq    224(%rsp), %r8
+ 48 03 9c 24 c8 00 00 00       addq    200(%rsp), %rbx
  4e 8d 14 10                   leaq    (%rax,%r10), %r10
  48 c1 c0 10                   rolq    $16, %rax
Comment 4 commit-hook freebsd_committer 2020-07-23 19:20:24 UTC
A commit references this bug:

Author: emaste
Date: Thu Jul 23 19:19:33 UTC 2020
New revision: 363455
URL: https://svnweb.freebsd.org/changeset/base/363455

Log:
  modules/crypto: disable optimized assembly skein1024 implementation

  It is presumably broken in the same way as userland skein1024 (see r363454)

  PR:		248221

Changes:
  head/sys/modules/crypto/Makefile
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2020-07-23 19:45:53 UTC
^Triage: over to committer of emergency workaround.
Comment 7 Jeremy Faulkner 2020-10-02 12:32:25 UTC
gldisater@freebsd-current:/usr/src $ uname -a
FreeBSD freebsd-current 13.0-CURRENT FreeBSD 13.0-CURRENT #3 r366277: Tue Sep 29 18:59:47 EDT 2020     root@freebsd-current:/usr/obj/usr/src/amd64.amd64/sys/GENERIC  amd64

--- lib/libmd__L ---
ld: error: duplicate symbol: Skein1024_Process_Block
>>> defined at skein_block.c:453 (/usr/src/sys/crypto/skein/skein_block.c:453)
>>>            skein_block.pico:(Skein1024_Process_Block)
>>> defined at skein_block_asm.S:1089 (/usr/src/sys/crypto/skein/amd64/skein_block_asm.S:1089)
>>>            skein_block_asm.pico:(.text+0x1B77)

ld: error: duplicate symbol: Skein_256_Process_Block
>>> defined at skein_block.c:60 (/usr/src/sys/crypto/skein/skein_block.c:60)
>>>            skein_block.pico:(Skein_256_Process_Block)
>>> defined at skein_block_asm.S:466 (/usr/src/sys/crypto/skein/amd64/skein_block_asm.S:466)
>>>            skein_block_asm.pico:(.text+0x0)

ld: error: duplicate symbol: Skein_512_Process_Block
>>> defined at skein_block.c:245 (/usr/src/sys/crypto/skein/skein_block.c:245)
>>>            skein_block.pico:(Skein_512_Process_Block)
>>> defined at skein_block_asm.S:795 (/usr/src/sys/crypto/skein/amd64/skein_block_asm.S:795)
>>>            skein_block_asm.pico:(.text+0x8E9)
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** [libmd.so.6.full] Error code 1

make[4]: stopped in /usr/src/lib/libmd
1 error

make[4]: stopped in /usr/src/lib/libmd
Comment 8 Ed Maste freebsd_committer 2020-10-02 13:33:25 UTC
(In reply to Jeremy Faulkner from comment #7)
Can you confirm this is with a no-clean build? kevans and I are discussing the appropriate way to address this but if you're currently stuck a 'make -C lib/libmd clean' should address the issue.
Comment 9 Jeremy Faulkner 2020-10-02 13:52:13 UTC
Yes, this was a NO_CLEAN=. buildworld. I've already started a plain buildworld, it might be done by noon.
Comment 10 commit-hook freebsd_committer 2020-10-02 14:01:26 UTC
A commit references this bug:

Author: emaste
Date: Fri Oct  2 14:00:52 UTC 2020
New revision: 366362
URL: https://svnweb.freebsd.org/changeset/base/366362

Log:
  libmd: add dependency workaround for r366344

  r366344 fixed and reenabled the assembly optimized skein implementation,
  but skein_block objects were not being rebuilt in no-clean builds. This
  resulted in failing no-clean builds. SKEIN_USE_ASM controls which
  routines come from C vs assembly, and with no explicit dependency
  r366344's change to SKEIN_USE_ASM did not cause skein_block.{o,pico}
  to be rebuilt.

  Add a dependency on this Makefile for the skein_block objects. This
  dependency is broader in scope than absolutely required (that is, the
  skein_block objects will now be rebuilt on any change to this Makefile).
  There are ways this could be addressed, but it is probably not worth the
  additional effort or testing time to pursue them.

  PR:		248221
  Reported by:	kevans, Jeremy Faulkner
  Discussed with:	kevans
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/lib/libmd/Makefile
Comment 11 Jeremy Faulkner 2020-10-02 14:38:15 UTC
These commits only re-enabled the libmd use of the skein assembly, when will the crypto kernel module's use be re-enabled?
Comment 12 Ed Maste freebsd_committer 2020-10-02 14:44:03 UTC
(In reply to Jeremy Faulkner from comment #11)
I'll re-enable the kernel side soon, after making sure we address the stale object issue there too.
Comment 13 commit-hook freebsd_committer 2020-10-10 01:13:41 UTC
A commit references this bug:

Author: emaste
Date: Sat Oct 10 01:13:15 UTC 2020
New revision: 366596
URL: https://svnweb.freebsd.org/changeset/base/366596

Log:
  modules/crypto: reenable assembly optimized skein implementation

  r366344 corrected the optimized amd64 skein assembly implementation, so
  we can now enable it again.

  Also add a dependency on this Makefile for the skein_block object, so
  that it will be rebuit (similar to r366362).

  PR:		248221
  Sponsored by:	The FreeBSD Foundation

Changes:
  head/sys/modules/crypto/Makefile