[v4,05/10] signal: Introduce TRAP_PERF si_code and si_perf to siginfo
diff mbox series

Message ID 20210408103605.1676875-6-elver@google.com
State New, archived
Headers show
Series
  • Add support for synchronous signals on perf events
Related show

Commit Message

Marco Elver April 8, 2021, 10:36 a.m. UTC
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>
---
 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(-)

Comments

Marek Szyprowski April 20, 2021, 9:26 p.m. UTC | #1
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.

> ---
>   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
Marco Elver April 20, 2021, 10:42 p.m. UTC | #2
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
>
Marek Szyprowski April 21, 2021, 6:21 a.m. UTC | #3
Hi,

On 21.04.2021 00:42, Marco Elver wrote:
> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> 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?

This happens with standard multi_v7_defconfig (arm) or just defconfig 
for arm64.

> -- Also, can you share how you run this? Can it be reproduced in qemu?
Nothing special. I just boot my test systems and see that they are 
waiting lots of time during the udev populating /dev and network 
interfaces configuration. I didn't try with qemu yet.
> -- How did you derive this patch to be at fault? Why not just
> 97ba62b27867, given you also need to revert it?
Well, I've just run my boot tests with automated 'git bisect' and that 
was its result. It was a bit late in the evening, so I didn't analyze it 
further, I've just posted a report about the issue I've found. It looks 
that bisecting pointed to a wrong commit somehow.
> If you are unsure which patch exactly it is, can you try just
> reverting 97ba62b27867 and see what happens?

Indeed, this is a real faulty commit. Initially I've decided to revert 
it to let kernel compile (it uses some symbols introduced by this 
commit). Reverting only it on top of linux-next 20210420 also fixes the 
issue. I'm sorry for the noise in this thread. I hope we will find what 
really causes the issue.

Best regards
Marek Szyprowski April 21, 2021, 7:35 a.m. UTC | #4
On 21.04.2021 08:21, Marek Szyprowski wrote:
> On 21.04.2021 00:42, Marco Elver wrote:
>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski 
>> <m.szyprowski@samsung.com> wrote:
>>> 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?
>
> This happens with standard multi_v7_defconfig (arm) or just defconfig 
> for arm64.
>
>> -- Also, can you share how you run this? Can it be reproduced in qemu?
> Nothing special. I just boot my test systems and see that they are 
> waiting lots of time during the udev populating /dev and network 
> interfaces configuration. I didn't try with qemu yet.
>> -- How did you derive this patch to be at fault? Why not just
>> 97ba62b27867, given you also need to revert it?
> Well, I've just run my boot tests with automated 'git bisect' and that 
> was its result. It was a bit late in the evening, so I didn't analyze 
> it further, I've just posted a report about the issue I've found. It 
> looks that bisecting pointed to a wrong commit somehow.
>> If you are unsure which patch exactly it is, can you try just
>> reverting 97ba62b27867 and see what happens?
>
> Indeed, this is a real faulty commit. Initially I've decided to revert 
> it to let kernel compile (it uses some symbols introduced by this 
> commit). Reverting only it on top of linux-next 20210420 also fixes 
> the issue. I'm sorry for the noise in this thread. I hope we will find 
> what really causes the issue.

This was a premature conclusion. It looks that during the test I've did 
while writing that reply, the modules were not deployed properly and a 
test board (RPi4) booted without modules. In that case the board booted 
fine and there was no udev timeout. After deploying kernel modules, the 
udev timeout is back.

Best regards
Marco Elver April 21, 2021, 8:11 a.m. UTC | #5
On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 21.04.2021 08:21, Marek Szyprowski wrote:
> > On 21.04.2021 00:42, Marco Elver wrote:
> >> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
> >> <m.szyprowski@samsung.com> wrote:
> >>> 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?
> >
> > This happens with standard multi_v7_defconfig (arm) or just defconfig
> > for arm64.
> >
> >> -- Also, can you share how you run this? Can it be reproduced in qemu?
> > Nothing special. I just boot my test systems and see that they are
> > waiting lots of time during the udev populating /dev and network
> > interfaces configuration. I didn't try with qemu yet.
> >> -- How did you derive this patch to be at fault? Why not just
> >> 97ba62b27867, given you also need to revert it?
> > Well, I've just run my boot tests with automated 'git bisect' and that
> > was its result. It was a bit late in the evening, so I didn't analyze
> > it further, I've just posted a report about the issue I've found. It
> > looks that bisecting pointed to a wrong commit somehow.
> >> If you are unsure which patch exactly it is, can you try just
> >> reverting 97ba62b27867 and see what happens?
> >
> > Indeed, this is a real faulty commit. Initially I've decided to revert
> > it to let kernel compile (it uses some symbols introduced by this
> > commit). Reverting only it on top of linux-next 20210420 also fixes
> > the issue. I'm sorry for the noise in this thread. I hope we will find
> > what really causes the issue.
>
> This was a premature conclusion. It looks that during the test I've did
> while writing that reply, the modules were not deployed properly and a
> test board (RPi4) booted without modules. In that case the board booted
> fine and there was no udev timeout. After deploying kernel modules, the
> udev timeout is back.

I'm confused now. Can you confirm that the problem is due to your
kernel modules, or do you think it's still due to 97ba62b27867? Or
fb6cc127e0b6 (this patch)?

