Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

["Fernando Luis Vázquez Cao" <fernando@oss.ntt.co.jp>]

Hi Eric!

On Mon, 2006-07-10 at 05:37 -0600, Eric W. Biederman wrote:
> Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> writes:
> 
> > Hi Keith,
> >
> > Thank you for the comments.
> >
> > On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
> >> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
> >> >On the event of a stack overflow critical data that usually resides at
> >> >the bottom of the stack is likely to be stomped and, consequently, its
> >> >use should be avoided.
> >> >
> >> >In particular, in the i386 and IA64 architectures the macro
> >> >smp_processor_id ultimately makes use of the "cpu" member of struct
> >> >thread_info which resides at the bottom of the stack. x86_64, on the
> >> >other hand, is not affected by this problem because it benefits from
> >> >the use of the PDA infrastructure.
> >> >
> >> >To circumvent this problem I suggest implementing
> >> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
> >> >IA64 and use it as a replacement for smp_processor_id in the reboot path
> >> >to the dump capture kernel. This is a possible implementation for i386.
> >> 
> >> I agree with avoiding the use of thread_info when the stack might be
> >> corrupt.  However your patch results in reading apic data and scanning
> >> NR_CPU sized tables for each IPI that is sent, which will slow down the
> >> sending of all IPIs, not just dump.
> > This patch only affects IPIs sent using send_IPI_allbutself which is
> > rarely called, so the impact in performance should be negligible.
> 
> Well smp_call_function uses it so I don't know if rarely called applies.
> 
> However when called with the NMI vector every instance of send_IPI_allbutself
> transforms this into send_IPI_mask.  Which is why we need to know our current
> cpu in the first place.
> 
> Therefore why don't we just do that explicitly in crash.c
> i.e.
> 
> static void smp_send_nmi_allbutself(void)
> {
> 	cpumask_t mask = cpu_online_map;
> 	cpu_clear(safe_smp_processor_id(), mask);
> 	send_IPI_mask(mask, NMI_VECTOR);
> }
> 
> That will guarantee that any effects this code paranoia may have
> are only seen in the crash dump path.

That is a good idea, but I have on concern. In mach-default by default
we use __send_IPI_shortcut (no_broadcast==0) instead of send_IPI_mask.
Is it always safe to ignore the no_broadcast setting? In other words,
can __send_IPI_shortcut be replaced by send_IPI_mask safely?

The implementation of send_IPI_allbutself in the different architectures
follows:

smp_send_nmi_allbutself
  send_IPI_allbutself

* mach-bigsmp
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-default
send_IPI_allbutself
  __local_send_IPI_allbutself
    if (no_broadcast) {
      cpu_clear(smp_processor_id(), mask)
      send_IPI_mask(mask, vector)
        send_IPI_mask_bitmask
          apic_wait_icr_idle
    } else {
      __send_IPI_shortcut(APIC_DEST_ALLBUT, vector)
        apic_wait_icr_idle
    }

* mach-es7000
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask);
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-numaq
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-summit
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

Regards,

Fernando

> 
> 
> >> It would be far cheaper to define
> >> a per-cpu variable containing the logical cpu number, set that variable
> >> once as each cpu is brought up and just read it in cases where you
> >> might not trust the integrity of struct thread_info.  safe_smp_processor_id()
> >> resolves to just a read of the per cpu variable.
> > But to read a per-cpu variable you need to index the corresponding array
> > with processor id of the current CPU (see code below), but that is
> > precisely what we are trying to figure out. Anyway as
> > send_IPI_allbutself is not a fast path (correct if this assumption is
> > wrong) the current implementation of safe_smp_processor_id should be
> > fine.
> >
> > #define get_cpu_var(var) (*({ preempt_disable();
> > &__get_cpu_var(var); }))
> > #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
> >
> > Am I missing something obvious?
> 
> No.  Except that other architectures have cheaper per pointers so they
> don't have that problem.
> 
> Eric