Re: [PATCH v4 05/10] signal: Introduce TRAP_PERF si_code and si_perf to siginfo

[Marco Elver <elver@google.com>]

On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Marco,
>
> On 08.04.2021 12:36, Marco Elver wrote:
> > Introduces the TRAP_PERF si_code, and associated siginfo_t field
> > si_perf. These will be used by the perf event subsystem to send signals
> > (if requested) to the task where an event occurred.
> >
> > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> # m68k
> > Acked-by: Arnd Bergmann <arnd@arndb.de> # asm-generic
> > Signed-off-by: Marco Elver <elver@google.com>
>
> This patch landed in linux-next as commit fb6cc127e0b6 ("signal:
> Introduce TRAP_PERF si_code and si_perf to siginfo"). It causes
> regression on my test systems (arm 32bit and 64bit). Most systems fails
> to boot in the given time frame. I've observed that there is a timeout
> waiting for udev to populate /dev and then also during the network
> interfaces configuration. Reverting this commit, together with
> 97ba62b27867 ("perf: Add support for SIGTRAP on perf events") to let it
> compile, on top of next-20210420 fixes the issue.

Thanks, this is weird for sure and nothing in particular stands out.

I have questions:
-- Can you please share your config?
-- Also, can you share how you run this? Can it be reproduced in qemu?
-- How did you derive this patch to be at fault? Why not just
97ba62b27867, given you also need to revert it?

If you are unsure which patch exactly it is, can you try just
reverting 97ba62b27867 and see what happens?

Thanks,
-- Marco

> > ---
> >   arch/m68k/kernel/signal.c          |  3 +++
> >   arch/x86/kernel/signal_compat.c    |  5 ++++-
> >   fs/signalfd.c                      |  4 ++++
> >   include/linux/compat.h             |  2 ++
> >   include/linux/signal.h             |  1 +
> >   include/uapi/asm-generic/siginfo.h |  6 +++++-
> >   include/uapi/linux/signalfd.h      |  4 +++-
> >   kernel/signal.c                    | 11 +++++++++++
> >   8 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
> > index 349570f16a78..a4b7ee1df211 100644
> > --- a/arch/m68k/kernel/signal.c
> > +++ b/arch/m68k/kernel/signal.c
> > @@ -622,6 +622,9 @@ static inline void siginfo_build_tests(void)
> >       /* _sigfault._addr_pkey */
> >       BUILD_BUG_ON(offsetof(siginfo_t, si_pkey) != 0x12);
> >
> > +     /* _sigfault._perf */
> > +     BUILD_BUG_ON(offsetof(siginfo_t, si_perf) != 0x10);
> > +
> >       /* _sigpoll */
> >       BUILD_BUG_ON(offsetof(siginfo_t, si_band)   != 0x0c);
> >       BUILD_BUG_ON(offsetof(siginfo_t, si_fd)     != 0x10);
> > diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
> > index a5330ff498f0..0e5d0a7e203b 100644
> > --- a/arch/x86/kernel/signal_compat.c
> > +++ b/arch/x86/kernel/signal_compat.c
> > @@ -29,7 +29,7 @@ static inline void signal_compat_build_tests(void)
> >       BUILD_BUG_ON(NSIGFPE  != 15);
> >       BUILD_BUG_ON(NSIGSEGV != 9);
> >       BUILD_BUG_ON(NSIGBUS  != 5);
> > -     BUILD_BUG_ON(NSIGTRAP != 5);
> > +     BUILD_BUG_ON(NSIGTRAP != 6);
> >       BUILD_BUG_ON(NSIGCHLD != 6);
> >       BUILD_BUG_ON(NSIGSYS  != 2);
> >
> > @@ -138,6 +138,9 @@ static inline void signal_compat_build_tests(void)
> >       BUILD_BUG_ON(offsetof(siginfo_t, si_pkey) != 0x20);
> >       BUILD_BUG_ON(offsetof(compat_siginfo_t, si_pkey) != 0x14);
> >
> > +     BUILD_BUG_ON(offsetof(siginfo_t, si_perf) != 0x18);
> > +     BUILD_BUG_ON(offsetof(compat_siginfo_t, si_perf) != 0x10);
> > +
> >       CHECK_CSI_OFFSET(_sigpoll);
> >       CHECK_CSI_SIZE  (_sigpoll, 2*sizeof(int));
> >       CHECK_SI_SIZE   (_sigpoll, 4*sizeof(int));
> > diff --git a/fs/signalfd.c b/fs/signalfd.c
> > index 456046e15873..040a1142915f 100644
> > --- a/fs/signalfd.c
> > +++ b/fs/signalfd.c
> > @@ -134,6 +134,10 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> >   #endif
> >               new.ssi_addr_lsb = (short) kinfo->si_addr_lsb;
> >               break;
> > +     case SIL_PERF_EVENT:
> > +             new.ssi_addr = (long) kinfo->si_addr;
> > +             new.ssi_perf = kinfo->si_perf;
> > +             break;
> >       case SIL_CHLD:
> >               new.ssi_pid    = kinfo->si_pid;
> >               new.ssi_uid    = kinfo->si_uid;
> > diff --git a/include/linux/compat.h b/include/linux/compat.h
> > index 6e65be753603..c8821d966812 100644
> > --- a/include/linux/compat.h
> > +++ b/include/linux/compat.h
> > @@ -236,6 +236,8 @@ typedef struct compat_siginfo {
> >                                       char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD];
> >                                       u32 _pkey;
> >                               } _addr_pkey;
> > +                             /* used when si_code=TRAP_PERF */
> > +                             compat_u64 _perf;
> >                       };
> >               } _sigfault;
> >
> > diff --git a/include/linux/signal.h b/include/linux/signal.h
> > index 205526c4003a..1e98548d7cf6 100644
> > --- a/include/linux/signal.h
> > +++ b/include/linux/signal.h
> > @@ -43,6 +43,7 @@ enum siginfo_layout {
> >       SIL_FAULT_MCEERR,
> >       SIL_FAULT_BNDERR,
> >       SIL_FAULT_PKUERR,
> > +     SIL_PERF_EVENT,
> >       SIL_CHLD,
> >       SIL_RT,
> >       SIL_SYS,
> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> > index d2597000407a..d0bb9125c853 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -91,6 +91,8 @@ union __sifields {
> >                               char _dummy_pkey[__ADDR_BND_PKEY_PAD];
> >                               __u32 _pkey;
> >                       } _addr_pkey;
> > +                     /* used when si_code=TRAP_PERF */
> > +                     __u64 _perf;
> >               };
> >       } _sigfault;
> >
> > @@ -155,6 +157,7 @@ typedef struct siginfo {
> >   #define si_lower    _sifields._sigfault._addr_bnd._lower
> >   #define si_upper    _sifields._sigfault._addr_bnd._upper
> >   #define si_pkey             _sifields._sigfault._addr_pkey._pkey
> > +#define si_perf              _sifields._sigfault._perf
> >   #define si_band             _sifields._sigpoll._band
> >   #define si_fd               _sifields._sigpoll._fd
> >   #define si_call_addr        _sifields._sigsys._call_addr
> > @@ -253,7 +256,8 @@ typedef struct siginfo {
> >   #define TRAP_BRANCH     3   /* process taken branch trap */
> >   #define TRAP_HWBKPT     4   /* hardware breakpoint/watchpoint */
> >   #define TRAP_UNK    5       /* undiagnosed trap */
> > -#define NSIGTRAP     5
> > +#define TRAP_PERF    6       /* perf event with sigtrap=1 */
> > +#define NSIGTRAP     6
> >
> >   /*
> >    * There is an additional set of SIGTRAP si_codes used by ptrace
> > diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
> > index 83429a05b698..7e333042c7e3 100644
> > --- a/include/uapi/linux/signalfd.h
> > +++ b/include/uapi/linux/signalfd.h
> > @@ -39,6 +39,8 @@ struct signalfd_siginfo {
> >       __s32 ssi_syscall;
> >       __u64 ssi_call_addr;
> >       __u32 ssi_arch;
> > +     __u32 __pad3;
> > +     __u64 ssi_perf;
> >
> >       /*
> >        * Pad strcture to 128 bytes. Remember to update the
> > @@ -49,7 +51,7 @@ struct signalfd_siginfo {
> >        * comes out of a read(2) and we really don't want to have
> >        * a compat on read(2).
> >        */
> > -     __u8 __pad[28];
> > +     __u8 __pad[16];
> >   };
> >
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index f2718350bf4b..7061e4957650 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1199,6 +1199,7 @@ static inline bool has_si_pid_and_uid(struct kernel_siginfo *info)
> >       case SIL_FAULT_MCEERR:
> >       case SIL_FAULT_BNDERR:
> >       case SIL_FAULT_PKUERR:
> > +     case SIL_PERF_EVENT:
> >       case SIL_SYS:
> >               ret = false;
> >               break;
> > @@ -2531,6 +2532,7 @@ static void hide_si_addr_tag_bits(struct ksignal *ksig)
> >       case SIL_FAULT_MCEERR:
> >       case SIL_FAULT_BNDERR:
> >       case SIL_FAULT_PKUERR:
> > +     case SIL_PERF_EVENT:
> >               ksig->info.si_addr = arch_untagged_si_addr(
> >                       ksig->info.si_addr, ksig->sig, ksig->info.si_code);
> >               break;
> > @@ -3341,6 +3343,10 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
> >   #endif
> >               to->si_pkey = from->si_pkey;
> >               break;
> > +     case SIL_PERF_EVENT:
> > +             to->si_addr = ptr_to_compat(from->si_addr);
> > +             to->si_perf = from->si_perf;
> > +             break;
> >       case SIL_CHLD:
> >               to->si_pid = from->si_pid;
> >               to->si_uid = from->si_uid;
> > @@ -3421,6 +3427,10 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
> >   #endif
> >               to->si_pkey = from->si_pkey;
> >               break;
> > +     case SIL_PERF_EVENT:
> > +             to->si_addr = compat_ptr(from->si_addr);
> > +             to->si_perf = from->si_perf;
> > +             break;
> >       case SIL_CHLD:
> >               to->si_pid    = from->si_pid;
> >               to->si_uid    = from->si_uid;
> > @@ -4601,6 +4611,7 @@ static inline void siginfo_buildtime_checks(void)
> >       CHECK_OFFSET(si_lower);
> >       CHECK_OFFSET(si_upper);
> >       CHECK_OFFSET(si_pkey);
> > +     CHECK_OFFSET(si_perf);
> >
> >       /* sigpoll */
> >       CHECK_OFFSET(si_band);
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>