On Fri, 30 Apr 2021 at 22:15, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: [...] > arm64 only abuses si_errno in compat code for bug compatibility with > arm32. > > > Given it'd be wasted space otherwise, and we define the semantics of > > whatever is stored in siginfo on the new signal, it'd be good to keep. > > Except you don't completely. You are not defining a new signal. You > are extending the definition of SIGTRAP. Anything generic that > responds to all SIGTRAPs can reasonably be looking at si_errno. I see where you're coming from, and agree with this if si_errno already had some semantics for some subset of SIGTRAPs. I've tried to analyze the situation a bit further, since siginfo seems to be a giant minefield and semantics is underspecified at best. :-) Do any of the existing SIGTRAPs define si_errno to be set? As far as I can tell, they don't. If this is true, I think there are benefits and downsides to repurposing si_errno (similar to what SIGSYS did). The obvious downside is as you suggest, it's not always a real errno. The benefit is that we avoid introducing more and more fields -- i.e. if we permit si_errno to be repurposed for SIGTRAP and its value depends on the precise si_code, too, we simplify siginfo's overall definition (also given every new field needs more code in kernel/signal.c, too). Given SIGTRAPs are in response to some user-selected event in the user's code (breakpoints, ptrace, etc. ... now perf events), the user must already check the si_code to select the right action because SIGTRAPs are not alike (unlike, e.g. SIGSEGV). Because of this, I think that repurposing si_errno in an si_code-dependent way for SIGTRAPs is safe. If you think it is simply untenable to repurpose si_errno for SIGTRAPs, please confirm -- I'll just send a minimal patch to fix (I'd probably just remove setting it... everything else is too intrusive as a "fix".. sigh). The cleanups as you outline below seem orthogonal and not urgent for 5.13 (all changes and cleanups for __ARCH_SI_TRAPNO seem too intrusive without -next exposure). Thanks, -- Marco > Further you are already adding a field with si_perf you can just as > easily add a second field with well defined semantics for that data. > > >> The code is only safe if the analysis that says we can move si_trapno > >> and perhaps the ia64 fields into the union is correct. It looks like > >> ia64 much more actively uses it's signal extension fields including for > >> SIGTRAP, so I am not at all certain the generic definition of > >> perf_sigtrap is safe on ia64. > > > > Trying to understand the requirements of si_trapno myself: safe here > > would mean that si_trapno is not required if we fire our SIGTRAP / > > TRAP_PERF. > > > > As far as I can tell that is the case -- see below. > > > >> > I suppose in theory sparc64 or alpha might start using the other > >> > fields in the future, and an application might be compiled against > >> > mismatched headers, but that is unlikely and is already broken > >> > with the current headers. > >> > >> If we localize the use of si_trapno to just a few special cases on alpha > >> and sparc I think we don't even need to worry about breaking userspace > >> on any architecture. It will complicate siginfo_layout, but it is a > >> complication that reflects reality. > >> > >> I don't have a clue how any of this affects ia64. Does perf work on > >> ia64? Does perf work on sparc, and alpha? > >> > >> If perf works on ia64 we need to take a hard look at what is going on > >> there as well. > > > > No perf on ia64, but it seems alpha and sparc have perf: > > > > $ git grep 'select.*HAVE_PERF_EVENTS$' -- arch/ > > arch/alpha/Kconfig: select HAVE_PERF_EVENTS <-- > > arch/arc/Kconfig: select HAVE_PERF_EVENTS > > arch/arm/Kconfig: select HAVE_PERF_EVENTS > > arch/arm64/Kconfig: select HAVE_PERF_EVENTS > > arch/csky/Kconfig: select HAVE_PERF_EVENTS > > arch/hexagon/Kconfig: select HAVE_PERF_EVENTS > > arch/mips/Kconfig: select HAVE_PERF_EVENTS > > arch/nds32/Kconfig: select HAVE_PERF_EVENTS > > arch/parisc/Kconfig: select HAVE_PERF_EVENTS > > arch/powerpc/Kconfig: select HAVE_PERF_EVENTS > > arch/riscv/Kconfig: select HAVE_PERF_EVENTS > > arch/s390/Kconfig: select HAVE_PERF_EVENTS > > arch/sh/Kconfig: select HAVE_PERF_EVENTS > > arch/sparc/Kconfig: select HAVE_PERF_EVENTS <-- > > arch/x86/Kconfig: select HAVE_PERF_EVENTS > > arch/xtensa/Kconfig: select HAVE_PERF_EVENTS > > > > Now, given ia64 is not an issue, I wanted to understand the semantics of > > si_trapno. Per https://man7.org/linux/man-pages/man2/sigaction.2.html, I > > see: > > > > int si_trapno; /* Trap number that caused > > hardware-generated signal > > (unused on most architectures) */ > > > > ... its intended semantics seem to suggest it would only be used by some > > architecture-specific signal like you identified above. So if the > > semantics is some code of a hardware trap/fault, then we're fine and do > > not need to set it. > > > > Also bearing in mind we define the semantics any new signal, and given > > most architectures do not have si_trapno, definitions of new generic > > signals should probably not include odd architecture specific details > > related to old architectures. > > > > From all this, my understanding now is that we can move si_trapno into > > the union, correct? What else did you have in mind? > > Yes. Let's move si_trapno into the union. > > That implies a few things like siginfo_layout needs to change. > > The helpers in kernel/signal.c can change to not imply that > if you define __ARCH_SI_TRAPNO you must always define and > pass in si_trapno. A force_sig_trapno could be defined instead > to handle the cases that alpha and sparc use si_trapno. > > It would be nice if a force_sig_perf_trap could be factored > out of perf_trap and placed in kernel/signal.c. > > My experience (especially this round) is that it becomes much easier to > audit the users of siginfo if there is a dedicated function in > kernel/signal.c that is simply passed the parameters that need > to be placed in siginfo. > > So I would very much like to see if I can make force_sig_info static. > > Eric >
