Re: [PATCH] x86: Move CET control to _dl_x86_feature_control [BZ #25887]

[Florian Weimer <fweimer@redhat.com>]

* H. J. Lu via Libc-alpha:

> Here is the updated patch to use bitfields.

Thanks, I like this better.

> diff --git a/sysdeps/x86/cet-tunables.h b/sysdeps/x86/cet-control.h
> similarity index 61%
> rename from sysdeps/x86/cet-tunables.h
> rename to sysdeps/x86/cet-control.h
> index 5e1e42df10..3a314f9609 100644
> --- a/sysdeps/x86/cet-tunables.h
> +++ b/sysdeps/x86/cet-control.h
> @@ -16,14 +16,26 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -/* Valid control values:
> -   0: Enable CET features based on ELF property note.
> -   1: Always disable CET features.
> -   2: Always enable CET features.
> -   3: Enable CET features permissively.
> - */
> -#define CET_ELF_PROPERTY	0
> -#define CET_ALWAYS_OFF		1
> -#define CET_ALWAYS_ON		2
> -#define CET_PERMISSIVE		3
> -#define CET_MAX			CET_PERMISSIVE
> +#ifndef _CET_CONTROL_H
> +#define _CET_CONTROL_H
> +
> +/* For each CET feature, IBT and SHSTK, valid control values  */

Missing ”.” at end of comment.

> +enum dl_x86_cet_control
> +{
> +  /* Enable CET features based on ELF property note.  */
> +  elf_property = 0,
> +  /* Always enable CET features.  */
> +  always_on,
> +  /* Always disable CET features. */
> +  always_off,
> +  /* Enable CET features permissively. */

Missing double space after “.”.

> +  permissive
> +};

Given enum constantse are not not scoped and this is a widely-included
header, I think you should include at least a “cet_” prefix in these
enum constants.

It's still not clear to me why CET control variables have to be in
_rtld_global.  Regular global variables would likely lead to clearer
code, I think.  _rtld_global is needed for read-write data that is
shared with libc/libdl, and this does not seem to apply to CET control
settings.

Thanks,
Florian