On Mon, Jan 28, 2019 at 04:07:26PM -0500, Mathieu Desnoyers wrote:

----- On Jan 28, 2019, at 3:46 PM, paulmck paulmck@linux.ibm.com wrote:

quoted

On Mon, Jan 28, 2019 at 12:27:03PM -0800, Linus Torvalds wrote:

quoted

On Mon, Jan 28, 2019 at 10:27 AM Mathieu Desnoyers
[off-list ref] wrote:

quoted

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.

Hmm. I think this is right. You shouldn't access another threads mm
pointer without proper locking.

That said, we *could* make the mm_cachep be SLAB_TYPESAFE_BY_RCU,
which would allow speculatively reading data off the mm pointer under
RCU. It might not be the *right* mm if somebody just did an exit, but
for things like this it shouldn't matter.

That sounds much simpler and more effective than the contention-reduction
approach that I suggested.  ;-)

I'd be tempted to stick to the locking approach for a fix, and implement
Linus' type-safe mm_cachep idea if anyone complains about the overhead
of membarrier GLOBAL_EXPEDITED (and submit for a future merge window).

I tested the KASAN splat reproducer from Jann locally, and confirmed that
my patch fixes the issue it reproduces.

Please let me know if the task_lock() approach is OK as a fix for now.

Agreed, no need for added complexity until there is a clear need.

I'm also awaiting a Tested-by from Jann before submitting this for real.

Makes sense to me!

							Thanx, Paul

Thanks,

Mathieu

quoted

						Thanx, Paul

quoted

But if this is the only case that might care, it sounds like just
doing the proper locking is the right approach.

           Linus

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com