Jan Kiszka <[email protected]> writes:
> On 15.03.21 10:47, Philippe Gerum wrote: >> >> Jan Kiszka <[email protected]> writes: >> >>> On 15.03.21 10:00, Philippe Gerum wrote: >>>> >>>> Jan Kiszka <[email protected]> writes: >>>> >>>>> On 14.03.21 18:14, Philippe Gerum wrote: >>>>>> >>>>>> Jan Kiszka via Xenomai <[email protected]> writes: >>>>>> >>>>>>> From: Jan Kiszka <[email protected]> >>>>>>> >>>>>>> This is only called during early init, e.g. for switching alternatives. >>>>>>> Still, switch_mm_irqs_off would complain without this, and we are better >>>>>>> safe than sorry. >>>>>>> >>>>>> >>>>>> The way this is done in Dovetail is fragile too, since the protection we >>>>>> have there still expects the pipeline entry code not to mess up on >>>>>> handling an interrupt, which defeats the purpose of such >>>>>> precaution. Besides, the temp_state should be snapshot under protection >>>>>> too. IOW, IRQs should be hard disabled fully while using the temporary >>>>>> mm. >>>>>> >>>>>> Upstreaming a similar patch for Dovetail. >>>>> >>>>> Just saw it: It's wrong as it left the hard_irq_save_full at the caller >>>>> site. Would raise a warning when debugging is enabled. Please fix. >>>>> >>>> >>>> I guess you mean local_irq_save_full(). After a second look, the hard >>>> irqs off section should cover the entire code poking new text into a >>>> live kernel. This is what switching to the _full() forms enforced >>>> already. So I see no point in changing the IRQ state in the use/unuse >>>> helpers eventually. >>> >>> Right, either way is needed. If the tlb flush should be fully protected, >>> the existing pattern is needed. But maybe more: >>> >>> Under I-pipe, playing with full disabling didn't work out well. Some >>> spinlock function called via get_locked_pte reenabled hard IRQs. Not >>> sure if dovetail is affected by that as well. >>> >> >> In the case at hand, the PTE of the poking address is locked and >> returned outside of the locked section, so this should not be a concern >> for live patching. >> > > Yeah, I'm carrying this now: > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index cbaa584a9f23..affc594cc939 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -808,6 +808,7 @@ static inline temp_mm_state_t use_temporary_mm(struct > mm_struct *mm) > temp_mm_state_t temp_state; > > lockdep_assert_irqs_disabled(); > + WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled()); > > /* > * Make sure not to be in TLB lazy mode, as otherwise we'll end up > @@ -821,8 +822,6 @@ static inline temp_mm_state_t use_temporary_mm(struct > mm_struct *mm) > * unuse_temporary_mm() assumes hardirqs were off on entry to > * use_temporary_mm(), assert this condition. > */ > - WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled()); > - hard_cond_local_irq_disable(); > temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm); > switch_mm_irqs_off(NULL, mm, current); > > @@ -846,8 +845,8 @@ static inline temp_mm_state_t use_temporary_mm(struct > mm_struct *mm) > static inline void unuse_temporary_mm(temp_mm_state_t prev_state) > { > lockdep_assert_irqs_disabled(); > + WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled()); > switch_mm_irqs_off(NULL, prev_state.mm, current); > - hard_cond_local_irq_enable(); > > /* > * Restore the breakpoints if they were disabled before the temporary mm > > And no warnings so far. > > Jan I have almost the same here for Dovetail, except that I'm not duplicating the assertion in unuse_temporary_mm since there is no reason for the hard irq state flipping in between. No issue observed either. -- Philippe.