Thanks,
-- Marco
Marek Szyprowski April 21, 2021, 9:35 a.m. UTC | #6
Hi Marco,

On 21.04.2021 10:11, Marco Elver wrote:
> On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> On 21.04.2021 08:21, Marek Szyprowski wrote:
>>> On 21.04.2021 00:42, Marco Elver wrote:
>>>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> 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?
>>> This happens with standard multi_v7_defconfig (arm) or just defconfig
>>> for arm64.
>>>
>>>> -- Also, can you share how you run this? Can it be reproduced in qemu?
>>> Nothing special. I just boot my test systems and see that they are
>>> waiting lots of time during the udev populating /dev and network
>>> interfaces configuration. I didn't try with qemu yet.
>>>> -- How did you derive this patch to be at fault? Why not just
>>>> 97ba62b27867, given you also need to revert it?
>>> Well, I've just run my boot tests with automated 'git bisect' and that
>>> was its result. It was a bit late in the evening, so I didn't analyze
>>> it further, I've just posted a report about the issue I've found. It
>>> looks that bisecting pointed to a wrong commit somehow.
>>>> If you are unsure which patch exactly it is, can you try just
>>>> reverting 97ba62b27867 and see what happens?
>>> Indeed, this is a real faulty commit. Initially I've decided to revert
>>> it to let kernel compile (it uses some symbols introduced by this
>>> commit). Reverting only it on top of linux-next 20210420 also fixes
>>> the issue. I'm sorry for the noise in this thread. I hope we will find
>>> what really causes the issue.
>> This was a premature conclusion. It looks that during the test I've did
>> while writing that reply, the modules were not deployed properly and a
>> test board (RPi4) booted without modules. In that case the board booted
>> fine and there was no udev timeout. After deploying kernel modules, the
>> udev timeout is back.
> I'm confused now. Can you confirm that the problem is due to your
> kernel modules, or do you think it's still due to 97ba62b27867? Or
> fb6cc127e0b6 (this patch)?

I don't use any custom kernel modules. I just deploy all modules that 
are being built from the given kernel defconfig (arm multi_v7_defconfig 
or arm64 default) and they are automatically loaded during the boot by 
udev. I've checked again and bisect was right. The kernel built from 
fb6cc127e0b6 suffers from the described issue, while the one build from 
the previous commit (2e498d0a74e5) works fine.

Best regards
Marek Szyprowski April 21, 2021, 10:57 a.m. UTC | #7
On 21.04.2021 11:35, Marek Szyprowski wrote:
> On 21.04.2021 10:11, Marco Elver wrote:
>> On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski 
>> <m.szyprowski@samsung.com> wrote:
>>> On 21.04.2021 08:21, Marek Szyprowski wrote:
>>>> On 21.04.2021 00:42, Marco Elver wrote:
>>>>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> 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?
>>>> This happens with standard multi_v7_defconfig (arm) or just defconfig
>>>> for arm64.
>>>>
>>>>> -- Also, can you share how you run this? Can it be reproduced in 
>>>>> qemu?
>>>> Nothing special. I just boot my test systems and see that they are
>>>> waiting lots of time during the udev populating /dev and network
>>>> interfaces configuration. I didn't try with qemu yet.
>>>>> -- How did you derive this patch to be at fault? Why not just
>>>>> 97ba62b27867, given you also need to revert it?
>>>> Well, I've just run my boot tests with automated 'git bisect' and that
>>>> was its result. It was a bit late in the evening, so I didn't analyze
>>>> it further, I've just posted a report about the issue I've found. It
>>>> looks that bisecting pointed to a wrong commit somehow.
>>>>> If you are unsure which patch exactly it is, can you try just
>>>>> reverting 97ba62b27867 and see what happens?
>>>> Indeed, this is a real faulty commit. Initially I've decided to revert
>>>> it to let kernel compile (it uses some symbols introduced by this
>>>> commit). Reverting only it on top of linux-next 20210420 also fixes
>>>> the issue. I'm sorry for the noise in this thread. I hope we will find
>>>> what really causes the issue.
>>> This was a premature conclusion. It looks that during the test I've did
>>> while writing that reply, the modules were not deployed properly and a
>>> test board (RPi4) booted without modules. In that case the board booted
>>> fine and there was no udev timeout. After deploying kernel modules, the
>>> udev timeout is back.
>> I'm confused now. Can you confirm that the problem is due to your
>> kernel modules, or do you think it's still due to 97ba62b27867? Or
>> fb6cc127e0b6 (this patch)?
>
> I don't use any custom kernel modules. I just deploy all modules that 
> are being built from the given kernel defconfig (arm 
> multi_v7_defconfig or arm64 default) and they are automatically loaded 
> during the boot by udev. I've checked again and bisect was right. The 
> kernel built from fb6cc127e0b6 suffers from the described issue, while 
> the one build from the previous commit (2e498d0a74e5) works fine.

I've managed to reproduce this issue with qemu. I've compiled the kernel 
for arm 32bit with multi_v7_defconfig and used some older Debian rootfs 
image. The log and qemu parameters are here: 
https://paste.debian.net/1194526/

Check the timestamp for the 'EXT4-fs (vda): re-mounted' message and 
'done (timeout)' status for the 'Waiting for /dev to be fully populated' 
message. This happens only when kernel modules build from the 
multi_v7_defconfig are deployed on the rootfs.

