Re: [PATCH v2] block-sha1: remove use of assembly

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 9, 2022, 22:32 UTC
Message-ID: <Yikq7POhuxeN1UPQ@nand.local>
In-Reply-To: <Yikl2eGbc8sPsy5G@camp.crustytoothpaste.net>

On Wed, Mar 09, 2022 at 10:10:33PM +0000, brian m. carlson wrote:

Show 18 quoted lines

> On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Tue, Mar 08 2022, brian m. carlson wrote:
> >
> > I think the $subject of the patch needs updating. It's not removing all
> > the assemply from the file, after this patch we still have the
> > ARM-specific assembly.
> >
> > I don't have a box to test that on, but I wonder if that also triggers
> > the pedantic mode?
> >
> > Perhaps:
> >
> >     block-sha1: remove superfluous i386 and x86-64 assembly
>
> I suspect it has the same problem.  My inclination is to just remove it,
> because my guess is that the compiler has gotten smarter between 2009
> and now.

Almost certainly. I don't have a machine to test it on, either, but I would be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on master today on an arm machine.

Show 7 quoted lines

> I honestly intend to just remove this code in a future version because
> everyone not using SHA1DC has a security problem and we shouldn't offer
> insecure options.
>
> However, I think for now, I'm just going to reroll this with the new
> title and then I can remove it in a future version unless somebody with
> an ARM system can relatively quickly tell me whether it's necessary.

I wonder if a good stop-gap for arm systems might be to do something like:

--- 8< ---

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index 1bb6e7c069..7402d02875 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -57,7 +57,7 @@
 #if defined(__i386__) || defined(__x86_64__)
   #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
 #elif defined(__GNUC__) && defined(__arm__)
-  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0)
+  #define setW(x, val) do { W(x) = (val); __extension__ __asm__("":::"memory"); } while (0)
 #else
   #define setW(x, val) (W(x) = (val))
 #endif

--- >8 ---

in the meantime in a separate patch. There it seems like the memory
barrier is useful for machines with fewer than 25-ish registers. Though
obviously moot if your ultimate goal is to get rid of the block sha1
code.

But in the meantime, a stop-gap patch may be useful. If you use that
diff, feel free to forge my Signed-off-by.

Thanks,
Taylor