On Mon, Jan 28, 2019 at 11:45:32PM +0100, Jann Horn wrote: > On Mon, Jan 28, 2019 at 11:39 PM Paul E. McKenney <paulmck@xxxxxxxxxxxxx> wrote: > > On Mon, Jan 28, 2019 at 05:07:07PM -0500, Mathieu Desnoyers wrote: > > > Jann Horn identified a racy access to p->mm in the global expedited > > > command of the membarrier system call. > > > > > > The suggested fix is to hold the task_lock() around the accesses to > > > p->mm and to the mm_struct membarrier_state field to guarantee the > > > existence of the mm_struct. > > > > > > Link: https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@xxxxxxxxxxxxxx > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > [...] > > > --- a/kernel/sched/membarrier.c > > > +++ b/kernel/sched/membarrier.c > > > @@ -81,12 +81,27 @@ static int membarrier_global_expedited(void) > > > > > > rcu_read_lock(); > > > p = task_rcu_dereference(&cpu_rq(cpu)->curr); > > > - if (p && p->mm && (atomic_read(&p->mm->membarrier_state) & > > > - MEMBARRIER_STATE_GLOBAL_EXPEDITED)) { > > > - if (!fallback) > > > - __cpumask_set_cpu(cpu, tmpmask); > > > - else > > > - smp_call_function_single(cpu, ipi_mb, NULL, 1); > > > + /* > > > + * Skip this CPU if the runqueue's current task is NULL or if > > > + * it is a kernel thread. > > > + */ > > > + if (p && READ_ONCE(p->mm)) { > > > + bool mm_match; > > > + > > > + /* > > > + * Read p->mm and access membarrier_state while holding > > > + * the task lock to ensure existence of mm. > > > + */ > > > + task_lock(p); > > > + mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) & > > > > Are we guaranteed that this p->mm will be the same as the one loaded via > > READ_ONCE() above? > > No; the way I read it, that's just an optimization and has no effect > on correctness. > > > Either way, wouldn't it be better to READ_ONCE() it a > > single time and use the same value everywhere? > > No; the first READ_ONCE() returns a pointer that you can't access > because it wasn't read under a lock. You can only use it for a NULL > check. Ah, of course! Thank you both! Thanx, Paul
