This are the remaining bits to cure the SIG_IGN mess. The preparatory work
from the previous version 3 has been merged already. Version 3 can be found
here:

   https://lore.kernel.org/lkml/20240610163452.591699700@linutronix.de

Last year I reread a 15 years old comment about the SIG_IGN problem:

 "FIXME: What we really want, is to stop this timer completely and restart
  it in case the SIG_IGN is removed. This is a non trivial change which
  involves sighand locking (sigh !), which we don't want to do late in the
  release cycle.  ...  A more complex fix which solves also another related
  inconsistency is already in the pipeline."

The embarrasing part was that I put that comment in back then. So I went
back and rumaged through old notes as I completely had forgotten why our
attempts to fix this back then failed.

It turned out that the comment is about right: sighand locking and life
time issues. So I sat down with the old notes and started to wrap my head
around this again.

The problem to solve:

Posix interval timers are not rearmed automatically by the kernel for
various reasons:

   1) To prevent DoS by extremly short intervals.
   2) To avoid timer overhead when a signal is pending and has not
      yet been delivered.

This is achieved by queueing the signal at timer expiry and rearming the
timer at signal delivery to user space. This puts the rearming basically
under scheduler control and the work happens in context of the task which
asked for the signal.

There is a problem with that vs. SIG_IGN. If a signal has SIG_IGN installed
as handler, the related signals are discarded. So in case of posix interval
timers this means that such a timer is never rearmed even when SIG_IGN is
replaced later with a real handler (including SIG_DFL).

To work around that the kernel self rearms those timers and throttles them
when the interval is smaller than a tick to prevent a DoS.

That just keeps timers ticking, which obviously has effects on power and
just creates work for nothing.

So ideally these timers should be stopped and rearmed when SIG_IGN is
replaced, which aligns with the regular handling of posix timers.

Sounds trivial, but isn't:

  1) Lock ordering.

     The timer lock cannot be taken with sighand lock held which is
     problematic vs. the atomicity of sigaction().

  2) Life time rules

     The timer and the sigqueue are separate entities which requires a
     lookup of the timer ID in the signal rearm code. This can be handled,
     but the separate life time rules are not necessarily robust.

  3) Finding the relevant timers

     Obviosly it is possible to walk the posix timer list under sighand
     lock and handle it from there. That can be expensive especially in the
     case that there are no affected timers as the walk would just end up
     doing nothing.

The following series is a new and this time actually working attempt to
solve this. It addresses it by:

  1) Embedding the preallocated sigqueue into struct k_itimer, which makes
     the life time rules way simpler and just needs a trivial reference
     count.

  2) Having a separate list in task::signal on which ignored timers are
     queued.

     This avoids walking a potentially large timer list for nothing on a
     SIG_IGN to handler transition.

  3) Requeueing the timers signal in the relevant signal queue so the timer
     is rearmed when the signal is actually delivered

     That turned out to be the least complicated way to address the sighand
     lock vs. timer lock ordering issue.

With that timers which have their signal ignored are not longer self
rearmed and the relevant workarounds including throttling for DoS
prevention are removed.

The series is also available from git:

    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git posixt-v4

Changes vs. V3:

    - Rebased to mainline
   
    - Fixed up a intermediate build breakage reported by 0-day

Thanks,

	tglx
---
 arch/x86/kernel/signal_32.c            |    2 
 arch/x86/kernel/signal_64.c            |    2 
 drivers/power/supply/charger-manager.c |    3 
 fs/proc/base.c                         |    4 
 fs/timerfd.c                           |    4 
 include/linux/alarmtimer.h             |   10 
 include/linux/posix-timers.h           |   67 +++-
 include/linux/sched/signal.h           |    4 
 include/uapi/asm-generic/siginfo.h     |    2 
 init/init_task.c                       |    5 
 kernel/fork.c                          |    1 
 kernel/signal.c                        |  476 +++++++++++++++++++--------------
 kernel/time/alarmtimer.c               |   87 ------
 kernel/time/itimer.c                   |   22 +
 kernel/time/posix-cpu-timers.c         |   38 +-
 kernel/time/posix-timers.c             |  227 +++++++--------
 kernel/time/posix-timers.h             |    8 
 net/netfilter/xt_IDLETIMER.c           |    4 
 18 files changed, 523 insertions(+), 443 deletions(-)

Thomas Gleixner <tglx@linutronix.de> writes:

