[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <874j6119vg.fsf@email.froward.int.ebiederm.org>
Date: Fri, 27 Sep 2024 08:53:07 -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 03/27] signal: Get rid of resched_timer logic
Thomas Gleixner <tglx@...utronix.de> writes:
> From: Thomas Gleixner <tglx@...utronix.de>
>
> There is no reason for handing the *resched pointer argument through
> several functions just to check whether the signal is related to a self
> rearming posix timer.
>
> SI_TIMER is only used by the posix timer code and cannot be queued from
> user space.
Huh??? We have rt_sigqueueinfo. You just touched the code that
copies the queued signal from userspace.
> The only extra check in collect_signal() to verify whether the
> queued signal is preallocated is not really useful. Some other places
> already check purely the SI_TIMER type.
The check to see if the signal was preallocated prevents shenanigans
with setting si_sys_private.
That is today you can queue a signal with rt_sigqueueinfo and set
si_sys_private and it will make it to userspace. I don't know how
much we care but that is the case.
Which means that WARN_ON you added in __send_signal_locked can
most definitely be triggered by userspace.
Eric
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>
> ---
> kernel/signal.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
> ---
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7706cd304785..3d2e087283ab 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -526,8 +526,7 @@ bool unhandled_signal(struct task_struct *tsk, int sig)
> return !tsk->ptrace;
> }
>
> -static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info,
> - bool *resched_timer)
> +static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info)
> {
> struct sigqueue *q, *first = NULL;
>
> @@ -549,12 +548,6 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> still_pending:
> list_del_init(&first->list);
> copy_siginfo(info, &first->info);
> -
> - *resched_timer =
> - (first->flags & SIGQUEUE_PREALLOC) &&
> - (info->si_code == SI_TIMER) &&
> - (info->si_sys_private);
> -
> __sigqueue_free(first);
> } else {
> /*
> @@ -571,13 +564,12 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
> }
> }
>
> -static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
> - kernel_siginfo_t *info, bool *resched_timer)
> +static int __dequeue_signal(struct sigpending *pending, sigset_t *mask, kernel_siginfo_t *info)
> {
> int sig = next_signal(pending, mask);
>
> if (sig)
> - collect_signal(sig, pending, info, resched_timer);
> + collect_signal(sig, pending, info);
> return sig;
> }
>
> @@ -589,17 +581,15 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
> int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
> {
> struct task_struct *tsk = current;
> - bool resched_timer = false;
> int signr;
>
> lockdep_assert_held(&tsk->sighand->siglock);
>
> *type = PIDTYPE_PID;
> - signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
> + signr = __dequeue_signal(&tsk->pending, mask, info);
> if (!signr) {
> *type = PIDTYPE_TGID;
> - signr = __dequeue_signal(&tsk->signal->shared_pending,
> - mask, info, &resched_timer);
> + signr = __dequeue_signal(&tsk->signal->shared_pending, mask, info);
>
> if (unlikely(signr == SIGALRM))
> posixtimer_rearm_itimer(tsk);
> @@ -626,7 +616,7 @@ int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
> }
>
> if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
> - if (unlikely(resched_timer))
> + if (unlikely(info->si_code == SI_TIMER && info->si_sys_private))
> posixtimer_rearm(info);
> }
>
> @@ -1011,6 +1001,9 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
>
> lockdep_assert_held(&t->sighand->siglock);
>
> + if (WARN_ON_ONCE(!is_si_special(info) && info->si_code == SI_TIMER))
> + return 0;
> +
> result = TRACE_SIGNAL_IGNORED;
> if (!prepare_signal(sig, t, force))
> goto ret;
Powered by blists - more mailing lists