Re: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?

[Beat Bolli <dev+git@drbeat.li>]

Hi

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.

Cheers, Beat