Re: [RFC][PATCH v2 11/11] context_tracking,x86: Fix text_poke_sync() vs NOHZ_FULL

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Oct 26, 2021 at 03:19:11PM -0300, Marcelo Tosatti wrote:
> On Thu, Oct 21, 2021 at 10:18:59PM +0200, Peter Zijlstra wrote:
> > On Thu, Oct 21, 2021 at 04:57:09PM -0300, Marcelo Tosatti wrote:
> > > > Pretty much everything in noinstr is magical, we just have to think
> > > > harder there (and possibly start writing more comments there).
> > > 
> > > mds_user_clear_cpu_buffers happens after sync_core, in your patchset, 
> > > if i am not mistaken.
> > 
> > Of course it does, mds_user_clear_cpu_buffers() is on exit, the
> > sync_core() is on entry.
> 
>                                                                   static_key enable/disable
> 
> __exit_to_user_mode ->                                            context_tracking_set_cpu_work(cpu, work)
>    user_enter_irqoff ->                                                  preempt_disable();
>    __context_tracking_enter(CONTEXT_USER);                               seq = atomic_read(&ct->seq);
>       ct_seq_user_enter(raw_cpu_ptr(&context_tracking));                 if (__context_tracking_seq_in_user(seq)) {
>       {                                                                          /* ctrl-dep */
>         arch_atomic_set(&ct->work, 0);                                           atomic_or(work, &ct->work);
>         return arch_atomic_add_return(CT_SEQ_USER, &ct->seq);                    ret = atomic_try_cmpxchg(&ct->seq, &seq, seq|CT_SEQ_WORK);
>                                                                          }
>       }                                                                  preempt_enable();
>    arch_exit_to_user_mode()
>    mds_user_clear_cpu_buffers();  <--- sync_core work queued,
>                                        but not executed.
>                                        i-cache potentially stale?
> 
> ct_seq_user_enter should happen _after_ all possible static_key users?

Right, so this one is actually okay, because that branch is *never*
changed after boot.

I'm not quite sure why it isn't an alternative(). At some point I
proposed static_call_lock() [1] and the corrolary is static_branch_lock(),
which I suppose could be employed here. But I'm not sure that actually
helps much with auditing all that.


[1] https://lkml.kernel.org/r/20210904105529.GA5106@worktop.programming.kicks-ass.net