Best regards
Marco Elver April 21, 2021, 11:03 a.m. UTC | #8
On Wed, 21 Apr 2021 at 12:57, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 21.04.2021 11:35, Marek Szyprowski wrote:
> > On 21.04.2021 10:11, Marco Elver wrote:
> >> On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski
> >> <m.szyprowski@samsung.com> wrote:
> >>> On 21.04.2021 08:21, Marek Szyprowski wrote:
> >>>> On 21.04.2021 00:42, Marco Elver wrote:
> >>>>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>> 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?
> >>>> This happens with standard multi_v7_defconfig (arm) or just defconfig
> >>>> for arm64.
> >>>>
> >>>>> -- Also, can you share how you run this? Can it be reproduced in
> >>>>> qemu?
> >>>> Nothing special. I just boot my test systems and see that they are
> >>>> waiting lots of time during the udev populating /dev and network
> >>>> interfaces configuration. I didn't try with qemu yet.
> >>>>> -- How did you derive this patch to be at fault? Why not just
> >>>>> 97ba62b27867, given you also need to revert it?
> >>>> Well, I've just run my boot tests with automated 'git bisect' and that
> >>>> was its result. It was a bit late in the evening, so I didn't analyze
> >>>> it further, I've just posted a report about the issue I've found. It
> >>>> looks that bisecting pointed to a wrong commit somehow.
> >>>>> If you are unsure which patch exactly it is, can you try just
> >>>>> reverting 97ba62b27867 and see what happens?
> >>>> Indeed, this is a real faulty commit. Initially I've decided to revert
> >>>> it to let kernel compile (it uses some symbols introduced by this
> >>>> commit). Reverting only it on top of linux-next 20210420 also fixes
> >>>> the issue. I'm sorry for the noise in this thread. I hope we will find
> >>>> what really causes the issue.
> >>> This was a premature conclusion. It looks that during the test I've did
> >>> while writing that reply, the modules were not deployed properly and a
> >>> test board (RPi4) booted without modules. In that case the board booted
> >>> fine and there was no udev timeout. After deploying kernel modules, the
> >>> udev timeout is back.
> >> I'm confused now. Can you confirm that the problem is due to your
> >> kernel modules, or do you think it's still due to 97ba62b27867? Or
> >> fb6cc127e0b6 (this patch)?
> >
> > I don't use any custom kernel modules. I just deploy all modules that
> > are being built from the given kernel defconfig (arm
> > multi_v7_defconfig or arm64 default) and they are automatically loaded
> > during the boot by udev. I've checked again and bisect was right. The
> > kernel built from fb6cc127e0b6 suffers from the described issue, while
> > the one build from the previous commit (2e498d0a74e5) works fine.
>
> I've managed to reproduce this issue with qemu. I've compiled the kernel
> for arm 32bit with multi_v7_defconfig and used some older Debian rootfs
> image. The log and qemu parameters are here:
> https://paste.debian.net/1194526/
>
> Check the timestamp for the 'EXT4-fs (vda): re-mounted' message and
> 'done (timeout)' status for the 'Waiting for /dev to be fully populated'
> message. This happens only when kernel modules build from the
> multi_v7_defconfig are deployed on the rootfs.

Still hard to say what is going on and what is at fault. But being
able to repro this in qemu helps debug quicker -- would you also be
able to share the precise rootfs.img, i.e. upload it somewhere I can
fetch it? And just to be sure, please also share your .config, as it
might have compiler-version dependent configuration that might help
repro (unlikely, but you never know).

Thanks,
-- Marco
Marek Szyprowski April 21, 2021, 1:19 p.m. UTC | #9
Hi Marco,

