Re: [PATCH 2/4] x86/signal: Use switch_to_permissive_pkey_reg() helper

[Dmitry Vyukov]

On Fri, 21 Feb 2025 at 17:26, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> ...
> > -/*
> > - * Enable all pkeys temporarily, so as to ensure that both the current
> > - * execution stack as well as the alternate signal stack are writeable.
> > - * The application can use any of the available pkeys to protect the
> > - * alternate signal stack, and we don't know which one it is, so enable
> > - * all. The PKRU register will be reset to init_pkru later in the flow,
> > - * in fpu__clear_user_states(), and it is the application's responsibility
> > - * to enable the appropriate pkey as the first step in the signal handler
> > - * so that the handler does not segfault.
> > - */
> > -static inline u32 sig_prepare_pkru(void)
> > -{
> > -     u32 orig_pkru = read_pkru();
> > -
> > -     write_pkru(0);
> > -     return orig_pkru;
> > -}
> > -
> >  /*
> >   * Set up a signal frame.
> >   */
> > @@ -157,8 +140,18 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> >               return (void __user *)-1L;
> >       }
> >
> > -     /* Update PKRU to enable access to the alternate signal stack. */
> > -     pkru = sig_prepare_pkru();
> > +     /*
> > +      * Enable all pkeys temporarily, so as to ensure that both the current
> > +      * execution stack as well as the alternate signal stack are
> > +      * writeable. The application can use any of the available pkeys to
> > +      * protect the alternate signal stack, and we don't know which one it
> > +      * is, so enable all. The PKRU register will be reset to init_pkru
> > +      * later in the flow, in fpu__clear_user_states(), and it is the
> > +      * application's responsibility to enable the appropriate pkey as the
> > +      * first step in the signal handler so that the handler does not
> > +      * segfault.
> > +      */
> > +     pkru = switch_to_permissive_pkey_reg();
> I think this hurts readability too much in the get_sigframe() code. On
> some level, it's silly to have a basically empty helper. But in this
> case, it does help keep the signal code readable.
>
> In other words, this would be preferred:
>
> /*
>  * Keep existing big comment
>  */
> static inline u32 sig_prepare_pkru(void)
> {
>         return switch_to_permissive_pkey_reg();
> }

Makes sense. Done in v3.