Re: [PATCH] EDAC, MCE, AMD: Fix code to prevent NULL dereference

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 2/18/2014 2:46 AM, Borislav Petkov wrote:
> Ok, let's try a simpler thing. Only build-tested here:
>
>   
> +	if (!fam_ops)
> +		return NOTIFY_DONE;
> +
>   	if (amd_filter_mce(m))
>   		return NOTIFY_STOP;
>   
> @@ -816,10 +819,10 @@ static int __init mce_amd_init(void)
>   	struct cpuinfo_x86 *c = &boot_cpu_data;
>   
>   	if (c->x86_vendor != X86_VENDOR_AMD)
> -		return 0;
> +		return -ENODEV;
>   
>   	if (c->x86 < 0xf || c->x86 > 0x16)
> -		return 0;
> +		return -ENODEV;
>   
>   	fam_ops = kzalloc(sizeof(struct amd_decoder_ops), GFP_KERNEL);
>   	if (!fam_ops)
> @@ -874,6 +877,7 @@ static int __init mce_amd_init(void)
>   	default:
>   		printk(KERN_WARNING "Huh? What family is it: 0x%x?!\n", c->x86);
>   		kfree(fam_ops);
> +		fam_ops = NULL;
>   		return -EINVAL;
>   	}
>   
>

This works. But a drawback is that you wouldn't get the output from more 
generic error decoding that happens after the 'switch' in amd_decode_mce:

     pr_emerg(HW_ERR "Error Status: %s\n", decode_error_status(m))
     (etc..) (etc..)
     amd_decode_err_code(m->status & 0xffff);

A quick fix for this is to rearrange the above chunk of code to happen 
before the 'switch'
Tried it on local machine.Here's some sample outputs:

on unsupported h/w:
[   46.822828] [Hardware Error]: Error Status: Uncorrected, software 
containable error.
[   46.822846] [Hardware Error]: CPU:0 (15:30:0) 
MC0_STATUS[-|UE|-|-|-|-|-]: 0xa000000000010f0f
[   46.822858] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, 
mem-tx: GEN, part-proc: GEN (timed out)

on supported h/w:(a MC0 error)
[   84.305292] [Hardware Error]: Error Status: Uncorrected, software 
containable error.
[   84.305312] [Hardware Error]: CPU:0 (15:30:0) 
MC0_STATUS[-|UE|-|-|-|-|-]: 0xa000000000010f0f
[   84.305327] [Hardware Error]: cache level: L3/GEN, mem/io: GEN, 
mem-tx: GEN, part-proc: GEN (timed out)
[   84.305343] [Hardware Error]: MC0 Error:  Internal error condition 
type 1.

on supported h/w:(a MC4 ECC error)
[  128.942878] [Hardware Error]: Error Status: System Fatal error.
[  128.942897] [Hardware Error]: CPU:0 (15:30:0) 
MC4_STATUS[-|UE|-|PCC|AddrV|-|-|UECC]: 0xa600200000080a23
[  128.942914] [Hardware Error]: MC4_ADDR: 0x0000000000000000
[  128.942922] [Hardware Error]: cache level: L3/GEN, mem/io: MEM, 
mem-tx: WR, part-proc: RES (no timeout)
[  128.942939] [Hardware Error]: MC4 Error (node 0): DRAM ECC error 
detected on the NB.
[  128.942971] EDAC MC0: 1 UE on mc#0csrow#2channel#0 (csrow:2 channel:0 
page:0x0 offset:0x0 grain:0)

A word about your earlier suggestion of using amd_notifier_call_chain in 
mce_amd_inj:
The changes will need to be more involved..
- Firstly, x86_mce_decoder_chain is defined in mce.c. So we'd need to 
move it to somewhere in asm/mce.h
- include notifier.h in asm/mce.h (build error saying there are multiple 
definitions of 'x86_mce_decoder_chain' when I tried this.. haven't 
figured out why yet..)
- You'd need to change i_mce to pointer type which in turn will need 
changes in the manner we reference the struct variables in the code

Not sure if you need these many changes, not to mention - touch common 
mce code.
Simpler solution might be to rearrange the code in amd_decode_mce and 
use your hunk..
Thoughts?

Thanks,
-Aravind.