linux-kernel - Re: [PATCH 1/3] sched: fix exit

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:   Thu, 22 Oct 2020 14:51:00 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Will Deacon <will@...nel.org>, paulmck <paulmck@...nel.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Andy Lutomirski <luto@...capital.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        linux-mm <linux-mm@...ck.org>
Subject: Re: [PATCH 1/3] sched: fix exit_mm vs membarrier (v4)

Hi,

On Tue, Oct 20, 2020 at 10:59:58AM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 20, 2020, at 10:36 AM, Peter Zijlstra peterz@...radead.org wrote:
> 
> > On Tue, Oct 20, 2020 at 09:47:13AM -0400, Mathieu Desnoyers wrote:
> >> +void membarrier_update_current_mm(struct mm_struct *next_mm)
> >> +{
> >> +	struct rq *rq = this_rq();
> >> +	int membarrier_state = 0;
> >> +
> >> +	if (next_mm)
> >> +		membarrier_state = atomic_read(&next_mm->membarrier_state);
> >> +	if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> >> +		return;
> >> +	WRITE_ONCE(rq->membarrier_state, membarrier_state);
> >> +}
> > 
> > This is suspisioucly similar to membarrier_switch_mm().
> > 
> > Would something like so make sense?
> 
> Very much yes. Do you want me to re-send the series, or you
> want to fold this in as you merge it ?
> 
> Thanks,
> 
> Mathieu
> 
> > 
> > ---
> > --- a/kernel/sched/membarrier.c
> > +++ b/kernel/sched/membarrier.c
> > @@ -206,14 +206,7 @@ void membarrier_exec_mmap(struct mm_stru
> > 
> > void membarrier_update_current_mm(struct mm_struct *next_mm)
> > {
> > -	struct rq *rq = this_rq();
> > -	int membarrier_state = 0;
> > -
> > -	if (next_mm)
> > -		membarrier_state = atomic_read(&next_mm->membarrier_state);
> > -	if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> > -		return;
> > -	WRITE_ONCE(rq->membarrier_state, membarrier_state);
> > +	membarrier_switch_mm(this_rq(), NULL, next_mm);
> > }
> > 
> > static int membarrier_global_expedited(void)
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index d2621155393c..3d589c2ffd28 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2645,12 +2645,14 @@ static inline void membarrier_switch_mm(struct rq *rq,
> > 					struct mm_struct *prev_mm,
> > 					struct mm_struct *next_mm)
> > {
> > -	int membarrier_state;
> > +	int membarrier_state = 0;
> > 
> > 	if (prev_mm == next_mm)

Unless I'm missing something subtle, in exit_mm(),
membarrier_update_current_mm() is called with @next_mm == NULL, and
inside membarrier_update_current_mm(), membarrier_switch_mm() is called
wiht @prev_mm == NULL. As a result, the branch above is taken, so
membarrier_update_current_mm() becomes a nop. I think we should use the
previous value of current->mm as the @prev_mm, something like below
maybe?

void update_current_mm(struct mm_struct *next_mm)
{
	struct mm_struct *prev_mm;
	unsigned long flags;

	local_irq_save(flags);
	prev_mm = current->mm;
	current->mm = next_mm;
	membarrier_switch_mm(this_rq(), prev_mm, next_mm);
	local_irq_restore(flags);
}

, and replace all settings for "current->mm" in kernel with
update_current_mm().

Thoughts?

Regards,
Boqun

> > 		return;
> > 
> > -	membarrier_state = atomic_read(&next_mm->membarrier_state);
> > +	if (next_mm)
> > +		membarrier_state = atomic_read(&next_mm->membarrier_state);
> > +
> > 	if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> >  		return;
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com