Message-ID: <87o749xisy.fsf@email.froward.int.ebiederm.org>
Date: Fri, 27 Sep 2024 09:39:09 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,  Anna-Maria Behnsen
 <anna-maria@...utronix.de>,  Frederic Weisbecker <frederic@...nel.org>,
  John Stultz <jstultz@...gle.com>,  Peter Zijlstra <peterz@...radead.org>,
  Ingo Molnar <mingo@...nel.org>,  Stephen Boyd <sboyd@...nel.org>,  Oleg
 Nesterov <oleg@...hat.com>
Subject: Re: [patch v4 00/27] posix-timers: Cure the SIG_IGN mess

Thomas Gleixner <tglx@...utronix.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

Powered by blists - more mailing lists