linux-kernel - Re: [PATCH v4] kernel/fork: beware of __put_task

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]

Date:   Mon, 6 Feb 2023 15:32:54 -0300
From:   Wander Lairson Costa <wander@...hat.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Andrei Vagin <avagin@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Hu Chunyu <chuhu@...hat.com>, Oleg Nesterov <oleg@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>,
        Paul McKenney <paulmck@...nel.org>
Subject: Re: [PATCH v4] kernel/fork: beware of __put_task_struct calling context

On Mon, Feb 6, 2023 at 11:57 AM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
>
> On 2023-02-06 10:04:47 [-0300], Wander Lairson Costa wrote:
> > Under PREEMPT_RT, __put_task_struct() indirectly acquires sleeping
> > locks. Therefore, it can't be called from an non-preemptible context.
> >
> > One practical example is splat inside inactive_task_timer(), which is
> > called in a interrupt context:
>
> Do you have more?
> The inactive_task_timer() is marked as HRTIMER_MODE_REL_HARD which means
> in runs in hardirq-context. The author of commit
>    850377a875a48 ("sched/deadline: Ensure inactive_timer runs in hardirq context")
>
> should have been aware of that.
> We have on workaround of that put_task() in sched-switch. I wasn't aware
> of this shortcoming. So either we have more problems or potential
> problems or this is the only finding so far.
>

Valentin spotted two other potential issues, I fixed them in v2[1].
Also there is a discussion there that led to this implementation.

> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 9f7fe3541897..532dd2ceb6a3 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -857,6 +857,29 @@ void __put_task_struct(struct task_struct *tsk)
> …
> > +void __put_task_struct(struct task_struct *tsk)
> > +{
> > +     if (IS_ENABLED(CONFIG_PREEMPT_RT) && (!preemptible() || !in_task()))
>
> Is it safe to use the rcu member in any case? If so why not use it
> unconditionally?
>

I am unsure what would be the consequences of moving every call to
RCU, so I thought it would be better to play safe and do it only when
necessary.

> > +             /*
> > +              * under PREEMPT_RT, we can't call put_task_struct
> > +              * in atomic context because it will indirectly
> > +              * acquire sleeping locks.
> > +              *
> > +              * call_rcu() will schedule delayed_put_task_struct_rcu()
> > +              * to be called in process context.
> > +              */
> > +             call_rcu(&tsk->rcu, delayed_put_task_struct_rcu);
> > +     else
> > +             ___put_task_struct(tsk);
> > +}
> >  EXPORT_SYMBOL_GPL(__put_task_struct);
> >
> >  void __init __weak arch_task_cache_init(void) { }
>
> Sebastian
>

[1] https://lore.kernel.org/all/20230120150246.20797-1-wander@redhat.com/