Message-ID: <87ikrf78xa.ffs@tglx>
Date: Thu, 19 Dec 2024 20:46:25 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: syzbot <syzbot+3c2e3cc60665d71de2f7@...kaller.appspotmail.com>,
 anna-maria@...utronix.de, frederic@...nel.org,
 linux-kernel@...r.kernel.org, peterz@...radead.org,
 syzkaller-bugs@...glegroups.com
Cc: Eric Biederman <ebiederm@...ssion.com>, Oleg Nesterov <oleg@...hat.com>
Subject: [PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

syzbot triggered the warning in posixtimer_send_sigqueue(), which warns
about a non-ignored signal being already queued on the ignored list:

  WARNING: ... at kernel/signal.c:2050 posixtimer_send_sigqueue

The warning is actually bogus, as the following valid sequence can
trigger it:

  signal($SIG, SIGIGN);
  timer_settime(...);			// arm periodic timer

    timer fires, signal is ignored and queued on ignored list

  sigprocmask(SIG_BLOCK, ...);        // block the signal
  timer_settime(...);			// re-arm periodic timer

    timer fires, signal is not ignored because it is blocked
      ---> Warning triggers as signal is on the ignored list

Ideally timer_settime() should remove the signal, but that's racy and
incomplete vs. other scenarios and requires a full re-evaluation of the
pending signal list.

Instead of adding more complexity, handle it gracefully by removing the
warning and requeueing the signal to the pending list. If the signal gets
unblocked and is still ignored, it's going back to the ignore list. If a
handler was installed before unblocking, it's going to be delivered.

There is a related scenario to trigger the complementary warning in the
signal ignored path, which does not expect the signal to be on the pending
list when it is ignored.

  WARNING: ... at kernel/signal.c:2014 posixtimer_send_sigqueue

That can be triggered even before the above change via:

  task1			task2

  signal($SIG, SIGIGN);
			sigprocmask(SIG_BLOCK, ...);

  timer_create();	// Signal target is task2
  timer_settime(...);	// arm periodic timer

    timer fires, signal is not ignored because it is blocked
    and queued on the pending list of task2

       	      	     	syscall()
			   // Sets the pending flag
			   sigprocmask(SIG_UNBLOCK, ...);

			-> preemption, task2 does not make it
                           back to exit to userspace and therefore
                           cannot dequeue the signal before:

  timer_settime(...);	// re-arm periodic timer

    timer fires, signal is ignored
      ---> Warning triggers as signal is on task2's pending list
           and the thread group is not exiting

Consequently, remove that warning too and just keep the signal on the
pending list. If the signal is dequeued by task2 and still ignored, it will
be moved to the ignored list. If a handler gets installed before the
dequeue, then it will be delivered in the same way as a signal, which is on
the ignored list when SIGIGN is lifted and therefore moved back to the
pending list.

Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")
Reported-by: syzbot+3c2e3cc60665d71de2f7@...kaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
Closes: https://lore.kernel.org/all/6761b16e.050a0220.29fcd0.006d.GAE@google.com
---
 kernel/signal.c |   38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2007,11 +2007,23 @@ void posixtimer_send_sigqueue(struct k_i
 
 		if (!list_empty(&q->list)) {
 			/*
-			 * If task group is exiting with the signal already pending,
-			 * wait for __exit_signal() to do its job. Otherwise if
-			 * ignored, it's not supposed to be queued. Try to survive.
+			 * The signal was ignored and blocked. The timer
+			 * expiry queued it because blocked signals are
+			 * queued independent of the ignored state.
+			 *
+			 * The unblocking set SIGPENDING, but the signal
+			 * was not yet dequeued from the pending list,
+			 * which would have put it back on the ignore list.
+			 * So prepare_signal() sees unblocked and ignored,
+			 * which ends up here. Leave it queued like a
+			 * regular signal.
+			 *
+			 * The same happens when the task group is exiting
+			 * and the signal is already queued.
+			 * prepare_signal() treats SIGNAL_GROUP_EXIT as
+			 * ignored independent of its queued state. This
+			 * gets cleaned up in __exit_signal().
 			 */
-			WARN_ON_ONCE(!(t->signal->flags & SIGNAL_GROUP_EXIT));
 			goto out;
 		}
 
@@ -2046,17 +2058,25 @@ void posixtimer_send_sigqueue(struct k_i
 		goto out;
 	}
 
-	/* This should never happen and leaks a reference count */
-	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
-		hlist_del_init(&tmr->ignored_list);
-
 	if (unlikely(!list_empty(&q->list))) {
 		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 
-	posixtimer_sigqueue_getref(q);
+	/*
+	 * If the signal is on the ignore list, it got blocked after it was
+	 * ignored earlier. But nothing lifted the ignore. Move it back to
+	 * the pending list to be consistent with the regular signal
+	 * handling. If it gets unblocked, it will be ignored again unless
+	 * a handler has been installed before unblocking. If it's not on
+	 * the ignore list acquire a reference count.
+	 */
+	if (likely(hlist_unhashed(&tmr->ignored_list)))
+		posixtimer_sigqueue_getref(q);
+	else
+		hlist_del_init(&tmr->ignored_list);
+
 	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out:

Powered by blists - more mailing lists