On 21.04.2021 13:03, Marco Elver wrote:
> On Wed, 21 Apr 2021 at 12:57, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> On 21.04.2021 11:35, Marek Szyprowski wrote:
>>> On 21.04.2021 10:11, Marco Elver wrote:
>>>> On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> On 21.04.2021 08:21, Marek Szyprowski wrote:
>>>>>> On 21.04.2021 00:42, Marco Elver wrote:
>>>>>>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
>>>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>>>> 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?
>>>>>> This happens with standard multi_v7_defconfig (arm) or just defconfig
>>>>>> for arm64.
>>>>>>
>>>>>>> -- Also, can you share how you run this? Can it be reproduced in
>>>>>>> qemu?
>>>>>> Nothing special. I just boot my test systems and see that they are
>>>>>> waiting lots of time during the udev populating /dev and network
>>>>>> interfaces configuration. I didn't try with qemu yet.
>>>>>>> -- How did you derive this patch to be at fault? Why not just
>>>>>>> 97ba62b27867, given you also need to revert it?
>>>>>> Well, I've just run my boot tests with automated 'git bisect' and that
>>>>>> was its result. It was a bit late in the evening, so I didn't analyze
>>>>>> it further, I've just posted a report about the issue I've found. It
>>>>>> looks that bisecting pointed to a wrong commit somehow.
>>>>>>> If you are unsure which patch exactly it is, can you try just
>>>>>>> reverting 97ba62b27867 and see what happens?
>>>>>> Indeed, this is a real faulty commit. Initially I've decided to revert
>>>>>> it to let kernel compile (it uses some symbols introduced by this
>>>>>> commit). Reverting only it on top of linux-next 20210420 also fixes
>>>>>> the issue. I'm sorry for the noise in this thread. I hope we will find
>>>>>> what really causes the issue.
>>>>> This was a premature conclusion. It looks that during the test I've did
>>>>> while writing that reply, the modules were not deployed properly and a
>>>>> test board (RPi4) booted without modules. In that case the board booted
>>>>> fine and there was no udev timeout. After deploying kernel modules, the
>>>>> udev timeout is back.
>>>> I'm confused now. Can you confirm that the problem is due to your
>>>> kernel modules, or do you think it's still due to 97ba62b27867? Or
>>>> fb6cc127e0b6 (this patch)?
>>> I don't use any custom kernel modules. I just deploy all modules that
>>> are being built from the given kernel defconfig (arm
>>> multi_v7_defconfig or arm64 default) and they are automatically loaded
>>> during the boot by udev. I've checked again and bisect was right. The
>>> kernel built from fb6cc127e0b6 suffers from the described issue, while
>>> the one build from the previous commit (2e498d0a74e5) works fine.
>> I've managed to reproduce this issue with qemu. I've compiled the kernel
>> for arm 32bit with multi_v7_defconfig and used some older Debian rootfs
>> image. The log and qemu parameters are here:
>> https://protect2.fireeye.com/v1/url?k=7cfc23a2-23671aa9-7cfda8ed-002590f5b904-dab7e2ec39dae1f9&q=1&e=36a5ed13-6ad5-430c-8f44-e95c4f0af5c3&u=https%3A%2F%2Fpaste.debian.net%2F1194526%2F
>>
>> Check the timestamp for the 'EXT4-fs (vda): re-mounted' message and
>> 'done (timeout)' status for the 'Waiting for /dev to be fully populated'
>> message. This happens only when kernel modules build from the
>> multi_v7_defconfig are deployed on the rootfs.
> Still hard to say what is going on and what is at fault. But being
> able to repro this in qemu helps debug quicker -- would you also be
> able to share the precise rootfs.img, i.e. upload it somewhere I can
> fetch it? And just to be sure, please also share your .config, as it
> might have compiler-version dependent configuration that might help
> repro (unlikely, but you never know).

I've managed to reproduce this issue with a public Raspberry Pi OS Lite 
rootfs image, even without deploying kernel modules:

https://downloads.raspberrypi.org/raspios_lite_armhf/images/raspios_lite_armhf-2021-03-25/2021-03-04-raspios-buster-armhf-lite.zip

# qemu-system-arm -M virt -smp 2 -m 512 -kernel zImage -append "earlycon 
console=ttyAMA0 root=/dev/vda2 rw rootwait" -serial stdio -display none 
-monitor null -device virtio-blk-device,drive=virtio-blk -drive 
file=/tmp/2021-03-04-raspios-buster-armhf-lite.img,id=virtio-blk,if=none,format=raw 
-netdev user,id=user -device virtio-net-device,netdev=user

The above one doesn't boot if zImage z compiled from commit fb6cc127e0b6 
and boots if compiled from 2e498d0a74e5. In both cases I've used default 
arm/multi_v7_defconfig and 
gcc-linaro-6.4.1-2017.11-x86_64_arm-linux-gnueabi toolchain.

Best regards
Jon Hunter April 21, 2021, 3:07 p.m. UTC | #10
Hello!

On 08/04/2021 11: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>


Since next-20210420 I have noticed a boot regression on all 32-bit Tegra
that we are testing. Bisect is pointing to this commit and reverting
this patch and patch 6/10 does resolve the issue.

Interestingly there is no apparent crash, but these systems just appear
to hang silently after mounting the rootfs. If anyone has any thoughts
let me know!

Thanks
Jon
Marco Elver April 21, 2021, 3:11 p.m. UTC | #11
+Cc linux-arm-kernel

