linux-kernel - Re: [PATCH v7 1/3] sched/core: warn on call 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <62q45o4f7s67lnbaunozcmkt4qjlpkh36aan7n2iktqgfl3aw3@ufjjmfxilb2k>
Date:   Tue, 2 May 2023 11:46:42 -0300
From:   Wander Lairson Costa <wander@...hat.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Oleg Nesterov <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Christian Brauner <brauner@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andrei Vagin <avagin@...il.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:PERFORMANCE EVENTS SUBSYSTEM" 
        <linux-perf-users@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v7 1/3] sched/core: warn on call put_task_struct in
 invalid context

On Fri, Apr 28, 2023 at 06:17:58PM +0200, Sebastian Andrzej Siewior wrote:
> On 2023-04-25 08:43:01 [-0300], Wander Lairson Costa wrote:
> > Under PREEMPT_RT, spinlocks become sleepable locks. put_task_struct()
> > indirectly acquires a spinlock. Therefore, it can't be called in
> > atomic/interrupt context in RT kernels.
> > 
> > To prevent such conditions, add a check for atomic/interrupt context
> > before calling put_task_struct().
> > 
> > Signed-off-by: Wander Lairson Costa <wander@...hat.com>
> > Suggested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> 
> Been only CCed here.
> 

I relied on the get_maintainer script to generate the recipient list for
me. I will explicity add you to the CC list next time.

> I asked to not special case PREEMPT_RT but doing this (clean up via RCU)
> unconditionally. I don't remember that someone said "this is a bad
> because $reason".
> 

Yes, I can do it. Although I would prefer to do it in a follow up patch.
This way, if something goes wrong, it is easier to revert.

> Lockdep will complain about this on !RT.
> 

In my tests, it didn't.

> The below open codes rtlock_might_resched() with no explanation on why
> it works or where it comes from.
> 

I will add some comments on the next patch version.

> The function is named put_task_struct_atomic_safe() yet it behaves it
> differently on PREEMPT_RT otherwise it remains put_task_struct().
> 
> Not good.

That's intentional. We only need to do the cleanup under RT, but for !RT
we don't need it. Anyway, in the end we will endup with an
unconditional call_rcu().

> 
> > ---
> >  include/linux/sched/task.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> > index 357e0068497c..b597b97b1f8f 100644
> > --- a/include/linux/sched/task.h
> > +++ b/include/linux/sched/task.h
> > @@ -113,14 +113,28 @@ static inline struct task_struct *get_task_struct(struct task_struct *t)
> >  
> >  extern void __put_task_struct(struct task_struct *t);
> >  
> > +#define PUT_TASK_RESCHED_OFFSETS \
> > +	(rcu_preempt_depth() << MIGHT_RESCHED_RCU_SHIFT)
> > +
> > +#define __put_task_might_resched() \
> > +	__might_resched(__FILE__, __LINE__, PUT_TASK_RESCHED_OFFSETS)
> > +
> > +#define put_task_might_resched()			\
> > +	do {						\
> > +		if (IS_ENABLED(CONFIG_PREEMPT_RT))	\
> > +			__put_task_might_resched();	\
> > +	} while (0)
> > +
> >  static inline void put_task_struct(struct task_struct *t)
> >  {
> > +	put_task_might_resched();
> >  	if (refcount_dec_and_test(&t->usage))
> >  		__put_task_struct(t);
> >  }
> >  
> >  static inline void put_task_struct_many(struct task_struct *t, int nr)
> >  {
> > +	put_task_might_resched();
> >  	if (refcount_sub_and_test(nr, &t->usage))
> >  		__put_task_struct(t);
> >  }
> > -- 
> > 2.40.0
> > 
> 
> Sebastian
>