Bug 29953 - Invalid x86_non_temporal_threshold without cache info
Summary: Invalid x86_non_temporal_threshold without cache info
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: dynamic-link (show other bugs)
Version: 2.36
: P2 minor
Target Milestone: 2.37
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 30037 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-01-02 17:33 UTC by Alexis Engelke
Modified: 2023-07-17 07:29 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Add minval (378 bytes, patch)
2023-01-02 21:21 UTC, Noah Goldstein
Details | Diff
Ensure minimum value (672 bytes, patch)
2023-01-03 12:49 UTC, Alexis Engelke
Details | Diff
A patch (1.20 KB, patch)
2023-01-03 16:23 UTC, H.J. Lu
Details | Diff
Fix with new API for clamping tunable value (1.80 KB, patch)
2023-01-03 18:13 UTC, Noah Goldstein
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Engelke 2023-01-02 17:33:37 UTC
When cpuid reports no information on a shared cache (e.g., in an emulator), the x86_non_temporal_threshold will be set to zero, causing memcpy/memset to behave wrong for mid-sized operations. sysdeps/x86/dl-cacheinfo.h indicates that the minimum value must be 0x4040, but this is not enforced for the default value (see also commit b446822b6ae).

In the emulator, `/lib64/ld-linux-x86-64.so.2 --list-tunables` reports:

    glibc.cpu.x86_non_temporal_threshold: 0x0 (min: 0x0, max: 0xffffffffffffffff)

Setting the tunable explicitly (env GLIBC_TUNABLES=glibc.cpu.x86_non_temporal_threshold=0x4040) avoids the problem.