On Wed, 21 Apr 2021 at 15:19, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Marco,
>
> On 21.04.2021 13:03, Marco Elver wrote:
> > On Wed, 21 Apr 2021 at 12:57, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> On 21.04.2021 11:35, Marek Szyprowski wrote:
> >>> On 21.04.2021 10:11, Marco Elver wrote:
> >>>> On Wed, 21 Apr 2021 at 09:35, Marek Szyprowski
> >>>> <m.szyprowski@samsung.com> wrote:
> >>>>> On 21.04.2021 08:21, Marek Szyprowski wrote:
> >>>>>> On 21.04.2021 00:42, Marco Elver wrote:
> >>>>>>> On Tue, 20 Apr 2021 at 23:26, Marek Szyprowski
> >>>>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>>>> 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?
> >>>>>> This happens with standard multi_v7_defconfig (arm) or just defconfig
> >>>>>> for arm64.
> >>>>>>
> >>>>>>> -- Also, can you share how you run this? Can it be reproduced in
> >>>>>>> qemu?
> >>>>>> Nothing special. I just boot my test systems and see that they are
> >>>>>> waiting lots of time during the udev populating /dev and network
> >>>>>> interfaces configuration. I didn't try with qemu yet.
> >>>>>>> -- How did you derive this patch to be at fault? Why not just
> >>>>>>> 97ba62b27867, given you also need to revert it?
> >>>>>> Well, I've just run my boot tests with automated 'git bisect' and that
> >>>>>> was its result. It was a bit late in the evening, so I didn't analyze
> >>>>>> it further, I've just posted a report about the issue I've found. It
> >>>>>> looks that bisecting pointed to a wrong commit somehow.
> >>>>>>> If you are unsure which patch exactly it is, can you try just
> >>>>>>> reverting 97ba62b27867 and see what happens?
> >>>>>> Indeed, this is a real faulty commit. Initially I've decided to revert
> >>>>>> it to let kernel compile (it uses some symbols introduced by this
> >>>>>> commit). Reverting only it on top of linux-next 20210420 also fixes
> >>>>>> the issue. I'm sorry for the noise in this thread. I hope we will find
> >>>>>> what really causes the issue.
> >>>>> This was a premature conclusion. It looks that during the test I've did
> >>>>> while writing that reply, the modules were not deployed properly and a
> >>>>> test board (RPi4) booted without modules. In that case the board booted
> >>>>> fine and there was no udev timeout. After deploying kernel modules, the
> >>>>> udev timeout is back.
> >>>> I'm confused now. Can you confirm that the problem is due to your
> >>>> kernel modules, or do you think it's still due to 97ba62b27867? Or
> >>>> fb6cc127e0b6 (this patch)?
> >>> I don't use any custom kernel modules. I just deploy all modules that
> >>> are being built from the given kernel defconfig (arm
> >>> multi_v7_defconfig or arm64 default) and they are automatically loaded
> >>> during the boot by udev. I've checked again and bisect was right. The
> >>> kernel built from fb6cc127e0b6 suffers from the described issue, while
> >>> the one build from the previous commit (2e498d0a74e5) works fine.
> >> I've managed to reproduce this issue with qemu. I've compiled the kernel
> >> for arm 32bit with multi_v7_defconfig and used some older Debian rootfs
> >> image. The log and qemu parameters are here:
> >> https://protect2.fireeye.com/v1/url?k=7cfc23a2-23671aa9-7cfda8ed-002590f5b904-dab7e2ec39dae1f9&q=1&e=36a5ed13-6ad5-430c-8f44-e95c4f0af5c3&u=https%3A%2F%2Fpaste.debian.net%2F1194526%2F
> >>
> >> Check the timestamp for the 'EXT4-fs (vda): re-mounted' message and
> >> 'done (timeout)' status for the 'Waiting for /dev to be fully populated'
> >> message. This happens only when kernel modules build from the
> >> multi_v7_defconfig are deployed on the rootfs.
> > Still hard to say what is going on and what is at fault. But being
> > able to repro this in qemu helps debug quicker -- would you also be
> > able to share the precise rootfs.img, i.e. upload it somewhere I can
> > fetch it? And just to be sure, please also share your .config, as it
> > might have compiler-version dependent configuration that might help
> > repro (unlikely, but you never know).
>
> I've managed to reproduce this issue with a public Raspberry Pi OS Lite
> rootfs image, even without deploying kernel modules:
>
> https://downloads.raspberrypi.org/raspios_lite_armhf/images/raspios_lite_armhf-2021-03-25/2021-03-04-raspios-buster-armhf-lite.zip
>
> # qemu-system-arm -M virt -smp 2 -m 512 -kernel zImage -append "earlycon
> console=ttyAMA0 root=/dev/vda2 rw rootwait" -serial stdio -display none
> -monitor null -device virtio-blk-device,drive=virtio-blk -drive
> file=/tmp/2021-03-04-raspios-buster-armhf-lite.img,id=virtio-blk,if=none,format=raw
> -netdev user,id=user -device virtio-net-device,netdev=user
>
> The above one doesn't boot if zImage z compiled from commit fb6cc127e0b6
> and boots if compiled from 2e498d0a74e5. In both cases I've used default
> arm/multi_v7_defconfig and
> gcc-linaro-6.4.1-2017.11-x86_64_arm-linux-gnueabi toolchain.

Yup, I've narrowed it down to the addition of "__u64 _perf" to
siginfo_t. My guess is the __u64 causes a different alignment for a
bunch of adjacent fields. It seems that x86 and m68k are the only ones
that have compile-time tests for the offsets. Arm should probably add
those -- I have added a bucket of static_assert() in
arch/arm/kernel/signal.c and see that something's off.

I'll hopefully have a fix in a day or so.

Thanks,
-- Marco
Marco Elver April 21, 2021, 4:27 p.m. UTC | #12
On Wed, Apr 21, 2021 at 05:11PM +0200, Marco Elver wrote:
> +Cc linux-arm-kernel
> 
[...]
> >
> > I've managed to reproduce this issue with a public Raspberry Pi OS Lite
> > rootfs image, even without deploying kernel modules:
> >
> > https://downloads.raspberrypi.org/raspios_lite_armhf/images/raspios_lite_armhf-2021-03-25/2021-03-04-raspios-buster-armhf-lite.zip
> >
> > # qemu-system-arm -M virt -smp 2 -m 512 -kernel zImage -append "earlycon
> > console=ttyAMA0 root=/dev/vda2 rw rootwait" -serial stdio -display none
> > -monitor null -device virtio-blk-device,drive=virtio-blk -drive
> > file=/tmp/2021-03-04-raspios-buster-armhf-lite.img,id=virtio-blk,if=none,format=raw
> > -netdev user,id=user -device virtio-net-device,netdev=user
> >
> > The above one doesn't boot if zImage z compiled from commit fb6cc127e0b6
> > and boots if compiled from 2e498d0a74e5. In both cases I've used default
> > arm/multi_v7_defconfig and
> > gcc-linaro-6.4.1-2017.11-x86_64_arm-linux-gnueabi toolchain.
> 
> Yup, I've narrowed it down to the addition of "__u64 _perf" to
> siginfo_t. My guess is the __u64 causes a different alignment for a
> bunch of adjacent fields. It seems that x86 and m68k are the only ones
> that have compile-time tests for the offsets. Arm should probably add
> those -- I have added a bucket of static_assert() in
> arch/arm/kernel/signal.c and see that something's off.
> 
> I'll hopefully have a fix in a day or so.

Arm and compiler folks: are there some special alignment requirement for
__u64 on arm 32-bit? (And if there is for arm64, please shout as well.)

With the static-asserts below, the only thing that I can do to fix it is
to completely remove the __u64. Padding it before or after with __u32
just does not work. It seems that the use of __u64 shifts everything
in __sifields by 4 bytes.

diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index d0bb9125c853..b02a4ac55938 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -92,7 +92,10 @@ union __sifields {
 				__u32 _pkey;
 			} _addr_pkey;
 			/* used when si_code=TRAP_PERF */
-			__u64 _perf;
+			struct {
+				__u32 _perf1;
+				__u32 _perf2;
+			} _perf;
 		};
 	} _sigfault;

^^ works, but I'd hate to have to split this into 2 __u32 because it
makes the whole design worse.

What alignment trick do we have to do here to fix it for __u64?


------ >8 ------

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index a3a38d0a4c85..6c558dc314c3 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -725,3 +725,41 @@ asmlinkage void do_rseq_syscall(struct pt_regs *regs)
 	rseq_syscall(regs);
 }
 #endif
+
+/*
+ * Compile-time tests for siginfo_t offsets. Changes to NSIG* likely come with
+ * new fields; new fields should be added below.
+ */
+static_assert(NSIGILL	== 11);
+static_assert(NSIGFPE	== 15);
+static_assert(NSIGSEGV	== 9);
+static_assert(NSIGBUS	== 5);
+static_assert(NSIGTRAP	== 6);
+static_assert(NSIGCHLD	== 6);
+static_assert(NSIGSYS	== 2);
+static_assert(offsetof(siginfo_t, si_signo)	== 0x00);
+static_assert(offsetof(siginfo_t, si_errno)	== 0x04);
+static_assert(offsetof(siginfo_t, si_code)	== 0x08);
+static_assert(offsetof(siginfo_t, si_pid)	== 0x0c);
+#if 0
+static_assert(offsetof(siginfo_t, si_uid)	== 0x10);
+static_assert(offsetof(siginfo_t, si_tid)	== 0x0c);
+static_assert(offsetof(siginfo_t, si_overrun)	== 0x10);
+static_assert(offsetof(siginfo_t, si_status)	== 0x14);
+static_assert(offsetof(siginfo_t, si_utime)	== 0x18);
+static_assert(offsetof(siginfo_t, si_stime)	== 0x1c);
+static_assert(offsetof(siginfo_t, si_value)	== 0x14);
+static_assert(offsetof(siginfo_t, si_int)	== 0x14);
+static_assert(offsetof(siginfo_t, si_ptr)	== 0x14);
+static_assert(offsetof(siginfo_t, si_addr)	== 0x0c);
+static_assert(offsetof(siginfo_t, si_addr_lsb)	== 0x10);
+static_assert(offsetof(siginfo_t, si_lower)	== 0x14);
+static_assert(offsetof(siginfo_t, si_upper)	== 0x18);
+static_assert(offsetof(siginfo_t, si_pkey)	== 0x14);
+static_assert(offsetof(siginfo_t, si_perf)	== 0x10);
+static_assert(offsetof(siginfo_t, si_band)	== 0x0c);
+static_assert(offsetof(siginfo_t, si_fd)	== 0x10);
+static_assert(offsetof(siginfo_t, si_call_addr)	== 0x0c);
+static_assert(offsetof(siginfo_t, si_syscall)	== 0x10);
+static_assert(offsetof(siginfo_t, si_arch)	== 0x14);
+#endif
Marco Elver April 21, 2021, 6:23 p.m. UTC | #13
On Wed, Apr 21, 2021 at 06:27PM +0200, Marco Elver wrote:
> On Wed, Apr 21, 2021 at 05:11PM +0200, Marco Elver wrote:
> > +Cc linux-arm-kernel
> > 
> [...]
> > >
> > > I've managed to reproduce this issue with a public Raspberry Pi OS Lite
> > > rootfs image, even without deploying kernel modules:
> > >
> > > https://downloads.raspberrypi.org/raspios_lite_armhf/images/raspios_lite_armhf-2021-03-25/2021-03-04-raspios-buster-armhf-lite.zip
> > >
> > > # qemu-system-arm -M virt -smp 2 -m 512 -kernel zImage -append "earlycon
> > > console=ttyAMA0 root=/dev/vda2 rw rootwait" -serial stdio -display none
> > > -monitor null -device virtio-blk-device,drive=virtio-blk -drive
> > > file=/tmp/2021-03-04-raspios-buster-armhf-lite.img,id=virtio-blk,if=none,format=raw
> > > -netdev user,id=user -device virtio-net-device,netdev=user
> > >
> > > The above one doesn't boot if zImage z compiled from commit fb6cc127e0b6
> > > and boots if compiled from 2e498d0a74e5. In both cases I've used default
> > > arm/multi_v7_defconfig and
> > > gcc-linaro-6.4.1-2017.11-x86_64_arm-linux-gnueabi toolchain.
> > 
> > Yup, I've narrowed it down to the addition of "__u64 _perf" to
> > siginfo_t. My guess is the __u64 causes a different alignment for a
> > bunch of adjacent fields. It seems that x86 and m68k are the only ones
> > that have compile-time tests for the offsets. Arm should probably add
> > those -- I have added a bucket of static_assert() in
> > arch/arm/kernel/signal.c and see that something's off.
> > 
> > I'll hopefully have a fix in a day or so.
> 
> Arm and compiler folks: are there some special alignment requirement for
> __u64 on arm 32-bit? (And if there is for arm64, please shout as well.)
> 
> With the static-asserts below, the only thing that I can do to fix it is
> to completely remove the __u64. Padding it before or after with __u32
> just does not work. It seems that the use of __u64 shifts everything
> in __sifields by 4 bytes.
> 
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index d0bb9125c853..b02a4ac55938 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -92,7 +92,10 @@ union __sifields {
>  				__u32 _pkey;
>  			} _addr_pkey;
>  			/* used when si_code=TRAP_PERF */
> -			__u64 _perf;
> +			struct {
> +				__u32 _perf1;
> +				__u32 _perf2;
> +			} _perf;
>  		};
>  	} _sigfault;
> 
> ^^ works, but I'd hate to have to split this into 2 __u32 because it
> makes the whole design worse.
> 
> What alignment trick do we have to do here to fix it for __u64?

