Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 16 Jun 2021 14:28:54 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH 5/8] membarrier, kthread: Use _ONCE accessors for task->mm
To:     Andy Lutomirski <luto@kernel.org>, x86@kernel.org
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-mm@kvack.org,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Peter Zijlstra <peterz@infradead.org>
References: <cover.1623813516.git.luto@kernel.org>
        <74ace142f48db7d0e71b05b5ace72bfe8e0a2652.1623813516.git.luto@kernel.org>
In-Reply-To: <74ace142f48db7d0e71b05b5ace72bfe8e0a2652.1623813516.git.luto@kernel.org>
MIME-Version: 1.0
Message-Id: <1623817648.jh19a97b5a.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:
> membarrier reads cpu_rq(remote cpu)->curr->mm without locking.  Use
> READ_ONCE() and WRITE_ONCE() to remove the data races.
>=20
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/exec.c                 | 2 +-
>  kernel/kthread.c          | 4 ++--
>  kernel/sched/membarrier.c | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
>=20
> diff --git a/fs/exec.c b/fs/exec.c
> index 18594f11c31f..2e63dea83411 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1007,7 +1007,7 @@ static int exec_mmap(struct mm_struct *mm)
>  	local_irq_disable();
>  	active_mm =3D tsk->active_mm;
>  	tsk->active_mm =3D mm;
> -	tsk->mm =3D mm;
> +	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
>  	/*
>  	 * This prevents preemption while active_mm is being loaded and
>  	 * it and mm are being updated, which could cause problems for
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 8275b415acec..4962794e02d5 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -1322,7 +1322,7 @@ void kthread_use_mm(struct mm_struct *mm)
>  		mmgrab(mm);
>  		tsk->active_mm =3D mm;
>  	}
> -	tsk->mm =3D mm;
> +	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
>  	membarrier_update_current_mm(mm);
>  	switch_mm_irqs_off(active_mm, mm, tsk);
>  	membarrier_finish_switch_mm(atomic_read(&mm->membarrier_state));
> @@ -1363,7 +1363,7 @@ void kthread_unuse_mm(struct mm_struct *mm)
>  	smp_mb__after_spinlock();
>  	sync_mm_rss(mm);
>  	local_irq_disable();
> -	tsk->mm =3D NULL;
> +	WRITE_ONCE(tsk->mm, NULL);  /* membarrier reads this without locks */
>  	membarrier_update_current_mm(NULL);
>  	/* active_mm is still 'mm' */
>  	enter_lazy_tlb(mm, tsk);
> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> index 3173b063d358..c32c32a2441e 100644
> --- a/kernel/sched/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -410,7 +410,7 @@ static int membarrier_private_expedited(int flags, in=
t cpu_id)
>  			goto out;
>  		rcu_read_lock();
>  		p =3D rcu_dereference(cpu_rq(cpu_id)->curr);
> -		if (!p || p->mm !=3D mm) {
> +		if (!p || READ_ONCE(p->mm) !=3D mm) {
>  			rcu_read_unlock();
>  			goto out;
>  		}
> @@ -423,7 +423,7 @@ static int membarrier_private_expedited(int flags, in=
t cpu_id)
>  			struct task_struct *p;
> =20
>  			p =3D rcu_dereference(cpu_rq(cpu)->curr);
> -			if (p && p->mm =3D=3D mm)
> +			if (p && READ_ONCE(p->mm) =3D=3D mm)

/* exec, kthread_use_mm write this without locks */ ?

Seems good to me.

Acked-by: Nicholas Piggin <npiggin@gmail.com>

>  				__cpumask_set_cpu(cpu, tmpmask);
>  		}
>  		rcu_read_unlock();
> @@ -521,7 +521,7 @@ static int sync_runqueues_membarrier_state(struct mm_=
struct *mm)
>  		struct task_struct *p;
> =20
>  		p =3D rcu_dereference(rq->curr);
> -		if (p && p->mm =3D=3D mm)
> +		if (p && READ_ONCE(p->mm) =3D=3D mm)
>  			__cpumask_set_cpu(cpu, tmpmask);
>  	}
>  	rcu_read_unlock();
> --=20
> 2.31.1
>=20
>=20