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

[H.J. Lu]

On Mon, May 18, 2020 at 12:19 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * 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.

Fixed.

> > +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 “.”.

Fixed.

> > +  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.

Fixed.

> 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.

We don't have a pure local struct for rtld.  _rtld_local is a hidden
alias of _rtld_global:

extern struct rtld_global _rtld_local
    __attribute__ ((alias ("_rtld_global"), visibility ("hidden")));

struct rtld_global is used to improve IP-relative access.  It may not
be important for x86-64.  But it is still useful for i386 which doesn't
have IP-relative addressing.

Here is the updated patch.  I combined

   Include <dl-procruntime.c> to get architecture specific initializer in
    rtld_global.

OK for master?

Thanks.

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-x86-Move-CET-control-to-_dl_x86_feature_control-BZ-2.patch
Type: text/x-patch
Size: 13717 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20200518/5d3987b4/attachment.bin>