So I think we just have to settle on 'unsigned long' here. On many
architectures, like 32-bit Arm, the alignment of a structure is that of
its largest member. This means that there is no portable way to add
64-bit integers to siginfo_t on 32-bit architectures.

In the case of the si_perf field, word size is sufficient since the data
it contains is user-defined. On 32-bit architectures, any excess bits of
perf_event_attr::sig_data will therefore be truncated when copying into
si_perf.

Feel free to test the below if you have time, but the below lets me boot
32-bit arm which previously timed out. It also passes all the
static_asserts() I added (will send those as separate patches).

Once I'm convinced this passes all others tests too, I'll send a patch.

Thanks,
-- Marco


diff --git a/include/linux/compat.h b/include/linux/compat.h
index c8821d966812..f0d2dd35d408 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -237,7 +237,7 @@ typedef struct compat_siginfo {
 					u32 _pkey;
 				} _addr_pkey;
 				/* used when si_code=TRAP_PERF */
-				compat_u64 _perf;
+				compat_ulong_t _perf;
 			};
 		} _sigfault;
 
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index d0bb9125c853..03d6f6d2c1fe 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -92,7 +92,7 @@ union __sifields {
 				__u32 _pkey;
 			} _addr_pkey;
 			/* used when si_code=TRAP_PERF */
-			__u64 _perf;
+			unsigned long _perf;
 		};
 	} _sigfault;
Marek Szyprowski April 22, 2021, 6:12 a.m. UTC | #14
Hi Marco,

On 21.04.2021 20:23, Marco Elver wrote:
> On Wed, Apr 21, 2021 at 06:27PM +0200, Marco Elver wrote:
>> On Wed, Apr 21, 2021 at 05:11PM +0200, Marco Elver wrote:
>>> +Cc linux-arm-kernel
>>>
>> [...]
>>>> I've managed to reproduce this issue with a public Raspberry Pi OS Lite
>>>> rootfs image, even without deploying kernel modules:
>>>>
>>>> https://downloads.raspberrypi.org/raspios_lite_armhf/images/raspios_lite_armhf-2021-03-25/2021-03-04-raspios-buster-armhf-lite.zip
>>>>
>>>> # qemu-system-arm -M virt -smp 2 -m 512 -kernel zImage -append "earlycon
>>>> console=ttyAMA0 root=/dev/vda2 rw rootwait" -serial stdio -display none
>>>> -monitor null -device virtio-blk-device,drive=virtio-blk -drive
>>>> file=/tmp/2021-03-04-raspios-buster-armhf-lite.img,id=virtio-blk,if=none,format=raw
>>>> -netdev user,id=user -device virtio-net-device,netdev=user
>>>>
>>>> The above one doesn't boot if zImage z compiled from commit fb6cc127e0b6
>>>> and boots if compiled from 2e498d0a74e5. In both cases I've used default
>>>> arm/multi_v7_defconfig and
>>>> gcc-linaro-6.4.1-2017.11-x86_64_arm-linux-gnueabi toolchain.
>>> Yup, I've narrowed it down to the addition of "__u64 _perf" to
>>> siginfo_t. My guess is the __u64 causes a different alignment for a
>>> bunch of adjacent fields. It seems that x86 and m68k are the only ones
>>> that have compile-time tests for the offsets. Arm should probably add
>>> those -- I have added a bucket of static_assert() in
>>> arch/arm/kernel/signal.c and see that something's off.
>>>
>>> I'll hopefully have a fix in a day or so.
>> Arm and compiler folks: are there some special alignment requirement for
>> __u64 on arm 32-bit? (And if there is for arm64, please shout as well.)
>>
>> With the static-asserts below, the only thing that I can do to fix it is
>> to completely remove the __u64. Padding it before or after with __u32
>> just does not work. It seems that the use of __u64 shifts everything
>> in __sifields by 4 bytes.
>>
>> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
>> index d0bb9125c853..b02a4ac55938 100644
>> --- a/include/uapi/asm-generic/siginfo.h
>> +++ b/include/uapi/asm-generic/siginfo.h
>> @@ -92,7 +92,10 @@ union __sifields {
>>   				__u32 _pkey;
>>   			} _addr_pkey;
>>   			/* used when si_code=TRAP_PERF */
>> -			__u64 _perf;
>> +			struct {
>> +				__u32 _perf1;
>> +				__u32 _perf2;
>> +			} _perf;
>>   		};
>>   	} _sigfault;
>>
>> ^^ works, but I'd hate to have to split this into 2 __u32 because it
>> makes the whole design worse.
>>
>> What alignment trick do we have to do here to fix it for __u64?
> So I think we just have to settle on 'unsigned long' here. On many
> architectures, like 32-bit Arm, the alignment of a structure is that of
> its largest member. This means that there is no portable way to add
> 64-bit integers to siginfo_t on 32-bit architectures.
>
> In the case of the si_perf field, word size is sufficient since the data
> it contains is user-defined. On 32-bit architectures, any excess bits of
> perf_event_attr::sig_data will therefore be truncated when copying into
> si_perf.
>
> Feel free to test the below if you have time, but the below lets me boot
> 32-bit arm which previously timed out. It also passes all the
> static_asserts() I added (will send those as separate patches).
>
> Once I'm convinced this passes all others tests too, I'll send a patch.

