Re: [Patch v3 07/13] x86/process Add arch_set_dumpable

[Tim Chen]

On 10/19/2018 01:16 PM, Thomas Gleixner wrote:
> On Fri, 19 Oct 2018, Thomas Gleixner wrote:
>> On Thu, 18 Oct 2018, Tim Chen wrote:
>>> On 10/18/2018 06:28 AM, Thomas Gleixner wrote:
>>>>
>>>> So now the obvious question. set_dumpable() operates on tsk->mm. i.e. it's
>>>> a process wide operation. But arch_set_dumpable() operates on the task
>>>> itself. What about the other tasks of that process?
>>>
>>> I missed this part.
>>>
>>> Fixing this is tricky as I don't see an easy way to
>>> reverse map mm back to all tasks that use the same mm
>>> to update their STIBP flags.
>>
>> task is known and handed in to the function. So why do you want to reverse
>> map mm?
>>
>> for_each_thread(task, ...) is what you want. The only thing to verify is
>> whether you can lock the tasks sighand lock there. And then it's enough to
>> set/clear the TIF bit and let it take effect at the next context switch.
> 
> Though there is another issue with mm shared between processes,
> i.e. CLONE_VM without CLONE_THREAD where set_dumpabe() affects both
> processes, but for_each_thread() only seing the threads which belong to the
> thread group of 'task'.

One complication is threads currently running on some remote CPU
when we change flag.  Normally we will call spec_ctrl_update_current to make
sure the CPU's current SPEC CTRL MSR stays consistent with TIF flag changes.

But for the remote CPUs, if we don't the update their SPEC CTRL MSRs, their
SPEC CTRL MSR will get out of step with the TIF flag state.  It can last for a
long time till we have a TIF flag change during context switches to cause SEPC CTRL MSR
update.

One thing I could do is to add a TIF flag to force update the SPEC_CTRL MSR on
next context switch.  Is that okay?  Or should I put such flag elsewhere?

Tim


> 
> I can understand your idea of clumping this on dumpable(), but at some
> point we really have to draw a line and just stop adding more and more
> expensive stuff to context switch.
> 
> Thanks,
> 
> 	tglx
>