linux-kernel - Re: [RFC PATCH v2] posix-timers: Support delivery of signals to the current thread

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]

Message-ID: <CACT4Y+YKy_4mBLYomr49+fTm31Y6Q_kXhJz8O-_RTjMe=B-6eg@mail.gmail.com>
Date:   Wed, 25 Jan 2023 16:34:35 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Marco Elver <elver@...gle.com>
Subject: Re: [RFC PATCH v2] posix-timers: Support delivery of signals to the
 current thread

 On Wed, 25 Jan 2023 at 16:17, Oleg Nesterov <oleg@...hat.com> wrote:
>
> On 01/25, Oleg Nesterov wrote:
> >
> > Other than that I see nothing wrong in this patch, but I forgot everything
> > about posix timers many years ago ;)
>
> Stupid question. Am I right that in posix_timer_event()
>
>         same_thread_group(current, pid_task(timr->it_pid, PIDTYPE_PID))
>
> is always true?
>
> If yes, perhaps we can do a much simpler change which doesn't even affect API?
> See the trivial patch below.
>
> send_sigqueue(PIDTYPE_TGID) notify any thread in thread group, so this doesn't
> really change the semantics, just complete_signal() will likely choose "current"
> as a target for signal_wake_up() and iiuc this is what we want for balancing?
>
> Oleg.
>
>
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 5dead89308b7..e38b53a0f814 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -336,6 +336,7 @@ void posixtimer_rearm(struct kernel_siginfo *info)
>  int posix_timer_event(struct k_itimer *timr, int si_private)
>  {
>         enum pid_type type;
> +       struct pid *pid;
>         int ret;
>         /*
>          * FIXME: if ->sigq is queued we can race with
> @@ -350,8 +351,9 @@ int posix_timer_event(struct k_itimer *timr, int si_private)
>          */
>         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);
> +       type = (timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_PID : PIDTYPE_TGID;
> +       pid = (type == PIDTYPE_PID) ? timr->it_pid : task_pid(current);
> +       ret = send_sigqueue(timr->sigq, pid, type);
>         /* If we failed to send the signal the timer stops. */
>         return ret > 0;
>  }

Hi Oleg,

This is indeed much simpler!

Do I understand correctly that:
1. I would need to use SIGEV_SIGNAL (without SIGEV_THREAD_ID)
2. The signal is still queued into process shared_pending
3. If the current task has not blocked the signal (it shouldn't), then
it won't kick any other task
4. The current task will likely deliver the signal right on the timer
interrupt return to userspace
?

This changes the existing behavior (the "average bear" may be surprised :))
https://elixir.bootlin.com/linux/v6.2-rc5/source/kernel/signal.c#L1007
But currnently it's also queued into shared_pending and any thread
could get the signal anyway. So I think this should be fine.

On the positive side: it should improve performance. Delivering to the
currently running task is better on all fronts (no kicking,
rescheduling, IPIs, better locality), right?

Let me test that it works for my use-case.