This fixes the issue I've observed on my test systems. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> Thanks,
> -- Marco
>
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index c8821d966812..f0d2dd35d408 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -237,7 +237,7 @@ typedef struct compat_siginfo {
>   					u32 _pkey;
>   				} _addr_pkey;
>   				/* used when si_code=TRAP_PERF */
> -				compat_u64 _perf;
> +				compat_ulong_t _perf;
>   			};
>   		} _sigfault;
>   
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index d0bb9125c853..03d6f6d2c1fe 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -92,7 +92,7 @@ union __sifields {
>   				__u32 _pkey;
>   			} _addr_pkey;
>   			/* used when si_code=TRAP_PERF */
> -			__u64 _perf;
> +			unsigned long _perf;
>   		};
>   	} _sigfault;
>   
>
Best regards
Marco Elver April 22, 2021, 6:47 a.m. UTC | #15
On Thu, 22 Apr 2021 at 08:12, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
[...]
> > So I think we just have to settle on 'unsigned long' here. On many
> > architectures, like 32-bit Arm, the alignment of a structure is that of
> > its largest member. This means that there is no portable way to add
> > 64-bit integers to siginfo_t on 32-bit architectures.
> >
> > In the case of the si_perf field, word size is sufficient since the data
> > it contains is user-defined. On 32-bit architectures, any excess bits of
> > perf_event_attr::sig_data will therefore be truncated when copying into
> > si_perf.
> >
> > Feel free to test the below if you have time, but the below lets me boot
> > 32-bit arm which previously timed out. It also passes all the
> > static_asserts() I added (will send those as separate patches).
> >
> > Once I'm convinced this passes all others tests too, I'll send a patch.
>
> This fixes the issue I've observed on my test systems. Feel free to add:
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thank you for testing! It's been sent:
https://lkml.kernel.org/r/20210422064437.3577327-1-elver@google.com

Thanks,
-- Marco
Jon Hunter April 22, 2021, 8:16 a.m. UTC | #16
On 22/04/2021 07:47, Marco Elver wrote:
> On Thu, 22 Apr 2021 at 08:12, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> [...]
>>> So I think we just have to settle on 'unsigned long' here. On many
>>> architectures, like 32-bit Arm, the alignment of a structure is that of
>>> its largest member. This means that there is no portable way to add
>>> 64-bit integers to siginfo_t on 32-bit architectures.
>>>
>>> In the case of the si_perf field, word size is sufficient since the data
>>> it contains is user-defined. On 32-bit architectures, any excess bits of
>>> perf_event_attr::sig_data will therefore be truncated when copying into
>>> si_perf.
>>>
>>> Feel free to test the below if you have time, but the below lets me boot
>>> 32-bit arm which previously timed out. It also passes all the
>>> static_asserts() I added (will send those as separate patches).
>>>
>>> Once I'm convinced this passes all others tests too, I'll send a patch.
>>
>> This fixes the issue I've observed on my test systems. Feel free to add:
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Thank you for testing! It's been sent:
> https://lkml.kernel.org/r/20210422064437.3577327-1-elver@google.com


Thanks! This fixes the problem for Tegra as well. I have responded to
the above patch with my tested-by.

Cheers
Jon
Alexander Egorenkov April 26, 2021, 7:35 a.m. UTC | #17
Hi,

this also fixes s390.
strace's tests-m32 on s390 were failing.

Regards
Alex

Patch
diff mbox series

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);