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

Hi Eric!

On Mon, 2006-07-10 at 05:37 -0600, Eric W. Biederman wrote:
Fernando Luis Vázquez Cao <fernando@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages