Linus Torvalds [off-list ref] writes:

On Tue, Jul 18, 2017 at 7:06 AM, Eric W. Biederman
[off-list ref] wrote:

quoted

struct siginfo is a union and the kernel since 2.4 has been hiding a union
tag in the high 16bits of si_code using the values:
__SI_KILL
__SI_TIMER
__SI_POLL
__SI_FAULT
__SI_CHLD
__SI_RT
__SI_MESGQ
__SI_SYS

While this looks plausible on the surface, in practice this situation has
not worked well.

So on the whole I think we just need to do this, but the part I really
hate about this series is still this the siginfo_layout() part.

I can well believe that it is needed for the compat case. siginfo is a
piece of crap crazy type, and re-ordering fields for compat is
something we are always going to have to do.

But for the native case, the *only* reason we do not just copy the
siginfo as-is seems to be that it's just too big, due to other bad
design decisions in siginfo ("let's make sure it's big enough by
allocating 512 bytes for it).

And afaik, absolutely nobody uses more than about 36 bytes of that
512-byte _sifields union (and that one use is SIGILL with three
pointers and three integers and some padding.

So why don't we just say "screw this idiotic layout crap, and just
unconditionally copy that much smaller maximum of bytes"?

Leave that layout thing purely for compat handling.

I completely agree.

Yes, yes, there's a couple of small gotchas's:

 - "_sys_private" for posix timers, and it would have to be moved to
the end of the structure so that it doesn't get copied.

I don't think we actually need _sys_private at all.

I think the best solution would involve embedding struct siginfo
into struct k_itimer (as we always allocate one).   Then we can just
perform container_of on the siginfo and look at the k_itimer instead.

 - make sure those 36 bytes are cleared when allocating the siginfo
(this should be trivial) so that we don't leak any other memory.

But on the whole, it looks pretty straightforward to just get rid of
those stupid layout things, and make them purely about compat stuff.

Please?

The si_code stuff clearly needs to be done regardless, so much of this
patch series looks good to me.  But if we're doign this cleanup, can't
we please go that one extra step and get rid of the crazy "let's treat
the union as different types", and just treat it as a largely opaque
thing.

Pretty please?

That is my next step.

I have started on it but it is a big additional patch.  I have to insert
a bunch of memsets to ensure we are not copying unitialized stack
contents to userspace.

I have been convinced not to expect any performance issues:
  - Worst case two reads from memory 60ns*2 = 120ns.
  - 650ns time to send a signal.
  - 350ns time to receive a signal.
  So that is maybe a 10% change, and more likely lost completely
  in the noise.

I intend to measure the performance change just copying it all to see if
I even need to optimize to just copy the needed 36 bytes.

The diffstat for introducing a clear_siginfo to ensure we have made
those memsets is huge so I am worried about introducing bugs along
the way or missing something.

 arch/alpha/kernel/osf_sys.c               |  1 +
 arch/alpha/kernel/signal.c                |  2 ++
 arch/alpha/kernel/traps.c                 |  5 ++++
 arch/alpha/mm/fault.c                     |  2 ++
 arch/arc/kernel/traps.c                   | 14 ++++++----
 arch/arc/mm/fault.c                       |  1 +
 arch/arm/kernel/ptrace.c                  |  2 ++
 arch/arm/kernel/swp_emulate.c             |  1 +
 arch/arm/kernel/traps.c                   |  5 ++++
 arch/arm/mm/alignment.c                   |  1 +
 arch/arm/mm/fault.c                       |  3 ++
 arch/arm/vfp/vfpmodule.c                  |  2 +-
 arch/arm64/kernel/debug-monitors.c        | 13 +++++----
 arch/arm64/kernel/fpsimd.c                |  2 +-
 arch/arm64/kernel/ptrace.c                | 13 +++++----
 arch/arm64/kernel/traps.c                 |  2 ++
 arch/arm64/mm/fault.c                     |  4 +++
 arch/blackfin/kernel/traps.c              |  1 +
 arch/c6x/kernel/traps.c                   |  1 +
 arch/cris/mm/fault.c                      |  1 +
 arch/frv/kernel/traps.c                   |  7 +++++
 arch/frv/mm/fault.c                       |  1 +
 arch/hexagon/kernel/traps.c               |  1 +
 arch/hexagon/mm/vm_fault.c                |  2 ++
 arch/ia64/kernel/brl_emu.c                |  3 ++
 arch/ia64/kernel/signal.c                 |  2 ++
 arch/ia64/kernel/traps.c                  |  3 +-
 arch/ia64/kernel/unaligned.c              |  1 +
 arch/ia64/mm/fault.c                      |  1 +
 arch/m32r/kernel/traps.c                  |  1 +
 arch/m32r/mm/fault.c                      |  1 +
 arch/m68k/kernel/traps.c                  |  2 ++
 arch/m68k/mm/fault.c                      |  3 +-
 arch/metag/kernel/traps.c                 |  2 ++
 arch/metag/mm/fault.c                     |  2 ++
 arch/microblaze/kernel/exceptions.c       |  1 +
 arch/microblaze/mm/fault.c                |  1 +
 arch/mips/kernel/traps.c                  | 29 +++++++++++++------
 arch/mips/mm/fault.c                      |  1 +
 arch/mn10300/kernel/fpu.c                 |  1 +
 arch/mn10300/kernel/traps.c               |  1 +
 arch/mn10300/mm/fault.c                   |  1 +
 arch/mn10300/mm/misalignment.c            |  2 ++
 arch/nios2/kernel/traps.c                 |  1 +
 arch/openrisc/kernel/traps.c              |  5 +++-
 arch/openrisc/mm/fault.c                  |  1 +
 arch/parisc/kernel/ptrace.c               |  1 +
 arch/parisc/kernel/traps.c                |  2 ++
 arch/parisc/kernel/unaligned.c            |  2 ++
 arch/parisc/math-emu/driver.c             |  1 +
 arch/parisc/mm/fault.c                    |  1 +
 arch/powerpc/kernel/process.c             |  2 ++
 arch/powerpc/kernel/traps.c               |  4 +--
 arch/powerpc/mm/fault.c                   |  1 +
 arch/powerpc/platforms/cell/spufs/fault.c |  2 +-
 arch/s390/kernel/traps.c                  |  3 ++
 arch/s390/mm/fault.c                      |  2 ++
 arch/score/kernel/traps.c                 |  1 +
 arch/score/mm/fault.c                     |  1 +
 arch/sh/kernel/hw_breakpoint.c            |  1 +
 arch/sh/kernel/traps_32.c                 |  4 +++
 arch/sh/math-emu/math.c                   |  1 +
 arch/sh/mm/fault.c                        |  1 +
 arch/sparc/kernel/process_64.c            |  1 +
 arch/sparc/kernel/sys_sparc_32.c          |  1 +
 arch/sparc/kernel/sys_sparc_64.c          |  1 +
 arch/sparc/kernel/traps_32.c              | 10 +++++++
 arch/sparc/kernel/traps_64.c              | 15 ++++++++++
 arch/sparc/kernel/unaligned_32.c          |  1 +
 arch/sparc/mm/fault_32.c                  |  1 +
 arch/sparc/mm/fault_64.c                  |  1 +
 arch/tile/kernel/hardwall.c               |  1 +
 arch/tile/kernel/ptrace.c                 |  2 +-
 arch/tile/kernel/single_step.c            | 24 +++++++++-------
 arch/tile/kernel/traps.c                  |  4 ++-
 arch/tile/kernel/unaligned.c              | 46 +++++++++++++++++--------------
 arch/tile/mm/fault.c                      |  1 +
 arch/um/kernel/ptrace.c                   |  2 +-
 arch/um/kernel/trap.c                     |  4 ++-
 arch/unicore32/kernel/fpu-ucf64.c         |  3 +-
 arch/unicore32/mm/fault.c                 |  3 ++
 arch/x86/entry/vsyscall/vsyscall_64.c     |  2 +-
 arch/x86/kernel/ptrace.c                  |  2 +-
 arch/x86/kernel/traps.c                   |  3 ++
 arch/x86/kvm/mmu.c                        |  1 +
 arch/x86/mm/fault.c                       |  1 +
 arch/xtensa/kernel/ptrace.c               |  1 +
 arch/xtensa/kernel/traps.c                |  1 +
 arch/xtensa/mm/fault.c                    |  1 +
 drivers/usb/core/devio.c                  |  4 +--
 fs/fcntl.c                                |  1 +
 include/linux/ptrace.h                    |  2 +-
 include/linux/signal.h                    |  5 ++++
 ipc/mqueue.c                              |  1 +
 kernel/debug/kdb/kdb_main.c               |  1 +
 kernel/ptrace.c                           |  2 +-
 kernel/seccomp.c                          |  2 +-
 kernel/signal.c                           | 21 ++++++++++----
 kernel/time/posix-timers.c                |  2 +-
 mm/memory-failure.c                       |  1 +
 100 files changed, 272 insertions(+), 85 deletions(-)

Eric