Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Anna-Maria Behnsen <anna-maria@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Sebastian Siewior <bigeasy@linutronix.de>,
        syzbot+5c54bd3eb218bb595aa9@syzkaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@google.com>,
        Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [patch 01/20] posix-timers: Prevent RT livelock in itimer_delete()
In-Reply-To: <87bkj03qmq.ffs@tglx>
References: <20230425181827.219128101@linutronix.de>
 <20230425183312.862346341@linutronix.de>
 <ZFPmKtFGPUQkeDEd@localhost.localdomain> <87bkj03qmq.ffs@tglx>
Date:   Fri, 05 May 2023 09:57:18 +0200
Message-ID: <875y9743dt.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, May 04 2023 at 20:20, Thomas Gleixner wrote:
> On Thu, May 04 2023 at 19:06, Frederic Weisbecker wrote:
>> Le Tue, Apr 25, 2023 at 08:48:56PM +0200, Thomas Gleixner a =C3=A9crit :
>>> itimer_delete() has a retry loop when the timer is concurrently expired=
. On
>>> non-RT kernels this just spin-waits until the timer callback has
>>> completed. On RT kernels this is a potential livelock when the exiting =
task
>>> preempted the hrtimer soft interrupt.
>>>=20
>>> This only affects hrtimer based timers as Posix CPU timers cannot be
>>> concurrently expired. For CONFIG_POSIX_CPU_TIMERS_TASK_WORK=3Dy this is
>>> obviously impossible as the task cannot run task work and exit at the s=
ame
>>> time. The CONFIG_POSIX_CPU_TIMERS_TASK_WORK=3Dn (only non-RT) is preven=
ted
>>> because interrupts are disabled.
>>
>> But the owner of the timer is not the same as the target of the timer, r=
ight?
>>
>> Though I seem to remember that we forbid setting a timer to a target out=
side
>> the current process, in which case the owner and the target are the same=
 at
>> this exit stage. But I can't remember what enforces that permission in
>> pid_for_clock()..
>
> The owner of the timer is always the one which needs to find the entity
> to synchronize on, whether that's the right hrtimer base or the task
> which runs the expiry code.
>
> wait_for_running_timer() is taking that into account:
>
>   - The hrtimer timer based posix timers lock the hrtimer base expiry
>     lock on the base to which the timer is currently associated
>
>   - Posix CPU timers can be armed on a differnet process (only per
>     thread timers are restricted to currents threadgroup) but the
>     wait_for_running() callback "knows" how to find that process:
>
>     When the timer is moved to the expiry list it gets:
>
>          cputimer->firing =3D 1;
>          rcu_assign_pointer(ctmr->handling, current);
>
>    and the wait for running side does:
>
>        rcu_read_lock()
>        tsk =3D rcu_dereference(timr->it.cpu.handling);
>        ....
>        mutex_lock(&tsk->posix_cputimers_work.mutex);
>
>    See collect_timerqueue(), handle_posix_cpu_timers() and
>    posix_cpu_timer_wait_running() for details.
>
>    commit f7abf14f0001 ("posix-cpu-timers: Implement the missing
>    timer_wait_running callback") has quite some prose in the changelog.

But you have a point. The comment I added in itimer_delete() vs. CPU
timers is wrong for timers which are armed on a different process.
Needs to be removed.

Thanks,

        tglx