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.
