Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue()

[Oleg Nesterov <oleg@redhat.com>]

On 08/24, Oleg Nesterov wrote:
>
> On 08/24, Thomas Gleixner wrote:
> >
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -983,6 +983,18 @@ static int de_thread(struct task_struct
> >  		}
> >
> >  		/*
> > +		 * Ensure that POSIX timer SIGEV_THREAD_ID signals pending for
> > +		 * the former leader are removed under sighand::siglock _before_
> > +		 * taking over the leader's TID. Otherwise the lockless cleanup
> > +		 * in release_task() can race against a concurrent signal
> > +		 * delivery to the new leader. The former leader has PF_EXITING
> > +		 * set which prevents queueing of SIGEV_THREAD_ID signals up to
> > +		 * the point where it's sighand gets cleared.
> > +		 */
> > +		scoped_guard(spinlock_irq, lock)
> > +			flush_sigqueue(&leader->pending);

scoped_guard(spinlock_irq) is not right. This needs scoped_guard(spinlock),
the code runs with irqs disabled.

> Hmm, at first glance... If we change de_thread() to do this _after_ transfer_pid's
> (before release_task(leader)), then posixtimer_send_sigqueue() doesn't need any
> changes, no?

IOW. Unless I am totally confused, we only need to flush the
SIGQUEUE_PREALLOC sigqueue's which were sent to the (old) leader
before it changed its pid. So we can do this

diff --git a/fs/exec.c b/fs/exec.c
index a14f28b15607..550367e7fe6c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1029,6 +1029,9 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 
+		scoped_guard(spinlock_irq, lock)
+			flush_sigqueue(&leader->pending);
+
 		release_task(leader);
 	}

outside of tasklist_lock.

We do not care if another sigqueue (SIGQUEUE_PREALLOC or not) comes to
leader->pending after that.

No?

Either way, this means that flush_sigqueue() is called again with irqs
disabled... Not a real problem, but can the change below work? Yes,
more fragile and probably "fixes symptom"...

Oleg.

diff --git a/kernel/signal.c b/kernel/signal.c
index bbc0fd4cc4d7..4d12ebba33f9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -477,14 +477,14 @@ static void __sigqueue_free(struct sigqueue *q)
 
 void flush_sigqueue(struct sigpending *queue)
 {
-	struct sigqueue *q;
+	struct sigqueue *q, *n;
 
 	sigemptyset(&queue->signal);
-	while (!list_empty(&queue->list)) {
-		q = list_entry(queue->list.next, struct sigqueue , list);
-		list_del_init(&q->list);
+
+	list_for_each_entry_safe(q, n, &queue->list, list)
 		__sigqueue_free(q);
-	}
+
+	INIT_LIST_HEAD(&queue->list);
 }
 
 /*