Subject: bug#64058: closed (Re: bug#64058: [PATCH] wc: Fix crashes due to incomplete AVX2 enumeration)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 15 Jun 2023 00:23:02 +0000

Your bug report

#64058: [PATCH] wc: Fix crashes due to incomplete AVX2 enumeration

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
64058: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64058
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#64058: [PATCH] wc: Fix crashes due to incomplete AVX2 enumeration
From: Pádraig Brady <[email-masked]>
To: Paul Eggert <[email-masked]>
Cc: [email-masked]
Date: Thu, 15 Jun 2023 01:22:07 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 14/06/2023 23:04, Paul Eggert wrote:
> On 6/14/23 03:46, Pádraig Brady wrote:
> 
>> Paul you removed the "avx" check from cksum.c. Was that intended?
> 
> No, it's a typo I introduced. Thanks for catching that. Fixed in the
> first attached patch.
> 
> While looking into this I noticed a couple of other cleanups, fixed in
> the other attached patches.
> 
> I installed these into coreutils on Savannah.

Changes look good.
I pushed the attached to explicitly document the cksum fix,
and cleanup a syntax-check failure.

Marking this as done.

thanks,
Pádraig

From d53190ed46a55f599800ebb2d8ddfe38205dbd24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Thu, 15 Jun 2023 00:53:54 +0100
Subject: [PATCH] doc: mention cksum error fix with cpu feature checks changes

* NEWS: Mention the error message to aid those searching
for solutions to the issue, and mention cksum also
as that was confirmed to fix the error with the adjusted
cpu feature detection, as discussed at https://bugs.debian.org/1037264
* src/cksum.c: Cleanup syntax-check failure from previous commit.
---
 NEWS        | 3 ++-
 src/cksum.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 3350f9871..535850549 100644
--- a/NEWS
+++ b/NEWS
@@ -29,7 +29,8 @@ GNU coreutils NEWS                                    -*- outline -*-
   'pr --length=1 --double-space' no longer enters an infinite loop.
   [This bug was present in "the beginning".]
 
-  'wc -l' no longer crashes on x86 Linux kernels that disable XSAVE YMM.
+  'wc -l' and 'cksum' no longer crash with an "Illegal instruction" error
+  on x86 Linux kernels that disable XSAVE YMM.  This was seen on Xen VMs.
   [bug introduced in coreutils-9.0]
 
 ** Changes in behavior
diff --git a/src/cksum.c b/src/cksum.c
index 26bb29bdb..5b6a59c16 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -229,9 +229,9 @@ crc_sum_stream (FILE *stream, void *resstream, uintmax_t *length)
   static bool (*cksum_fp) (FILE *, uint_fast32_t *, uintmax_t *);
   if (! cksum_fp)
     cksum_fp = pclmul_supported () ? cksum_pclmul : cksum_slice8;
-#else
+# else
   bool (*cksum_fp) (FILE *, uint_fast32_t *, uintmax_t *) = cksum_slice8;
-#endif
+# endif
 
   if (! cksum_fp (stream, &crc, &total_bytes))
     return -1;
-- 
2.40.1

Subject: [PATCH] wc: Fix crashes due to incomplete AVX2 enumeration
From: Dave Hansen <[email-masked]>
To: [email-masked]
Cc: Kristoffer Brånemyr <[email-masked]>, Dave Hansen <[email-masked]>
Date: Tue, 13 Jun 2023 10:26:10 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

The AVX2 enumeration for 'wc -l' is incomplete which may cause wc to
crash.

The Intel SDM documents the whole AVX2 enumeration sequence in its
"Detection of Intel AVX2" section.  There are three pieces:
 1. Ensuring the CPU supports AVX2 instructions
 2. Ensuring the OS has enabled XSAVE
 3. Ensuring XSAVE is managing the YMM registers (XGETBV)

The existing code does #1 and #2 but misses #3.  The kernel will
enable #3 in almost all situations where AVX2 is supported, but there
are situations where the kernel might disable XSAVE YMM support but
leave FP and SSE enabled.  This is _unusual_ today, but can occur when
Linux notices XSAVE enumeration issues or in virtual machines with
odd/buggy XSAVE enumeration.

If wc is used on one of those systems in '-l' mode, it will crash:

	# cat foo | ./wc.orig -l
	Illegal instruction (core dumped)

Fortunately, gcc and llvm provide a builtin to check for CPU support
and AVX2 is supported.  Use the builtin instead of using CPUID
directly.  This fixes the problem and is also substantially simpler.

The result is that wc no longer crashes on systems where XSAVE YMM
support is disabled:

	# cat foo | ./wc.fixed -l
	30232

I've reproduced this issue in the current upstream git repo which git
describe says is v9.3-53-g378902407.

I needed to run a patched version of Linux to expose this issue.  We
(the upstream Linux x86 maintainers) are considering merging a patch
to do this in the upstream kernel.  It would be great if a wc fix was
available.
---
 src/wc.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/src/wc.c b/src/wc.c
index becceda98..944cfc426 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -157,41 +157,7 @@ static enum total_type total_mode = total_auto;
 static bool
 avx2_supported (void)
 {
-  unsigned int eax = 0;
-  unsigned int ebx = 0;
-  unsigned int ecx = 0;
-  unsigned int edx = 0;
-  bool getcpuid_ok = false;
-  bool avx_enabled = false;
-
-  if (__get_cpuid (1, &eax, &ebx, &ecx, &edx))
-    {
-      getcpuid_ok = true;
-      if (ecx & bit_OSXSAVE)
-        avx_enabled = true;  /* Support is not disabled.  */
-    }
-
-
-  if (avx_enabled)
-    {
-      eax = ebx = ecx = edx = 0;
-      if (! __get_cpuid_count (7, 0, &eax, &ebx, &ecx, &edx))
-        getcpuid_ok = false;
-      else
-        {
-          if (! (ebx & bit_AVX2))
-            avx_enabled = false;  /* Hardware doesn't support it.  */
-        }
-    }
-
-
-  if (! getcpuid_ok)
-    {
-      if (debug)
-        error (0, 0, "%s", _("failed to get cpuid"));
-      return false;
-    }
-  else if (! avx_enabled)
+  if (!__builtin_cpu_supports("avx2"))
     {
       if (debug)
         error (0, 0, "%s", _("avx2 support not detected"));
-- 
2.34.1