On Sunday, October 15, 2023 12:37 PM, Beat Bolli wrote: >On 13.10.23 23:05, brian m. carlson wrote: >> On 2023-10-12 at 20:04:47, Feiyang Xue wrote: >>> Looking for comments on if this is a good idea to add SHA-NI option >>> to git, And if so, what'd be a good implementation. >>> >>> The attached patch is more of a proof of concept, using a sha-ni >>> example found on internet and have it tapped into git's "hash-ll.h" >>> when it sees make flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for >sha1/sha256 respectively. >>> >>> "git hash-object" is about 3-ish times faster for me >> >> You almost certainly don't want to use SHA-1 using native instructions >> because it doesn't detect collisions, unlike the default >> implementation (SHA-1-DC). Since SHA-1 collisions are relatively >> cheap (less than USD >> 45,000) to make, not detecting collisions would be a security issue >> worthy of a CVE. >> >> It's fine for SHA-256, but see below. >> >>> There are few topics that i'm uncertain about. >>> >>> 1. Is it good idea to have machine-specific asm codes in git. I read >>> there was commit fd1ec82547 which removed assembly implementation for >>> PPC_SHA1. Does that imply maintainers doesn't want machine-specific assembly >in source? >> >> I'd prefer we didn't. >> >> Nettle has SHA-NI extensions on systems that support it, and so does >> OpenSSL. OpenSSL can't be used by distros for licensing reasons, but >> Nettle can, and both of those libraries automatically detect the best >> code to use based on the chip. One of those libraries is almost >> always going to be linked into Git at some point because of libcurl anyway. >> >> If we ship the code, then someone has to maintain it, and I don't know >> if we have any assembly experts. We might also have a bug that >> produced incorrect results, which would be pretty disastrous for the >> affected users. It's much better for maintainability if we push that >> off onto the cryptographic library maintainers who are much more >> competent in that regard than I am (I will not speak for others) and >> let distro maintainers simply link the appropriate library, which, as >> I said above, they're already effectively doing. > >In addition to all the valid points brian makes, the patch uses a non-standard >assembler (yasm, see http://yasm.tortall.net/) that's not part of the default GNU >toolchain. Aside from that, not everyone uses the GNU toolchain. I would suggest that if someone needs ASM support for SHA-1, they could reimplement SHA-1-DC using rotate ASM primitives instead of directly using SHA1 assembly, but I question whether that is significantly faster than having the compiler optimizer generate the equivalent rotates. Still, maintaining any ASM code requires out of box skillsets that are unlikely sustainable. --Randall
