Thomas Gleixner <tglx@linutronix.de> writes:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> The si_sys_private member of the siginfo which is embedded in the
> preallocated sigqueue is used by the posix timer code to decide whether a
> timer must be reprogrammed on signal delivery.
>
> The handling of this is racy as a long standing comment in that code
> documents. It is modified with the timer lock held, but without sighand
> lock being held. The actual signal delivery code checks for it under
> sighand lock without holding the timer lock.
I suspect this falls under the ancient all integers are atomic rule
in practice.
> Hand the new value to send_sigqueue() as argument and store it with sighand
> lock held.
Is there any way we can simply remove the hack of using si_sys_private,
and use a field in the structure that contains the preallocated signal?
I don't have any issues with updating send_siqueue so that the locking
is consistent. However can we possibly name the argument something like
it_requeue_pending instead of si_private?
Eric
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> ---
> include/linux/sched/signal.h | 2 +-
> kernel/signal.c | 10 +++++++++-
> kernel/time/posix-timers.c | 15 +--------------
> 3 files changed, 11 insertions(+), 16 deletions(-)
> ---
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index c8ed09ac29ac..bd9f569231d9 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -340,7 +340,7 @@ extern int send_sig(int, struct task_struct *, int);
> extern int zap_other_threads(struct task_struct *p);
> extern struct sigqueue *sigqueue_alloc(void);
> extern void sigqueue_free(struct sigqueue *);
> -extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type);
> +extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type, int si_private);
> extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
>
> static inline void clear_notify_signal(void)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 3d2e087283ab..443baadb5ab0 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1915,7 +1915,7 @@ void sigqueue_free(struct sigqueue *q)
> __sigqueue_free(q);
> }
>
> -int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
> +int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type, int si_private)
> {
> int sig = q->info.si_signo;
> struct sigpending *pending;
> @@ -1950,6 +1950,14 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
> if (!likely(lock_task_sighand(t, &flags)))
> goto ret;
>
> + /*
> + * Update @q::info::si_sys_private for posix timer signals with
> + * sighand locked to prevent a race against dequeue_signal() which
> + * decides based on si_sys_private whether to invoke
> + * posixtimer_rearm() or not.
> + */
> + q->info.si_sys_private = si_private;
> +
> ret = 1; /* the signal is ignored */
> result = TRACE_SIGNAL_IGNORED;
> if (!prepare_signal(sig, t, false))
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index bcd5e56412e7..b6cca1ed2f90 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -299,21 +299,8 @@ int posix_timer_queue_signal(struct k_itimer *timr)
> if (timr->it_interval)
> si_private = ++timr->it_requeue_pending;
>
> - /*
> - * FIXME: if ->sigq is queued we can race with
> - * dequeue_signal()->posixtimer_rearm().
> - *
> - * If dequeue_signal() sees the "right" value of
> - * si_sys_private it calls posixtimer_rearm().
> - * We re-queue ->sigq and drop ->it_lock().
> - * posixtimer_rearm() locks the timer
> - * and re-schedules it while ->sigq is pending.
> - * Not really bad, but not that we want.
> - */
> - timr->sigq->info.si_sys_private = si_private;
> -
> type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
> - ret = send_sigqueue(timr->sigq, timr->it_pid, type);
> + ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
> /* If we failed to send the signal the timer stops. */
> return ret > 0;
> }