Re: [PATCH v14 4/8] signal: deduplicate code dealing with common _sigfault fields

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 9, 2020 at 4:41 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>
> Peter Collingbourne <pcc@xxxxxxxxxx> writes:
>
> > We're about to add more common _sigfault fields, so deduplicate the
> > existing code for initializing _sigfault fields in {send,force}_sig_*,
> > and for copying _sigfault fields in copy_siginfo_to_external32 and
> > post_copy_siginfo_from_user32, to reduce the number of places that
> > will need to be updated by upcoming changes.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>

Thanks for the review.

> No real objection but I am wondering if it might be better to
> introduce two small inline functions for setting common fields
> instead of:
>
> > +     if (siginfo_layout_is_fault(layout)) {
> > +             to->si_addr = ptr_to_compat(from->si_addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > +             to->si_trapno = from->si_trapno;
> > +#endif
> > +     }
>
> and
>
> > +     if (siginfo_layout_is_fault(layout)) {
> > +             to->si_addr = compat_ptr(from->si_addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > +             to->si_trapno = from->si_trapno;
> > +#endif
> > +     }
>
> perhaps called:
> copy_sigfault_common_to_external32
> post_copy_sigfault_common_from_user32
>
> I have not benchmarked or anything but my gut says one less conditional
> branch to worry about makes dealing with spectre easier and probably
> produces faster code as well.  Possibly even smaller code.

Dave made the same proposal on an earlier version of the patch which I
responded to in [1]. The main reason for keeping things as I
implemented them was because of the ptrace handling but if we do end
up dropping that as you proposed on the other patch then I think I'd
be happy to move the code into helper functions.

Peter

[1] https://lore.kernel.org/linux-parisc/CAMn1gO42arQKGBj1Nnbs86TGYyogpRR_t73H=GbTmQrbAbV30A@xxxxxxxxxxxxxx/

>
> Eric
>
> > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx>
> > Link: https://linux-review.googlesource.com/id/I4f56174e1b7b2bf4a3c8139e6879cbfd52750a24
> > ---
> >  include/linux/signal.h |  13 ++++++
> >  kernel/signal.c        | 101 ++++++++++++++++-------------------------
> >  2 files changed, 53 insertions(+), 61 deletions(-)
> >
> > diff --git a/include/linux/signal.h b/include/linux/signal.h
> > index b256f9c65661..e9fb05041e7a 100644
> > --- a/include/linux/signal.h
> > +++ b/include/linux/signal.h
> > @@ -50,6 +50,19 @@ enum siginfo_layout {
> >
> >  enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
> >
> > +static inline bool siginfo_layout_is_fault(enum siginfo_layout layout)
> > +{
> > +     switch (layout) {
> > +     case SIL_FAULT:
> > +     case SIL_FAULT_MCEERR:
> > +     case SIL_FAULT_BNDERR:
> > +     case SIL_FAULT_PKUERR:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> >  /*
> >   * Define some primitives to manipulate sigset_t.
> >   */
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index ef8f2a28d37c..74e7315c24db 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1650,6 +1650,15 @@ void force_sigsegv(int sig)
> >       force_sig(SIGSEGV);
> >  }
> >
> > +static void set_sigfault_common_fields(struct kernel_siginfo *info, int sig,
> > +                                    int code, void __user *addr)
> > +{
> > +     info->si_signo = sig;
> > +     info->si_errno = 0;
> > +     info->si_code = code;
> > +     info->si_addr = addr;
> > +}
> > +
> >  int force_sig_fault_to_task(int sig, int code, void __user *addr
> >       ___ARCH_SI_TRAPNO(int trapno)
> >       ___ARCH_SI_IA64(int imm, unsigned int flags, unsigned long isr)
> > @@ -1658,10 +1667,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr
> >       struct kernel_siginfo info;
> >
> >       clear_siginfo(&info);
> > -     info.si_signo = sig;
> > -     info.si_errno = 0;
> > -     info.si_code  = code;
> > -     info.si_addr  = addr;
> > +     set_sigfault_common_fields(&info, sig, code, addr);
> >  #ifdef __ARCH_SI_TRAPNO
> >       info.si_trapno = trapno;
> >  #endif
> > @@ -1690,10 +1696,7 @@ int send_sig_fault(int sig, int code, void __user *addr
> >       struct kernel_siginfo info;
> >
> >       clear_siginfo(&info);
> > -     info.si_signo = sig;
> > -     info.si_errno = 0;
> > -     info.si_code  = code;
> > -     info.si_addr  = addr;
> > +     set_sigfault_common_fields(&info, sig, code, addr);
> >  #ifdef __ARCH_SI_TRAPNO
> >       info.si_trapno = trapno;
> >  #endif
> > @@ -1711,10 +1714,7 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> >
> >       WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> >       clear_siginfo(&info);
> > -     info.si_signo = SIGBUS;
> > -     info.si_errno = 0;
> > -     info.si_code = code;
> > -     info.si_addr = addr;
> > +     set_sigfault_common_fields(&info, SIGBUS, code, addr);
> >       info.si_addr_lsb = lsb;
> >       return force_sig_info(&info);
> >  }
> > @@ -1725,10 +1725,7 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct *
> >
> >       WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR));
> >       clear_siginfo(&info);
> > -     info.si_signo = SIGBUS;
> > -     info.si_errno = 0;
> > -     info.si_code = code;
> > -     info.si_addr = addr;
> > +     set_sigfault_common_fields(&info, SIGBUS, code, addr);
> >       info.si_addr_lsb = lsb;
> >       return send_sig_info(info.si_signo, &info, t);
> >  }
> > @@ -1739,10 +1736,7 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper)
> >       struct kernel_siginfo info;
> >
> >       clear_siginfo(&info);
> > -     info.si_signo = SIGSEGV;
> > -     info.si_errno = 0;
> > -     info.si_code  = SEGV_BNDERR;
> > -     info.si_addr  = addr;
> > +     set_sigfault_common_fields(&info, SIGSEGV, SEGV_BNDERR, addr);
> >       info.si_lower = lower;
> >       info.si_upper = upper;
> >       return force_sig_info(&info);
> > @@ -1754,10 +1748,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey)
> >       struct kernel_siginfo info;
> >
> >       clear_siginfo(&info);
> > -     info.si_signo = SIGSEGV;
> > -     info.si_errno = 0;
> > -     info.si_code  = SEGV_PKUERR;
> > -     info.si_addr  = addr;
> > +     set_sigfault_common_fields(&info, SIGSEGV, SEGV_PKUERR, addr);
> >       info.si_pkey  = pkey;
> >       return force_sig_info(&info);
> >  }
> > @@ -1771,10 +1762,8 @@ int force_sig_ptrace_errno_trap(int errno, void __user *addr)
> >       struct kernel_siginfo info;
> >
> >       clear_siginfo(&info);
> > -     info.si_signo = SIGTRAP;
> > +     set_sigfault_common_fields(&info, SIGTRAP, TRAP_HWBKPT, addr);
> >       info.si_errno = errno;
> > -     info.si_code  = TRAP_HWBKPT;
> > -     info.si_addr  = addr;
> >       return force_sig_info(&info);
> >  }
> >
> > @@ -3267,12 +3256,23 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
> >  void copy_siginfo_to_external32(struct compat_siginfo *to,
> >               const struct kernel_siginfo *from)
> >  {
> > +     enum siginfo_layout layout =
> > +             siginfo_layout(from->si_signo, from->si_code);
> > +
> >       memset(to, 0, sizeof(*to));
> >
> >       to->si_signo = from->si_signo;
> >       to->si_errno = from->si_errno;
> >       to->si_code  = from->si_code;
> > -     switch(siginfo_layout(from->si_signo, from->si_code)) {
> > +
> > +     if (siginfo_layout_is_fault(layout)) {
> > +             to->si_addr = ptr_to_compat(from->si_addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > +             to->si_trapno = from->si_trapno;
> > +#endif
> > +     }
> > +
> > +     switch (layout) {
> >       case SIL_KILL:
> >               to->si_pid = from->si_pid;
> >               to->si_uid = from->si_uid;
> > @@ -3287,31 +3287,15 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
> >               to->si_fd   = from->si_fd;
> >               break;
> >       case SIL_FAULT:
> > -             to->si_addr = ptr_to_compat(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               break;
> >       case SIL_FAULT_MCEERR:
> > -             to->si_addr = ptr_to_compat(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_addr_lsb = from->si_addr_lsb;
> >               break;
> >       case SIL_FAULT_BNDERR:
> > -             to->si_addr = ptr_to_compat(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_lower = ptr_to_compat(from->si_lower);
> >               to->si_upper = ptr_to_compat(from->si_upper);
> >               break;
> >       case SIL_FAULT_PKUERR:
> > -             to->si_addr = ptr_to_compat(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_pkey = from->si_pkey;
> >               break;
> >       case SIL_CHLD:
> > @@ -3348,11 +3332,22 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
> >  static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
> >                                        const struct compat_siginfo *from)
> >  {
> > +     enum siginfo_layout layout =
> > +             siginfo_layout(from->si_signo, from->si_code);
> > +
> >       clear_siginfo(to);
> >       to->si_signo = from->si_signo;
> >       to->si_errno = from->si_errno;
> >       to->si_code  = from->si_code;
> > -     switch(siginfo_layout(from->si_signo, from->si_code)) {
> > +
> > +     if (siginfo_layout_is_fault(layout)) {
> > +             to->si_addr = compat_ptr(from->si_addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > +             to->si_trapno = from->si_trapno;
> > +#endif
> > +     }
> > +
> > +     switch (layout) {
> >       case SIL_KILL:
> >               to->si_pid = from->si_pid;
> >               to->si_uid = from->si_uid;
> > @@ -3367,31 +3362,15 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
> >               to->si_fd   = from->si_fd;
> >               break;
> >       case SIL_FAULT:
> > -             to->si_addr = compat_ptr(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               break;
> >       case SIL_FAULT_MCEERR:
> > -             to->si_addr = compat_ptr(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_addr_lsb = from->si_addr_lsb;
> >               break;
> >       case SIL_FAULT_BNDERR:
> > -             to->si_addr = compat_ptr(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_lower = compat_ptr(from->si_lower);
> >               to->si_upper = compat_ptr(from->si_upper);
> >               break;
> >       case SIL_FAULT_PKUERR:
> > -             to->si_addr = compat_ptr(from->si_addr);
> > -#ifdef __ARCH_SI_TRAPNO
> > -             to->si_trapno = from->si_trapno;
> > -#endif
> >               to->si_pkey = from->si_pkey;
> >               break;
> >       case SIL_CHLD:



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux