Re: [PATCH&RFC 2/2] OS_MCA Recovery from poisoned memory read

[Keith Owens <kaos@sgi.com>]

On Fri, 06 Aug 2004 21:17:39 +0900, 
Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> wrote:
>Thank you for your useful reply.
>
>But, there is one thing that I want to confirm.
>
>Keith Owens wrote:
>> +static isolate_status_t
>> +mca_page_isolate(unsigned long paddr)
>> +{
>> +       int i;
>> +       struct page *p;
>> +
>> +       /* whether physical address is valid or not */
>> +       if ( !ia64_phys_addr_valid(paddr) )  
>> 
>> The calls to mca_page_isolate() are racy.  That code is running in
>> normal kernel context after exiting from the MCA handler.  Other cpus
>> could be modifying the page tables at the same time, there could even
>> be two cpus running mca_handler_bh() at the same time for the same
>> page.
>
>I agree that there could be multiple cpus running handler_bh at the
>same time, so (even though it would be a rare case) I think it would be
>better if I avoid the race using something like a spinlock.
>
>ITOH, what the handler_bh should modify is not the page tables but the
>flag in a struct page which pfn_to_page convert from a physical address.
>Does the result of the translation from a physical address to a page that
>includes the address can be changed? (Do you suppose Memory Hotplugs?)

I had a quick look through mm/page_alloc.c and mm/memory.c.  Since
these are user pages, handler_bh should be able to get
mm->page_table_lock.  But what if the MCA occurred while the process
was already holding mm->page_table_lock?  Then mca_page_isolate() would
deadlock.

mca_handler_bh() is running as an extension of the MCA event which
means that it is not irq safe.  It is not safe to get any external lock
in mca_page_isolate() or mca_handler_bh().  Even calling printk() from
mca_handler_bh() is risky, if the MCA occurred during printk handling
then the printk call from mca_handler_bh() would deadlock on
logbuf_lock.

mca_handler_bh() can only lock against itself.  It is not safe to get
any external locks.

I am also concerned about the code in mca_handler_bh() that calls
schedule with SIGKILL set.  Again that is running as an extension of
the MCA event (not irq safe), which means that it could still own
locks, or even have interrupts disabled.

AFAICT, my concerns about the MCA event and mca_handler_bh() not being
irq safe are only a problem for the case when the MCA was triggered by
user space code but was delivered when the cpu was in kernel code.
Maybe we do not support the problem case.

*    offending process  affected process  OS MCA do
*     kernel mode        kernel mode       down system
*     kernel mode        user   mode       kill the process
*     user   mode        kernel mode       kill the process <== problem
*     user   mode        user   mode       kill the process