As a side note, ld.so won't load any DSO on x86-64 ("CPU ISA level is lower than required") when the cpuid vendor not listed in sysdeps/x86/cpu-features.c, as the feature bits in cpuid leaf 1 are not decoded in get_common_indices (family is NULL).
Comment 1 Noah Goldstein 2023-01-02 21:21:43 UTC
Created attachment 14545 [details]
Add minval
Comment 2 Noah Goldstein 2023-01-03 00:05:05 UTC
(In reply to Noah Goldstein from comment #1)
> Created attachment 14545 [details]
> Add minval

Sorry wasn't clear, does that fix the issue?
Comment 3 Alexis Engelke 2023-01-03 12:49:24 UTC
Created attachment 14547 [details]
Ensure minimum value

(In reply to Noah Goldstein from comment #2)
> Sorry wasn't clear, does that fix the issue?

No. Now --list-tunables reports:

    glibc.cpu.x86_non_temporal_threshold: 0x0 (min: 0x4040, max: 0xffffffffffffffff)

The attached patch fixes the problem that the default value is never checked against the limits.
Comment 4 H.J. Lu 2023-01-03 16:23:37 UTC
Created attachment 14551 [details]
A patch

Try this.
Comment 5 Noah Goldstein 2023-01-03 17:52:51 UTC
(In reply to H.J. Lu from comment #4)
> Created attachment 14551 [details]
> A patch
> 
> Try this.

Oh wow, I had always assumed `TUNABLE_SET_WITH_BOUNDS` would just clamp the value.

We also have a bug with `x86_rep_movsb_threshold` which requires at least 128 (2x AVX512 aligning vectors).
Comment 6 H.J. Lu 2023-01-03 17:55:46 UTC
(In reply to Noah Goldstein from comment #5)
> We also have a bug with `x86_rep_movsb_threshold` which requires at least
> 128 (2x AVX512 aligning vectors).

x86_rep_movsb_threshold should be fine.  minimum_rep_movsb_threshold should
be set and checked properly.
Comment 7 Noah Goldstein 2023-01-03 18:00:26 UTC
(In reply to H.J. Lu from comment #6)
> (In reply to Noah Goldstein from comment #5)
> > We also have a bug with `x86_rep_movsb_threshold` which requires at least
> > 128 (2x AVX512 aligning vectors).
> 
> x86_rep_movsb_threshold should be fine.  minimum_rep_movsb_threshold should
> be set and checked properly.

Seems like the exact same issue no?

```
  tunable_size = TUNABLE_GET (x86_rep_movsb_threshold, long int, NULL);
  if (tunable_size > minimum_rep_movsb_threshold)
    rep_movsb_threshold = tunable_size;

  // if tunable_size was zero, rep_movsb_threshold == 0
 
  ...

  // Won't clamp, will leave x86_rep_movsb_threshold at zero.
  TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold,
			   minimum_rep_movsb_threshold, SIZE_MAX);




```
Comment 8 H.J. Lu 2023-01-03 18:04:24 UTC
(In reply to Noah Goldstein from comment #7)
> (In reply to H.J. Lu from comment #6)
> > (In reply to Noah Goldstein from comment #5)
> > > We also have a bug with `x86_rep_movsb_threshold` which requires at least
> > > 128 (2x AVX512 aligning vectors).
> > 
> > x86_rep_movsb_threshold should be fine.  minimum_rep_movsb_threshold should
> > be set and checked properly.
> 
> Seems like the exact same issue no?
> 
> ```
>   tunable_size = TUNABLE_GET (x86_rep_movsb_threshold, long int, NULL);
>   if (tunable_size > minimum_rep_movsb_threshold)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     rep_movsb_threshold = tunable_size;
> 
>   // if tunable_size was zero, rep_movsb_threshold == 0

Since minimum_rep_movsb_threshold is > 0, rep_movsb_threshold won't be zero.

>  
>   ...
> 
>   // Won't clamp, will leave x86_rep_movsb_threshold at zero.
>   TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold,
> 			   minimum_rep_movsb_threshold, SIZE_MAX);
> 
> 
> 
> 
> ```
Comment 9 Noah Goldstein 2023-01-03 18:13:26 UTC
Created attachment 14552 [details]
Fix with new API for clamping tunable value

Personally more infavor of an approach like this.

Seems a bit insane that when we have a minval > 0, we default to potentially leaving the tunable at zero.
Comment 10 H.J. Lu 2023-01-03 18:22:07 UTC
(In reply to Noah Goldstein from comment #9)
> Created attachment 14552 [details]
> Fix with new API for clamping tunable value
> 
> Personally more infavor of an approach like this.
> 
> Seems a bit insane that when we have a minval > 0, we default to potentially
> leaving the tunable at zero.

TUNABLE_SET_WITH_BOUNDS is used to update dynamic bounds for display purpose.
The tunable value may be retrieved before TUNABLE_SET_WITH_BOUNDS.
Comment 11 Alexis Engelke 2023-01-03 18:39:23 UTC
(In reply to H.J. Lu from comment #4)
> Try this.
This fixes the problem, thanks.

(In reply to Noah Goldstein from comment #9)
> Created attachment 14552 [details]
> Fix with new API for clamping tunable value
This also fixes the bug when tunables are enabled, but has the problem that no clamping will happen when tunables are disabled.
Comment 12 Noah Goldstein 2023-01-03 19:24:54 UTC
(In reply to Alexis Engelke from comment #11)
> (In reply to H.J. Lu from comment #4)
> > Try this.
> This fixes the problem, thanks.
> 
> (In reply to Noah Goldstein from comment #9)
> > Created attachment 14552 [details]
> > Fix with new API for clamping tunable value
> This also fixes the bug when tunables are enabled, but has the problem that
> no clamping will happen when tunables are disabled.

We can go with H.J's fix, making the tunable setter clamping can be an independent QOL change later on.
Comment 13 Sourceware Commits 2023-01-03 21:58:28 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=48b74865c63840b288bd85b4d8743533b73b339b

commit 48b74865c63840b288bd85b4d8743533b73b339b
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Jan 3 13:06:48 2023 -0800

    x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
    
    The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
    be set to less than the minimum value when the shared cache size isn't
    available (e.g., in an emulator) or by the tunable.  Add checks for
    minimum and maximum of non_temporal_threshold.
    
    This fixes BZ #29953.
Comment 14 Florian Weimer 2023-03-13 17:59:37 UTC
*** Bug 30037 has been marked as a duplicate of this bug. ***
Comment 15 Florian Weimer 2023-03-13 18:00:45 UTC
(In reply to cvs-commit@gcc.gnu.org from comment #13)
> The master branch has been updated by H.J. Lu <hjl@sourceware.org>:
> 
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;
> h=48b74865c63840b288bd85b4d8743533b73b339b
> 
> commit 48b74865c63840b288bd85b4d8743533b73b339b
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Tue Jan 3 13:06:48 2023 -0800
> 
>     x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
>     
>     The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
>     be set to less than the minimum value when the shared cache size isn't
>     available (e.g., in an emulator) or by the tunable.  Add checks for
>     minimum and maximum of non_temporal_threshold.
>     
>     This fixes BZ #29953.

H.J., do you intend to backport this? Can we close this bug?
Comment 16 H.J. Lu 2023-03-13 18:17:38 UTC
(In reply to Florian Weimer from comment #15)
> (In reply to cvs-commit@gcc.gnu.org from comment #13)
> > The master branch has been updated by H.J. Lu <hjl@sourceware.org>:
> > 
> > https://sourceware.org/git/gitweb.cgi?p=glibc.git;
> > h=48b74865c63840b288bd85b4d8743533b73b339b
> > 
> > commit 48b74865c63840b288bd85b4d8743533b73b339b
> > Author: H.J. Lu <hjl.tools@gmail.com>
> > Date:   Tue Jan 3 13:06:48 2023 -0800
> > 
> >     x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
> >     
> >     The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
> >     be set to less than the minimum value when the shared cache size isn't
> >     available (e.g., in an emulator) or by the tunable.  Add checks for
> >     minimum and maximum of non_temporal_threshold.
> >     
> >     This fixes BZ #29953.
> 
> H.J., do you intend to backport this? Can we close this bug?

I will backport it.
Comment 17 Sourceware Commits 2023-03-15 22:18:18 UTC
The release/2.36/master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=f3991fec8071dbcf3ec9f13a91c738b66fcd4159

commit f3991fec8071dbcf3ec9f13a91c738b66fcd4159
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Jan 3 13:06:48 2023 -0800

    x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
    
    The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
    be set to less than the minimum value when the shared cache size isn't
    available (e.g., in an emulator) or by the tunable.  Add checks for
    minimum and maximum of non_temporal_threshold.
    
    This fixes BZ #29953.
    
    (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b)
Comment 18 Sourceware Commits 2023-04-20 15:52:51 UTC
The release/2.34/master branch has been updated by Florian Weimer <fw@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=89c017de2f52d17862bda9a6f8382e913457bfbe

commit 89c017de2f52d17862bda9a6f8382e913457bfbe
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Jan 3 13:06:48 2023 -0800

    x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
    
    The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
    be set to less than the minimum value when the shared cache size isn't
    available (e.g., in an emulator) or by the tunable.  Add checks for
    minimum and maximum of non_temporal_threshold.
    
    This fixes BZ #29953.
    
    (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b)
Comment 19 Sourceware Commits 2023-04-20 15:53:04 UTC
The release/2.35/master branch has been updated by Florian Weimer <fw@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=b7cc55a24ea303875d95d0db3f73f9239bba2583

commit b7cc55a24ea303875d95d0db3f73f9239bba2583
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Jan 3 13:06:48 2023 -0800

    x86: Check minimum/maximum of non_temporal_threshold [BZ #29953]
    
    The minimum non_temporal_threshold is 0x4040.  non_temporal_threshold may
    be set to less than the minimum value when the shared cache size isn't
    available (e.g., in an emulator) or by the tunable.  Add checks for
    minimum and maximum of non_temporal_threshold.
    
    This fixes BZ #29953.
    
    (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b)
Comment 20 Florian Weimer 2023-05-08 16:44:10 UTC
Backports completed.