> This are the remaining bits to cure the SIG_IGN mess. The preparatory work
> from the previous version 3 has been merged already. Version 3 can be found
> here:
>
>    https://lore.kernel.org/lkml/20240610163452.591699700@linutronix.de
>
> Last year I reread a 15 years old comment about the SIG_IGN problem:
>
>  "FIXME: What we really want, is to stop this timer completely and restart
>   it in case the SIG_IGN is removed. This is a non trivial change which
>   involves sighand locking (sigh !), which we don't want to do late in the
>   release cycle.  ...  A more complex fix which solves also another related
>   inconsistency is already in the pipeline."
>
> The embarrasing part was that I put that comment in back then. So I went
> back and rumaged through old notes as I completely had forgotten why our
> attempts to fix this back then failed.
>
> It turned out that the comment is about right: sighand locking and life
> time issues. So I sat down with the old notes and started to wrap my head
> around this again.
>
> The problem to solve:
>
> Posix interval timers are not rearmed automatically by the kernel for
> various reasons:
>
>    1) To prevent DoS by extremly short intervals.
>    2) To avoid timer overhead when a signal is pending and has not
>       yet been delivered.
>
> This is achieved by queueing the signal at timer expiry and rearming the
> timer at signal delivery to user space. This puts the rearming basically
> under scheduler control and the work happens in context of the task which
> asked for the signal.
>
> There is a problem with that vs. SIG_IGN. If a signal has SIG_IGN installed
> as handler, the related signals are discarded. So in case of posix interval
> timers this means that such a timer is never rearmed even when SIG_IGN is
> replaced later with a real handler (including SIG_DFL).
>
> To work around that the kernel self rearms those timers and throttles them
> when the interval is smaller than a tick to prevent a DoS.
>
> That just keeps timers ticking, which obviously has effects on power and
> just creates work for nothing.
>
> So ideally these timers should be stopped and rearmed when SIG_IGN is
> replaced, which aligns with the regular handling of posix timers.
>
> Sounds trivial, but isn't:
>
>   1) Lock ordering.
>
>      The timer lock cannot be taken with sighand lock held which is
>      problematic vs. the atomicity of sigaction().
>
>   2) Life time rules
>
>      The timer and the sigqueue are separate entities which requires a
>      lookup of the timer ID in the signal rearm code. This can be handled,
>      but the separate life time rules are not necessarily robust.
>
>   3) Finding the relevant timers
>
>      Obviosly it is possible to walk the posix timer list under sighand
>      lock and handle it from there. That can be expensive especially in the
>      case that there are no affected timers as the walk would just end up
>      doing nothing.
>
> The following series is a new and this time actually working attempt to
> solve this. It addresses it by:
>
>   1) Embedding the preallocated sigqueue into struct k_itimer, which makes
>      the life time rules way simpler and just needs a trivial reference
>      count.
>
>   2) Having a separate list in task::signal on which ignored timers are
>      queued.
>
>      This avoids walking a potentially large timer list for nothing on a
>      SIG_IGN to handler transition.
>
>   3) Requeueing the timers signal in the relevant signal queue so the timer
>      is rearmed when the signal is actually delivered
>
>      That turned out to be the least complicated way to address the sighand
>      lock vs. timer lock ordering issue.
>
> With that timers which have their signal ignored are not longer self
> rearmed and the relevant workarounds including throttling for DoS
> prevention are removed.
>
> The series is also available from git:
>
>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git posixt-v4
>
> Changes vs. V3:
>
>     - Rebased to mainline
>    
>     - Fixed up a intermediate build breakage reported by 0-day

I have stopped looking at this after patch 4.

The current code can and does handle userspace injecting a signal with
si_sys_private sent to an non-zero value using rt_sigqueueinfo(2) and
that value will be delivered to userspace.

I think the at least the ability to inject such a signal (ignoring
si_sys_private) is very interesting for debuggers and checkpoint restart
applications.

I get the feeling the rest of the patch series depends upon not
supporting userspace injecting signals with si_code == SI_TIMER.  That
seems unnecessary.

It seems reasonable to depend upon something like the SIGQUEUE_PREALLOC
in the flags field of struct sigqueue to detect a kernel generated
signal.  Rather than adding various hacks to make everything work
with just a struct kernel_siginfo_t.  Especially as the timer signals
today are the only signals that are preallocated.

Is there any chance 18/27 posix-timers: Embed sigqueue in struct k_itimer
can be moved up?

That should allow removing the reliance on si_sys_private.

That should prevent the need to add another hack with sys_private_ptr in
struct kernel_siginfo

Perhaps what needs to happen is to update collect_signal to return the
sigqueue entry (if it was preallocated), instead of the resched_timer.
Then the timer code can just use container_of to get the struct
k_itimer?

After that si_sys_private can move into struct k_itimer, and the code
won't need to worry about userspace setting that value, or about needing
to clear that value.  As si_sys_private will always be 0 in preallocated
signals.

Eric

On Fri, Sep 27 2024 at 09:39, Eric W. Biederman wrote:
> I have stopped looking at this after patch 4.
>
> The current code can and does handle userspace injecting a signal with
> si_sys_private sent to an non-zero value using rt_sigqueueinfo(2) and
> that value will be delivered to userspace.
>
> I think the at least the ability to inject such a signal (ignoring
> si_sys_private) is very interesting for debuggers and checkpoint restart
> applications.
>
> I get the feeling the rest of the patch series depends upon not
> supporting userspace injecting signals with si_code == SI_TIMER.  That
> seems unnecessary.
>
> It seems reasonable to depend upon something like the SIGQUEUE_PREALLOC
> in the flags field of struct sigqueue to detect a kernel generated
> signal.  Rather than adding various hacks to make everything work
> with just a struct kernel_siginfo_t.  Especially as the timer signals
> today are the only signals that are preallocated.

Fair enough.

> Is there any chance 18/27 posix-timers: Embed sigqueue in struct k_itimer
> can be moved up?
>
> That should allow removing the reliance on si_sys_private.
>
> That should prevent the need to add another hack with sys_private_ptr in
> struct kernel_siginfo
>
> Perhaps what needs to happen is to update collect_signal to return the
> sigqueue entry (if it was preallocated), instead of the resched_timer.
> Then the timer code can just use container_of to get the struct
> k_itimer?
>
> After that si_sys_private can move into struct k_itimer, and the code
> won't need to worry about userspace setting that value, or about needing
> to clear that value.  As si_sys_private will always be 0 in preallocated
> signals.

Let me try that.

Thanks for taking a look!

       tglx