lkml.org 
[lkml]   [2006]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id
    From
    Date
    On Tue, 2006-07-11 at 13:21 +0900, Fernando Luis Vázquez Cao wrote:
    > 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?
    >From reading the code, it seems that send_IPI_mask is always safer (we
    avoid the risk of sending an IPI to an offline CPU) and with it we can
    certainly accomplish what we want. I will prepare new patches taking all
    your comments and advices.

    Thank you,

    Fernando

    P.S.: Sorry for replying to myself...

    >
    > 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
    >
    > _______________________________________________
    > fastboot mailing list
    > fastboot@lists.osdl.org
    > https://lists.osdl.org/mailman/listinfo/fastboot

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-07-11 06:47    [W:2.237 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site