Re: [patch V5 00/26] posix-timers: Cure the SIG_IGN mess

[Eric W. Biederman]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:

> This are the remaining bits to cure the SIG_IGN mess. Version 4 can be found
> here:
>
>    https://lore.kernel.org/lkml/20240927083900.989915582@xxxxxxxxxxxxx
>
> 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-v5
>
> Changes vs. V4:
>
>     - Remove the si_sys_private restrictions - Eric
>
>     - Hand down the pointer to the preallocated sigqueue instead of relying
>       on si_sys_priv* magic - Eric

For the bits removing the dependency on si_sys_private
Acked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>

There are a lot of slightly subtle things going on in the surrounding
patches that my brain glossed over, so I can't speak for them, but
the removal of si_sys_private looks good.

Eric

> Thanks,
>
> 	tglx
> ---
>  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           |   70 ++++
>  include/linux/sched/signal.h           |    4 
>  init/init_task.c                       |    5 
>  kernel/fork.c                          |    1 
>  kernel/signal.c                        |  465 +++++++++++++++++++--------------
>  kernel/time/alarmtimer.c               |   87 ------
>  kernel/time/itimer.c                   |   22 +
>  kernel/time/posix-cpu-timers.c         |   38 +-
>  kernel/time/posix-timers.c             |  222 +++++++--------
>  kernel/time/posix-timers.h             |    8 
>  net/netfilter/xt_IDLETIMER.c           |    4 
>  15 files changed, 502 insertions(+), 445 deletions(-)