linux-kernel - Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 21 May 2008 16:01:35 -0700 (PDT)
From:	Roland McGrath <roland@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Oleg Nesterov <oleg@...sign.ru>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Austin Clements <amdragon+kernelbugzilla@....edu>,
	Ingo Molnar <mingo@...e.hu>, john stultz <johnstul@...ibm.com>,
	Michael Kerrisk <mtk.manpages@...glemail.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it
 is queued

The timer_delete/sigqueue_free behavior predates my involvement with the
code.  I think it is fine to leave pending signals queued.  (POSIX makes it
unspecified for deleted timers.  A robust application cannot even assume
that they will or won't be removed consistently on one running system.)

What I think everyone agrees is wrong in the abstract is the status quo,
where (effectively) signals stay queued but with zeroed siginfo_t values.
(This violates POSIX.)  If there is a signal, its siginfo_t must be intact.

A robust application has to drain any pending timer signals by unblocking
the signal or using sigwait.  This is true if signals might remain
properly queued as POSIX permits.  It's also true of existing Linux
kernels, where a bogus infoless signal remains pending.

There is also the caveat I mentioned before about RLIMIT_SIGPENDING.
An existing dumb application might do many iterations of timer_create,
timer_settime, (timer fires), timer_delete, all with the timer signal
blocked, and not expect that it can hit its rlimit and not be able to
create more timers until it drains those signals.  I don't mind breaking
any such application.  It's dumb.

As I read it, POSIX leaves it the same unspecified what is the
disposition of pending timer signals at the time of the implicit timer
deletion at exec.  But people sure find it surprising on exec.

So maybe this (wholly untested)?
The last hunk could go alone (just keep queued) before the rest (exec flush).

Thanks,
Roland

diff --git a/kernel/signal.c b/kernel/signal.c
index 72bb4f5..0000000 100644  
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -205,18 +205,25 @@ static void __sigqueue_free(struct sigqu
 	kmem_cache_free(sigqueue_cachep, q);
 }

-void flush_sigqueue(struct sigpending *queue)
+static void __flush_sigqueue(struct sigpending *queue, int timers)
 {
 	struct sigqueue *q;

 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
+		if (timers && q->info.si_code != SI_TIMER)
+			continue;
 		list_del_init(&q->list);
 		__sigqueue_free(q);
 	}
 }

+void flush_sigqueue(struct sigpending *queue)
+{
+	__flush_sigqueue(queue, 0);
+}
+
 /*
  * Flush all pending signals for a task.
  */
@@ -243,6 +250,7 @@ void ignore_signals(struct task_struct *

 /*
  * Flush all handlers for a task.
+ * Also flush all SI_TIMER signals from the queues.
  */

 void
@@ -257,6 +265,9 @@ flush_signal_handlers(struct task_struct
 		sigemptyset(&ka->sa.sa_mask);
 		ka++;
 	}
+
+	__flush_sigqueue(&t->pending, 1);
+	__flush_sigqueue(&t->signal->shared_pending, 1);
 }

 int unhandled_signal(struct task_struct *tsk, int sig)
@@ -1240,17 +1251,18 @@ void sigqueue_free(struct sigqueue *q)

 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 	/*
-	 * If the signal is still pending remove it from the
-	 * pending queue. We must hold ->siglock while testing
+	 * If the signal is still pending, leave it to be dequeued.
+	 * We must hold ->siglock while testing
 	 * q->list to serialize with collect_signal().
 	 */
 	spin_lock_irqsave(lock, flags);
+	q->flags &= ~SIGQUEUE_PREALLOC;
 	if (!list_empty(&q->list))
-		list_del_init(&q->list);
+		q = NULL;
 	spin_unlock_irqrestore(lock, flags);

-	q->flags &= ~SIGQUEUE_PREALLOC;
-	__sigqueue_free(q);
+	if (q)
+		__sigqueue_free(q);
 }

 int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/