Re: [PATCH] kexec-tools: ppc64: avoid adding coherent memory regions to crash memory ranges

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

On Monday 28 August 2017 07:07 PM, Simon Horman wrote:
> On Sun, Aug 27, 2017 at 11:12:37PM -0400, Pingfan Liu wrote:
>>
>>
>>
>> ----- Original Message -----
>>> From: "Hari Bathini" <hbathini@linux.vnet.ibm.com>
>>> To: "Simon Horman" <horms@verge.net.au>, "Kexec-ml" <kexec@lists.infradead.org>
>>> Cc: "Ankit Kumar" <ankit@linux.vnet.ibm.com>, "Anshuman Khandual" <khandual@linux.vnet.ibm.com>, "Ananth N
>>> Mavinakayanahalli" <ananth@linux.vnet.ibm.com>, "Alistair Popple" <alistair@popple.id.au>
>>> Sent: Thursday, August 17, 2017 8:31:51 PM
>>> Subject: [PATCH] kexec-tools: ppc64: avoid adding coherent memory regions to crash memory ranges
>>>
>>> Accelerator devices like GPU and FPGA cards contain onboard memory. This
>>> onboard memory is represented as a memory only NUMA node, integrating it
>>> with core memory subsystem. Since, the link through which these devices
>>> are integrated to core memory goes down after a system crash and they are
>>> meant for user workloads, avoid adding coherent device memory regions to
>>> crash memory ranges. Without this change, makedumpfile tool tries to save
>>> unaccessible coherent device memory regions, crashing the system.
>>>
>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>> ---
>>>   kexec/arch/ppc64/crashdump-ppc64.c |   64
>>>   +++++++++++++++++++++++++++++++++++-
>>>   kexec/arch/ppc64/kexec-ppc64.h     |    1 +
>>>   2 files changed, 63 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kexec/arch/ppc64/crashdump-ppc64.c
>>> b/kexec/arch/ppc64/crashdump-ppc64.c
>>> index 13995bf..7ea3983 100644
>>> --- a/kexec/arch/ppc64/crashdump-ppc64.c
>>> +++ b/kexec/arch/ppc64/crashdump-ppc64.c
>>> @@ -181,6 +181,53 @@ static int get_dyn_reconf_crash_memory_ranges(void)
>>>   	return 0;
>>>   }
>>>   
>>> +/*
>>> + * For a given memory node, check if it is mapped to system RAM or
>>> + * to onboard memory on accelerator device like GPU card or such.
>>> + */
>>> +static int is_coherent_device_mem(const char *fname)
>>> +{
>>> +	char fpath[PATH_LEN];
>>> +	char buf[32];
>>> +	DIR *dmem;
>>> +	FILE *file;
>>> +	struct dirent *mentry;
>>> +	int cnt, ret = 0;
>>> +
>>> +	strcpy(fpath, fname);
>>> +	if ((dmem = opendir(fpath)) == NULL) {
>>> +		perror(fpath);
>>> +		return -1;
>>> +	}
>>> +
>>> +	while ((mentry = readdir(dmem)) != NULL) {
>>> +		if (strcmp(mentry->d_name, "compatible"))
>>> +			continue;
>>> +
>>> +		strcat(fpath, "/compatible");
>>> +		if ((file = fopen(fpath, "r")) == NULL) {
>>> +			perror(fpath);
>>> +			ret = -1;
>>> +			break;
>>> +		}
>>> +		if ((cnt = fread(buf, 1, 32, file)) < 0) {
>>> +			perror(fpath);
>>> +			fclose(file);
>>> +			ret = -1;
>>> +			break;
>>> +		}
>>> +		if (!strncmp(buf, "ibm,coherent-device-memory", 26)) {
>>> +			ret = 1;
>>> +			break;
> This seems to leak file.
>
>>> +		}
>>> +		fclose(file);
>>> +	}
>>> +
>>> +	closedir(dmem);
>>> +	return ret;
>>> +}
>>> +
>>> +
>>>   /* Reads the appropriate file and retrieves the SYSTEM RAM regions for whom
>>>   to
>>>    * create Elf headers. Keeping it separate from get_memory_ranges() as
>>>    * requirements are different in the case of normal kexec and crashdumps.
>>> @@ -196,12 +243,12 @@ static int get_crash_memory_ranges(struct memory_range
>>> **range, int *ranges)
>>>   {
>>>   
>>>   	char device_tree[256] = "/proc/device-tree/";
>>> -	char fname[256];
>>> +	char fname[PATH_LEN];
>>>   	char buf[MAXBYTES];
>>>   	DIR *dir, *dmem;
>>>   	FILE *file;
>>>   	struct dirent *dentry, *mentry;
>>> -	int n, crash_rng_len = 0;
>>> +	int n, ret, crash_rng_len = 0;
>>>   	unsigned long long start, end;
>>>   	int page_size;
>>>   
>>> @@ -240,6 +287,19 @@ static int get_crash_memory_ranges(struct memory_range
>>> **range, int *ranges)
>>>   			continue;
>>>   		strcpy(fname, device_tree);
>>>   		strcat(fname, dentry->d_name);
>>> +
>>> +		ret = is_coherent_device_mem(fname);
>>> +		if (ret == -1) {
>>> +			closedir(dir);
>>> +			goto err;
>>> +		} else if (ret == 1) {
>>> +			/*
>>> +			 * Avoid adding this memory region as it is not
>>> +			 * mapped to system RAM.
>>> +			 */
>>> +			continue;
>>> +		}
>>> +
>>>   		if ((dmem = opendir(fname)) == NULL) {
>>>   			perror(fname);
>>>   			closedir(dir);
>>> diff --git a/kexec/arch/ppc64/kexec-ppc64.h b/kexec/arch/ppc64/kexec-ppc64.h
>>> index 633ae77..434b4bf 100644
>>> --- a/kexec/arch/ppc64/kexec-ppc64.h
>>> +++ b/kexec/arch/ppc64/kexec-ppc64.h
>>> @@ -1,6 +1,7 @@
>>>   #ifndef KEXEC_PPC64_H
>>>   #define KEXEC_PPC64_H
>>>   
>>> +#define PATH_LEN 256
>>>   #define MAXBYTES 128
>>>   #define MAX_LINE 160
>>>   #define CORE_TYPE_ELF32 1
>>>
>> Tested-by: Pingfan Liu <piliu@redhat.com>
> The above not withstanding I have applied this patch with Pingfan's tag.
> Please post a follow-up patch as appropriate.

Thanks, Simon.
Posted the follow-up patch at 
http://lists.infradead.org/pipermail/kexec/2017-August/019439.html

- Hari


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec