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 <[email protected]> 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
> [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
