Re: [PATCH v4] powerpc/fadump: fix race between pstore write and fadump crash trigger
Hi Sourabh, Thank you for the patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on linus/master linux/master v5.7 next-20200611] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Sourabh-Jain/powerpc-fadump-fix-race-between-pstore-write-and-fadump-crash-trigger/20200605-033545 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-randconfig-r002-20200612 (attached as .config) compiler: powerpc64le-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): arch/powerpc/kernel/fadump.c: In function 'crash_fadump': >> arch/powerpc/kernel/fadump.c:700:15: error: 'cpus_in_crash' undeclared >> (first use in this function) 700 | atomic_inc(&cpus_in_crash); | ^ arch/powerpc/kernel/fadump.c:700:15: note: each undeclared identifier is reported only once for each function it appears in arch/powerpc/kernel/fadump.c: In function 'fadump_update_elfcore_header': arch/powerpc/kernel/fadump.c:755:17: warning: variable 'elf' set but not used [-Wunused-but-set-variable] 755 | struct elfhdr *elf; | ^~~ arch/powerpc/kernel/fadump.c: At top level: arch/powerpc/kernel/fadump.c:1706:22: warning: no previous prototype for 'arch_reserved_kernel_pages' [-Wmissing-prototypes] 1706 | unsigned long __init arch_reserved_kernel_pages(void) | ^~ vim +/cpus_in_crash +700 arch/powerpc/kernel/fadump.c 678 679 void crash_fadump(struct pt_regs *regs, const char *str) 680 { 681 unsigned int msecs; 682 struct fadump_crash_info_header *fdh = NULL; 683 int old_cpu, this_cpu; 684 unsigned int ncpus = num_online_cpus() - 1; /* Do not include first CPU */ 685 686 if (!should_fadump_crash()) 687 return; 688 689 /* 690 * old_cpu == -1 means this is the first CPU which has come here, 691 * go ahead and trigger fadump. 692 * 693 * old_cpu != -1 means some other CPU has already on it's way 694 * to trigger fadump, just keep looping here. 695 */ 696 this_cpu = smp_processor_id(); 697 old_cpu = cmpxchg(&crashing_cpu, -1, this_cpu); 698 699 if (old_cpu != -1) { > 700 atomic_inc(&cpus_in_crash); 701 702 /* 703 * We can't loop here indefinitely. Wait as long as fadump 704 * is in force. If we race with fadump un-registration this 705 * loop will break and then we go down to normal panic path 706 * and reboot. If fadump is in force the first crashing 707 * cpu will definitely trigger fadump. 708 */ 709 while (fw_dump.dump_registered) 710 cpu_relax(); 711 return; 712 } 713 714 fdh = __va(fw_dump.fadumphdr_addr); 715 fdh->crashing_cpu = crashing_cpu; 716 crash_save_vmcoreinfo(); 717 718 if (regs) 719 fdh->regs = *regs; 720 else 721 ppc_save_regs(&fdh->regs); 722 723 fdh->online_mask = *cpu_online_mask; 724 725 /* 726 * If we came in via system reset, wait a while for the secondary 727 * CPUs to enter. 728 */ 729 if (TRAP(&(fdh->regs)) == 0x100) { 730 msecs = CRASH_TIMEOUT; 731 while ((atomic_read(&cpus_in_crash) < ncpus) && (--msecs > 0)) 732 mdelay(1); 733 } 734 735 fw_dump.ops->fadump_trigger(fdh, str); 736 } 737 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
[PATCH v4] powerpc/fadump: fix race between pstore write and fadump crash trigger
When we enter into fadump crash path via system reset we fail to update the pstore. On the system reset path we first update the pstore then we go for fadump crash. But the problem here is when all the CPUs try to get the pstore lock to initiate the pstore write, only one CPUs will acquire the lock and proceed with the pstore write. Since it in NMI context CPUs that fail to get lock do not wait for their turn to write to the pstore and simply proceed with the next operation which is fadump crash. One of the CPU who proceeded with fadump crash path triggers the crash and does not wait for the CPU who gets the pstore lock to complete the pstore update. Timeline diagram to depicts the sequence of events that leads to an unsuccessful pstore update when we hit fadump crash path via system reset. 12 3... n CPU Threads || | | || | | Reached to -->|--->|>| --->| system reset|| | | path|| | | || | | Try to -->|--->|>|>| acquire the || | | pstore lock || | | || | | || | | Got the -->| +->| | |<-+ pstore lock | | | | | |--> Didn't get the | --+ lock and moving || | |ahead on fadump || | |crash path || | | Begins the -->|| | | process to || | |<-- Got the chance to update the || | |trigger the crash pstore | -> | |... <- | | | | | | | | | | | | |<-- Triggers the | | | | | |crash | | | | | | ^ | | | | | | | Writing to -->| | | | | | | pstore | | | | | | | | | | ^ |__| | | CPU Relax | | | +-+ | v Race: crash triggered before pstore update completes To avoid this race condition a barrier is added on crash_fadump path, it prevents the CPU to trigger the crash until all the online CPUs completes their task. A barrier is added to make sure all the secondary CPUs hit the crash_fadump function before we initiates the crash. A timeout is kept to ensure the primary CPU (one who initiates the crash) do not wait for secondary CPUs indefinitely. Signed-off-by: Sourabh Jain --- arch/powerpc/kernel/fadump.c | 24 1 file changed, 24 insertions(+) --- Chanagelog: v1 -> v3: - https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-April/208267.html v3 -> v4: - Now the primary CPU (one who triggers dump) waits for all secondary CPUs to enter and then initiates the crash. --- diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index 59e60a9a9f5c..4953f3246220 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -32,6 +32,14 @@ #include #include +/* + * The CPU who acquired the lock to trigger the fadump crash should + * wait for other CPUs to enter. + * + * The timeout is in milliseconds. + */ +#define CRASH_TIMEOUT 500 + static struct fw_dump fw_dump; static void __init fadump_reserve_crash_area(u64 base); @@ -46,6 +54,8 @@ struct fadump_mrange_info reserved_mrange_info = { "reserved", NULL, 0, 0, 0 }; #ifdef CONFIG_CMA static struct cma *fadump_cma; +static atomic_t cpus_in_crash; + /* * fadump_cma_init() - Initialize CMA area from a fadump reserved memory * @@ -596,8 +606,10 @@ early_param("fadump_reserve_mem", early_fadump_reserve_mem); void crash_fadump(struct pt_regs *regs, const char *str) { + unsigned int msecs; struct fadump_crash_info_header *fdh = NULL; int old_cpu, this_cpu; + unsigned int ncpus = num_online_cpus() - 1; /* Do not include first CPU */ if (!should_fadump_crash()) return; @@ -613,6 +625,8 @@ void crash_fadump(struct pt_regs *regs, const char *str) old_cpu = cmpxchg(&crashing_cpu, -1, this_cpu); if (old_cpu != -1) { + atomic_inc(&cpus_in_crash); + /* * We can't loop here indefinitely. Wait as long as fadump * is in force. If we race with fadump un-registration th