Messages in this thread | | | Subject | Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id | From | Fernando Luis Vázquez Cao <> | Date | Tue, 11 Jul 2006 13:21:01 +0900 |
| |
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
- 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/
| |