All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
@ 2019-12-03 22:27 Mikhail Zaslonko
  2019-12-09 22:02 ` Kazuhito Hagio
  0 siblings, 1 reply; 10+ messages in thread
From: Mikhail Zaslonko @ 2019-12-03 22:27 UTC (permalink / raw)
  To: kexec; +Cc: prudo, k-hagio, dyoung

Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
support has been added yet. This patch adds the arch specific function
get_kaslr_offset() for s390x.
Since the values in vmcoreinfo are already relocated, the patch is
mainly relevant for vmlinux processing (-x option).

Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
---
 arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
 makedumpfile.h |  3 ++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/s390x.c b/arch/s390x.c
index bf9d58e..892df14 100644
--- a/arch/s390x.c
+++ b/arch/s390x.c
@@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
 	return TRUE;
 }
 
+unsigned long
+get_kaslr_offset_s390x(unsigned long vaddr)
+{
+	unsigned int i;
+	char buf[BUFSIZE_FGETS], *endp;
+
+	if (!info->file_vmcoreinfo)
+		return FALSE;
+
+	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
+		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
+		       info->name_vmcoreinfo, strerror(errno));
+		return FALSE;
+	}
+
+	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
+		i = strlen(buf);
+		if (!i)
+			break;
+		if (buf[i - 1] == '\n')
+			buf[i - 1] = '\0';
+		if (strncmp(buf, STR_KERNELOFFSET,
+			    strlen(STR_KERNELOFFSET)) == 0) {
+			info->kaslr_offset =
+				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
+			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
+		}
+	}
+
+	return info->kaslr_offset;
+}
+
 static int
 is_vmalloc_addr_s390x(unsigned long vaddr)
 {
diff --git a/makedumpfile.h b/makedumpfile.h
index ac11e90..26f6247 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
 int get_machdep_info_s390x(void);
 unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
 int is_iomem_phys_addr_s390x(unsigned long addr);
+unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
 #define find_vmemmap()		stub_false()
 #define get_phys_base()		stub_true()
 #define get_machdep_info()	get_machdep_info_s390x()
 #define get_versiondep_info()	stub_true()
-#define get_kaslr_offset(X)	stub_false()
+#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
 #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
 #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
 #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-03 22:27 [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x Mikhail Zaslonko
@ 2019-12-09 22:02 ` Kazuhito Hagio
  2019-12-12  9:50   ` Zaslonko Mikhail
  0 siblings, 1 reply; 10+ messages in thread
From: Kazuhito Hagio @ 2019-12-09 22:02 UTC (permalink / raw)
  To: Mikhail Zaslonko, kexec; +Cc: prudo, dyoung

Hi Mikhail,

Sorry for late reply.

> -----Original Message-----
> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
> support has been added yet. This patch adds the arch specific function
> get_kaslr_offset() for s390x.
> Since the values in vmcoreinfo are already relocated, the patch is
> mainly relevant for vmlinux processing (-x option).

In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
is supposed to return the KASLR offset only when the offset is needed to
add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
symbols from modules are resolved dynamically and don't need the offset.

This patch always returns the offset if any, as a result, I guess this patch
will not work as expected with module symbols in filter config file.

So... How about making get_kaslr_offset_arm64() general for other archs
(get_kaslr_offset_general() or something), then using it also for s390?
If OK, I can do that generalization.

Thanks,
Kazu

> 
> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> ---
>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>  makedumpfile.h |  3 ++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390x.c b/arch/s390x.c
> index bf9d58e..892df14 100644
> --- a/arch/s390x.c
> +++ b/arch/s390x.c
> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>  	return TRUE;
>  }
> 
> +unsigned long
> +get_kaslr_offset_s390x(unsigned long vaddr)
> +{
> +	unsigned int i;
> +	char buf[BUFSIZE_FGETS], *endp;
> +
> +	if (!info->file_vmcoreinfo)
> +		return FALSE;
> +
> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
> +		       info->name_vmcoreinfo, strerror(errno));
> +		return FALSE;
> +	}
> +
> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
> +		i = strlen(buf);
> +		if (!i)
> +			break;
> +		if (buf[i - 1] == '\n')
> +			buf[i - 1] = '\0';
> +		if (strncmp(buf, STR_KERNELOFFSET,
> +			    strlen(STR_KERNELOFFSET)) == 0) {
> +			info->kaslr_offset =
> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> +		}
> +	}
> +
> +	return info->kaslr_offset;
> +}
> +
>  static int
>  is_vmalloc_addr_s390x(unsigned long vaddr)
>  {
> diff --git a/makedumpfile.h b/makedumpfile.h
> index ac11e90..26f6247 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>  int get_machdep_info_s390x(void);
>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>  int is_iomem_phys_addr_s390x(unsigned long addr);
> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>  #define find_vmemmap()		stub_false()
>  #define get_phys_base()		stub_true()
>  #define get_machdep_info()	get_machdep_info_s390x()
>  #define get_versiondep_info()	stub_true()
> -#define get_kaslr_offset(X)	stub_false()
> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
> --
> 2.17.1
> 



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-09 22:02 ` Kazuhito Hagio
@ 2019-12-12  9:50   ` Zaslonko Mikhail
  2019-12-12 16:12     ` Kazuhito Hagio
  0 siblings, 1 reply; 10+ messages in thread
From: Zaslonko Mikhail @ 2019-12-12  9:50 UTC (permalink / raw)
  To: Kazuhito Hagio, kexec; +Cc: prudo, dyoung

Hello Kazu,

I think we can try to generalize the kaslr offset extraction. 
I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64() 
should work fine. The only concern of mine is this TODO statement:

if (_text <= vaddr && vaddr <= _end) {
	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
	return info->kaslr_offset;
	} else {
	/*
	* TODO: we need to check if it is vmalloc/vmmemmap/module
	* address, we will have different offset
	*/
	return 0;
}

Could you explain this one?

Thanks,
Mikhail

On 09.12.2019 23:02, Kazuhito Hagio wrote:
> Hi Mikhail,
> 
> Sorry for late reply.
> 
>> -----Original Message-----
>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
>> support has been added yet. This patch adds the arch specific function
>> get_kaslr_offset() for s390x.
>> Since the values in vmcoreinfo are already relocated, the patch is
>> mainly relevant for vmlinux processing (-x option).
> 
> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
> is supposed to return the KASLR offset only when the offset is needed to
> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
> symbols from modules are resolved dynamically and don't need the offset.
\> 
> This patch always returns the offset if any, as a result, I guess this patch
> will not work as expected with module symbols in filter config file.
> 
> So... How about making get_kaslr_offset_arm64() general for other archs
> (get_kaslr_offset_general() or something), then using it also for s390?
> If OK, I can do that generalization.
> 
> Thanks,
> Kazu
> 
>>
>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
>> ---
>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>>  makedumpfile.h |  3 ++-
>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390x.c b/arch/s390x.c
>> index bf9d58e..892df14 100644
>> --- a/arch/s390x.c
>> +++ b/arch/s390x.c
>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>  	return TRUE;
>>  }
>>
>> +unsigned long
>> +get_kaslr_offset_s390x(unsigned long vaddr)
>> +{
>> +	unsigned int i;
>> +	char buf[BUFSIZE_FGETS], *endp;
>> +
>> +	if (!info->file_vmcoreinfo)
>> +		return FALSE;
>> +
>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>> +		       info->name_vmcoreinfo, strerror(errno));
>> +		return FALSE;
>> +	}
>> +
>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>> +		i = strlen(buf);
>> +		if (!i)
>> +			break;
>> +		if (buf[i - 1] == '\n')
>> +			buf[i - 1] = '\0';
>> +		if (strncmp(buf, STR_KERNELOFFSET,
>> +			    strlen(STR_KERNELOFFSET)) == 0) {
>> +			info->kaslr_offset =
>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>> +		}
>> +	}
>> +
>> +	return info->kaslr_offset;
>> +}
>> +
>>  static int
>>  is_vmalloc_addr_s390x(unsigned long vaddr)
>>  {
>> diff --git a/makedumpfile.h b/makedumpfile.h
>> index ac11e90..26f6247 100644
>> --- a/makedumpfile.h
>> +++ b/makedumpfile.h
>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>>  int get_machdep_info_s390x(void);
>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>  int is_iomem_phys_addr_s390x(unsigned long addr);
>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>  #define find_vmemmap()		stub_false()
>>  #define get_phys_base()		stub_true()
>>  #define get_machdep_info()	get_machdep_info_s390x()
>>  #define get_versiondep_info()	stub_true()
>> -#define get_kaslr_offset(X)	stub_false()
>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
>> --
>> 2.17.1
>>
> 
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-12  9:50   ` Zaslonko Mikhail
@ 2019-12-12 16:12     ` Kazuhito Hagio
  2019-12-12 16:31       ` Zaslonko Mikhail
  0 siblings, 1 reply; 10+ messages in thread
From: Kazuhito Hagio @ 2019-12-12 16:12 UTC (permalink / raw)
  To: Zaslonko Mikhail, kexec; +Cc: prudo, dyoung

Hi Mikhail,

> -----Original Message-----
> Hello Kazu,
> 
> I think we can try to generalize the kaslr offset extraction.
> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
> should work fine. The only concern of mine is this TODO statement:
> 
> if (_text <= vaddr && vaddr <= _end) {
> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> 	return info->kaslr_offset;
> 	} else {
> 	/*
> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
> 	* address, we will have different offset
> 	*/
> 	return 0;
> }
> 
> Could you explain this one?

Probably it was considered that the check would be needed to support
the whole KASLR behavior when get_kaslr_offset_x86_64() was written
originally.

But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
the offset we need is the one for symbol addresses in vmlinux only.
As I said below, module symbol addresses are retrieved from vmcore.
Other addresses should not be passed to the function for now, as far
as I know.

So I think the TODO comment is confusing, and it would be better to
remove it or change it to something like:
/*
 * Returns 0 if vaddr does not need the offset to be added,
 * e.g. for module address.
 */

But if s390 uses get_kaslr_offset() in its arch-specific code to
adjust addresses other than kernel text address, we might need to
modify it for s390, not generalize it.

Thanks,
Kazu

> 
> Thanks,
> Mikhail
> 
> On 09.12.2019 23:02, Kazuhito Hagio wrote:
> > Hi Mikhail,
> >
> > Sorry for late reply.
> >
> >> -----Original Message-----
> >> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
> >> support has been added yet. This patch adds the arch specific function
> >> get_kaslr_offset() for s390x.
> >> Since the values in vmcoreinfo are already relocated, the patch is
> >> mainly relevant for vmlinux processing (-x option).
> >
> > In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
> > is supposed to return the KASLR offset only when the offset is needed to
> > add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
> > symbols from modules are resolved dynamically and don't need the offset.
> \>
> > This patch always returns the offset if any, as a result, I guess this patch
> > will not work as expected with module symbols in filter config file.
> >
> > So... How about making get_kaslr_offset_arm64() general for other archs
> > (get_kaslr_offset_general() or something), then using it also for s390?
> > If OK, I can do that generalization.
> >
> > Thanks,
> > Kazu
> >
> >>
> >> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
> >> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> >> ---
> >>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
> >>  makedumpfile.h |  3 ++-
> >>  2 files changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/s390x.c b/arch/s390x.c
> >> index bf9d58e..892df14 100644
> >> --- a/arch/s390x.c
> >> +++ b/arch/s390x.c
> >> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
> >>  	return TRUE;
> >>  }
> >>
> >> +unsigned long
> >> +get_kaslr_offset_s390x(unsigned long vaddr)
> >> +{
> >> +	unsigned int i;
> >> +	char buf[BUFSIZE_FGETS], *endp;
> >> +
> >> +	if (!info->file_vmcoreinfo)
> >> +		return FALSE;
> >> +
> >> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
> >> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
> >> +		       info->name_vmcoreinfo, strerror(errno));
> >> +		return FALSE;
> >> +	}
> >> +
> >> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
> >> +		i = strlen(buf);
> >> +		if (!i)
> >> +			break;
> >> +		if (buf[i - 1] == '\n')
> >> +			buf[i - 1] = '\0';
> >> +		if (strncmp(buf, STR_KERNELOFFSET,
> >> +			    strlen(STR_KERNELOFFSET)) == 0) {
> >> +			info->kaslr_offset =
> >> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
> >> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> >> +		}
> >> +	}
> >> +
> >> +	return info->kaslr_offset;
> >> +}
> >> +
> >>  static int
> >>  is_vmalloc_addr_s390x(unsigned long vaddr)
> >>  {
> >> diff --git a/makedumpfile.h b/makedumpfile.h
> >> index ac11e90..26f6247 100644
> >> --- a/makedumpfile.h
> >> +++ b/makedumpfile.h
> >> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
> >>  int get_machdep_info_s390x(void);
> >>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
> >>  int is_iomem_phys_addr_s390x(unsigned long addr);
> >> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
> >>  #define find_vmemmap()		stub_false()
> >>  #define get_phys_base()		stub_true()
> >>  #define get_machdep_info()	get_machdep_info_s390x()
> >>  #define get_versiondep_info()	stub_true()
> >> -#define get_kaslr_offset(X)	stub_false()
> >> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
> >>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
> >>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
> >>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
> >> --
> >> 2.17.1
> >>
> >
> >



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-12 16:12     ` Kazuhito Hagio
@ 2019-12-12 16:31       ` Zaslonko Mikhail
  2019-12-17 17:46         ` Kazuhito Hagio
  0 siblings, 1 reply; 10+ messages in thread
From: Zaslonko Mikhail @ 2019-12-12 16:31 UTC (permalink / raw)
  To: Kazuhito Hagio, kexec; +Cc: prudo, dyoung

Hi,

On 12.12.2019 17:12, Kazuhito Hagio wrote:
> Hi Mikhail,
> 
>> -----Original Message-----
>> Hello Kazu,
>>
>> I think we can try to generalize the kaslr offset extraction.
>> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
>> should work fine. The only concern of mine is this TODO statement:
>>
>> if (_text <= vaddr && vaddr <= _end) {
>> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>> 	return info->kaslr_offset;
>> 	} else {
>> 	/*
>> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
>> 	* address, we will have different offset
>> 	*/
>> 	return 0;
>> }
>>
>> Could you explain this one?
> 
> Probably it was considered that the check would be needed to support
> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
> originally.
> 
> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
> the offset we need is the one for symbol addresses in vmlinux only.
> As I said below, module symbol addresses are retrieved from vmcore.
> Other addresses should not be passed to the function for now, as far
> as I know.
> 
> So I think the TODO comment is confusing, and it would be better to
> remove it or change it to something like:
> /*
>  * Returns 0 if vaddr does not need the offset to be added,
>  * e.g. for module address.
>  */
> 
> But if s390 uses get_kaslr_offset() in its arch-specific code to
> adjust addresses other than kernel text address, we might need to
> modify it for s390, not generalize it.

Currently, s390 doesn't use get_kaslr_offset() in its arch-specific 
code. 

> 
> Thanks,
> Kazu
> 
>>
>> Thanks,
>> Mikhail
>>
>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
>>> Hi Mikhail,
>>>
>>> Sorry for late reply.
>>>
>>>> -----Original Message-----
>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
>>>> support has been added yet. This patch adds the arch specific function
>>>> get_kaslr_offset() for s390x.
>>>> Since the values in vmcoreinfo are already relocated, the patch is
>>>> mainly relevant for vmlinux processing (-x option).
>>>
>>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
>>> is supposed to return the KASLR offset only when the offset is needed to
>>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
>>> symbols from modules are resolved dynamically and don't need the offset.
>> \>
>>> This patch always returns the offset if any, as a result, I guess this patch
>>> will not work as expected with module symbols in filter config file.
>>>
>>> So... How about making get_kaslr_offset_arm64() general for other archs
>>> (get_kaslr_offset_general() or something), then using it also for s390?
>>> If OK, I can do that generalization.
>>>
>>> Thanks,
>>> Kazu
>>>
>>>>
>>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
>>>> ---
>>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>>>>  makedumpfile.h |  3 ++-
>>>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390x.c b/arch/s390x.c
>>>> index bf9d58e..892df14 100644
>>>> --- a/arch/s390x.c
>>>> +++ b/arch/s390x.c
>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>>>  	return TRUE;
>>>>  }
>>>>
>>>> +unsigned long
>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
>>>> +{
>>>> +	unsigned int i;
>>>> +	char buf[BUFSIZE_FGETS], *endp;
>>>> +
>>>> +	if (!info->file_vmcoreinfo)
>>>> +		return FALSE;
>>>> +
>>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>>>> +		       info->name_vmcoreinfo, strerror(errno));
>>>> +		return FALSE;
>>>> +	}
>>>> +
>>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>>>> +		i = strlen(buf);
>>>> +		if (!i)
>>>> +			break;
>>>> +		if (buf[i - 1] == '\n')
>>>> +			buf[i - 1] = '\0';
>>>> +		if (strncmp(buf, STR_KERNELOFFSET,
>>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
>>>> +			info->kaslr_offset =
>>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
>>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return info->kaslr_offset;
>>>> +}
>>>> +
>>>>  static int
>>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
>>>>  {
>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>> index ac11e90..26f6247 100644
>>>> --- a/makedumpfile.h
>>>> +++ b/makedumpfile.h
>>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>>>>  int get_machdep_info_s390x(void);
>>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>>>  #define find_vmemmap()		stub_false()
>>>>  #define get_phys_base()		stub_true()
>>>>  #define get_machdep_info()	get_machdep_info_s390x()
>>>>  #define get_versiondep_info()	stub_true()
>>>> -#define get_kaslr_offset(X)	stub_false()
>>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
>>>> --
>>>> 2.17.1
>>>>
>>>
>>>
> 
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-12 16:31       ` Zaslonko Mikhail
@ 2019-12-17 17:46         ` Kazuhito Hagio
  2019-12-26  3:38           ` lijiang
  0 siblings, 1 reply; 10+ messages in thread
From: Kazuhito Hagio @ 2019-12-17 17:46 UTC (permalink / raw)
  To: Zaslonko Mikhail, kexec; +Cc: prudo, dyoung

Hi Mikhail,

> -----Original Message-----
> Hi,
> 
> On 12.12.2019 17:12, Kazuhito Hagio wrote:
> > Hi Mikhail,
> >
> >> -----Original Message-----
> >> Hello Kazu,
> >>
> >> I think we can try to generalize the kaslr offset extraction.
> >> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
> >> should work fine. The only concern of mine is this TODO statement:
> >>
> >> if (_text <= vaddr && vaddr <= _end) {
> >> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> >> 	return info->kaslr_offset;
> >> 	} else {
> >> 	/*
> >> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
> >> 	* address, we will have different offset
> >> 	*/
> >> 	return 0;
> >> }
> >>
> >> Could you explain this one?
> >
> > Probably it was considered that the check would be needed to support
> > the whole KASLR behavior when get_kaslr_offset_x86_64() was written
> > originally.
> >
> > But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
> > the offset we need is the one for symbol addresses in vmlinux only.
> > As I said below, module symbol addresses are retrieved from vmcore.
> > Other addresses should not be passed to the function for now, as far
> > as I know.
> >
> > So I think the TODO comment is confusing, and it would be better to
> > remove it or change it to something like:
> > /*
> >  * Returns 0 if vaddr does not need the offset to be added,
> >  * e.g. for module address.
> >  */
> >
> > But if s390 uses get_kaslr_offset() in its arch-specific code to
> > adjust addresses other than kernel text address, we might need to
> > modify it for s390, not generalize it.
> 
> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
> code.

OK, I pushed a patch that generalizes it to my test repository.
Could you enable s390 to use it and test?
https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general

Thanks,
Kazu

> 
> >
> > Thanks,
> > Kazu
> >
> >>
> >> Thanks,
> >> Mikhail
> >>
> >> On 09.12.2019 23:02, Kazuhito Hagio wrote:
> >>> Hi Mikhail,
> >>>
> >>> Sorry for late reply.
> >>>
> >>>> -----Original Message-----
> >>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
> >>>> support has been added yet. This patch adds the arch specific function
> >>>> get_kaslr_offset() for s390x.
> >>>> Since the values in vmcoreinfo are already relocated, the patch is
> >>>> mainly relevant for vmlinux processing (-x option).
> >>>
> >>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
> >>> is supposed to return the KASLR offset only when the offset is needed to
> >>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
> >>> symbols from modules are resolved dynamically and don't need the offset.
> >> \>
> >>> This patch always returns the offset if any, as a result, I guess this patch
> >>> will not work as expected with module symbols in filter config file.
> >>>
> >>> So... How about making get_kaslr_offset_arm64() general for other archs
> >>> (get_kaslr_offset_general() or something), then using it also for s390?
> >>> If OK, I can do that generalization.
> >>>
> >>> Thanks,
> >>> Kazu
> >>>
> >>>>
> >>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
> >>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> >>>> ---
> >>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
> >>>>  makedumpfile.h |  3 ++-
> >>>>  2 files changed, 34 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/s390x.c b/arch/s390x.c
> >>>> index bf9d58e..892df14 100644
> >>>> --- a/arch/s390x.c
> >>>> +++ b/arch/s390x.c
> >>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
> >>>>  	return TRUE;
> >>>>  }
> >>>>
> >>>> +unsigned long
> >>>> +get_kaslr_offset_s390x(unsigned long vaddr)
> >>>> +{
> >>>> +	unsigned int i;
> >>>> +	char buf[BUFSIZE_FGETS], *endp;
> >>>> +
> >>>> +	if (!info->file_vmcoreinfo)
> >>>> +		return FALSE;
> >>>> +
> >>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
> >>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
> >>>> +		       info->name_vmcoreinfo, strerror(errno));
> >>>> +		return FALSE;
> >>>> +	}
> >>>> +
> >>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
> >>>> +		i = strlen(buf);
> >>>> +		if (!i)
> >>>> +			break;
> >>>> +		if (buf[i - 1] == '\n')
> >>>> +			buf[i - 1] = '\0';
> >>>> +		if (strncmp(buf, STR_KERNELOFFSET,
> >>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
> >>>> +			info->kaslr_offset =
> >>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
> >>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	return info->kaslr_offset;
> >>>> +}
> >>>> +
> >>>>  static int
> >>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
> >>>>  {
> >>>> diff --git a/makedumpfile.h b/makedumpfile.h
> >>>> index ac11e90..26f6247 100644
> >>>> --- a/makedumpfile.h
> >>>> +++ b/makedumpfile.h
> >>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
> >>>>  int get_machdep_info_s390x(void);
> >>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
> >>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
> >>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
> >>>>  #define find_vmemmap()		stub_false()
> >>>>  #define get_phys_base()		stub_true()
> >>>>  #define get_machdep_info()	get_machdep_info_s390x()
> >>>>  #define get_versiondep_info()	stub_true()
> >>>> -#define get_kaslr_offset(X)	stub_false()
> >>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
> >>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
> >>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
> >>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>
> >>>
> >
> >



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-17 17:46         ` Kazuhito Hagio
@ 2019-12-26  3:38           ` lijiang
  2019-12-26  4:07             ` lijiang
  0 siblings, 1 reply; 10+ messages in thread
From: lijiang @ 2019-12-26  3:38 UTC (permalink / raw)
  To: kexec, Kazuhito Hagio, Mikhail Zaslonko

Hi, Kazu and Mikhail,

> Hi Mikhail,
> 
>> -----Original Message-----
>> Hi,
>>
>> On 12.12.2019 17:12, Kazuhito Hagio wrote:
>>> Hi Mikhail,
>>>
>>>> -----Original Message-----
>>>> Hello Kazu,
>>>>
>>>> I think we can try to generalize the kaslr offset extraction.
>>>> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
>>>> should work fine. The only concern of mine is this TODO statement:
>>>>
>>>> if (_text <= vaddr && vaddr <= _end) {
>>>> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>> 	return info->kaslr_offset;
>>>> 	} else {
>>>> 	/*
>>>> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
>>>> 	* address, we will have different offset
>>>> 	*/
>>>> 	return 0;
>>>> }
>>>>
>>>> Could you explain this one?
>>>
>>> Probably it was considered that the check would be needed to support
>>> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
>>> originally.
>>>
>>> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
>>> the offset we need is the one for symbol addresses in vmlinux only.
>>> As I said below, module symbol addresses are retrieved from vmcore.
>>> Other addresses should not be passed to the function for now, as far
>>> as I know.
>>>
>>> So I think the TODO comment is confusing, and it would be better to
>>> remove it or change it to something like:
>>> /*
>>>  * Returns 0 if vaddr does not need the offset to be added,
>>>  * e.g. for module address.
>>>  */
>>>
>>> But if s390 uses get_kaslr_offset() in its arch-specific code to
>>> adjust addresses other than kernel text address, we might need to
>>> modify it for s390, not generalize it.
>>
>> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
>> code.
> 
> OK, I pushed a patch that generalizes it to my test repository.
> Could you enable s390 to use it and test?
> https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general
> 

I enabled it on s390 as below and tested, it worked.

@@ -1075,7 +1075,7 @@ int is_iomem_phys_addr_s390x(unsigned long addr);
 #define get_phys_base()                stub_true()
 #define get_machdep_info()     get_machdep_info_s390x()
 #define get_versiondep_info()  stub_true()
-#define get_kaslr_offset(X)    stub_false()
+#define get_kaslr_offset(X)    get_kaslr_offset_general(X)
 #define vaddr_to_paddr(X)      vaddr_to_paddr_s390x(X)

But, there is still a problem that needs to be improved. In the find_kaslr_offsets(),
the value of SYMBOL(_stext) is always 0(zero) and it is passed to the get_kaslr_offset().
For the following code in the get_kaslr_offset_general(), it does not work as expected.
...
	if (_text <= vaddr && vaddr <= _end)
		return info->kaslr_offset;
	else
		return 0;
...
Here is my log:
get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:0

After applied the following patch, got the expected result.
 int
 find_kaslr_offsets()
 {
@@ -3973,6 +4042,11 @@ find_kaslr_offsets()
         * called this function between open_vmcoreinfo() and
         * close_vmcoreinfo()
         */
+       READ_SYMBOL("_stext", _stext);
+       if (SYMBOL(_stext) == NOT_FOUND_SYMBOL) {
+                ERRMSG("Can't get the symbol of _stext.\n");
+                goto out;
+       }
        get_kaslr_offset(SYMBOL(_stext));

Here is my log:
get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:67fbc000

Basically, before using the value of SYMBOL(_stext), need to ensure that the SYMBOL(_stext) is parsed
correctly.

Thanks.

> Thanks,
> Kazu
> 
>>
>>>
>>> Thanks,
>>> Kazu
>>>
>>>>
>>>> Thanks,
>>>> Mikhail
>>>>
>>>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
>>>>> Hi Mikhail,
>>>>>
>>>>> Sorry for late reply.
>>>>>
>>>>>> -----Original Message-----
>>>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
>>>>>> support has been added yet. This patch adds the arch specific function
>>>>>> get_kaslr_offset() for s390x.
>>>>>> Since the values in vmcoreinfo are already relocated, the patch is
>>>>>> mainly relevant for vmlinux processing (-x option).
>>>>>
>>>>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
>>>>> is supposed to return the KASLR offset only when the offset is needed to
>>>>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
>>>>> symbols from modules are resolved dynamically and don't need the offset.
>>>> \>
>>>>> This patch always returns the offset if any, as a result, I guess this patch
>>>>> will not work as expected with module symbols in filter config file.
>>>>>
>>>>> So... How about making get_kaslr_offset_arm64() general for other archs
>>>>> (get_kaslr_offset_general() or something), then using it also for s390?
>>>>> If OK, I can do that generalization.
>>>>>
>>>>> Thanks,
>>>>> Kazu
>>>>>
>>>>>>
>>>>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
>>>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
>>>>>> ---
>>>>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>>>>>>  makedumpfile.h |  3 ++-
>>>>>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/s390x.c b/arch/s390x.c
>>>>>> index bf9d58e..892df14 100644
>>>>>> --- a/arch/s390x.c
>>>>>> +++ b/arch/s390x.c
>>>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>>>>>  	return TRUE;
>>>>>>  }
>>>>>>
>>>>>> +unsigned long
>>>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
>>>>>> +{
>>>>>> +	unsigned int i;
>>>>>> +	char buf[BUFSIZE_FGETS], *endp;
>>>>>> +
>>>>>> +	if (!info->file_vmcoreinfo)
>>>>>> +		return FALSE;
>>>>>> +
>>>>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>>>>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>>>>>> +		       info->name_vmcoreinfo, strerror(errno));
>>>>>> +		return FALSE;
>>>>>> +	}
>>>>>> +
>>>>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>>>>>> +		i = strlen(buf);
>>>>>> +		if (!i)
>>>>>> +			break;
>>>>>> +		if (buf[i - 1] == '\n')
>>>>>> +			buf[i - 1] = '\0';
>>>>>> +		if (strncmp(buf, STR_KERNELOFFSET,
>>>>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
>>>>>> +			info->kaslr_offset =
>>>>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
>>>>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return info->kaslr_offset;
>>>>>> +}
>>>>>> +
>>>>>>  static int
>>>>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
>>>>>>  {
>>>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>>>> index ac11e90..26f6247 100644
>>>>>> --- a/makedumpfile.h
>>>>>> +++ b/makedumpfile.h
>>>>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>>>>>>  int get_machdep_info_s390x(void);
>>>>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>>>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
>>>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>>>>>  #define find_vmemmap()		stub_false()
>>>>>>  #define get_phys_base()		stub_true()
>>>>>>  #define get_machdep_info()	get_machdep_info_s390x()
>>>>>>  #define get_versiondep_info()	stub_true()
>>>>>> -#define get_kaslr_offset(X)	stub_false()
>>>>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>>>>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>>>>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>>>>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-26  3:38           ` lijiang
@ 2019-12-26  4:07             ` lijiang
  2019-12-27 21:29               ` HAGIO KAZUHITO(萩尾 一仁)
  0 siblings, 1 reply; 10+ messages in thread
From: lijiang @ 2019-12-26  4:07 UTC (permalink / raw)
  To: kexec, k-hagio, Mikhail Zaslonko

在 2019年12月26日 11:38, lijiang 写道:
> Hi, Kazu and Mikhail,
> 
>> Hi Mikhail,
>>
>>> -----Original Message-----
>>> Hi,
>>>
>>> On 12.12.2019 17:12, Kazuhito Hagio wrote:
>>>> Hi Mikhail,
>>>>
>>>>> -----Original Message-----
>>>>> Hello Kazu,
>>>>>
>>>>> I think we can try to generalize the kaslr offset extraction.
>>>>> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
>>>>> should work fine. The only concern of mine is this TODO statement:
>>>>>
>>>>> if (_text <= vaddr && vaddr <= _end) {
>>>>> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>> 	return info->kaslr_offset;
>>>>> 	} else {
>>>>> 	/*
>>>>> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
>>>>> 	* address, we will have different offset
>>>>> 	*/
>>>>> 	return 0;
>>>>> }
>>>>>
>>>>> Could you explain this one?
>>>>
>>>> Probably it was considered that the check would be needed to support
>>>> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
>>>> originally.
>>>>
>>>> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
>>>> the offset we need is the one for symbol addresses in vmlinux only.
>>>> As I said below, module symbol addresses are retrieved from vmcore.
>>>> Other addresses should not be passed to the function for now, as far
>>>> as I know.
>>>>
>>>> So I think the TODO comment is confusing, and it would be better to
>>>> remove it or change it to something like:
>>>> /*
>>>>  * Returns 0 if vaddr does not need the offset to be added,
>>>>  * e.g. for module address.
>>>>  */
>>>>
>>>> But if s390 uses get_kaslr_offset() in its arch-specific code to
>>>> adjust addresses other than kernel text address, we might need to
>>>> modify it for s390, not generalize it.
>>>
>>> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
>>> code.
>>
>> OK, I pushed a patch that generalizes it to my test repository.
>> Could you enable s390 to use it and test?
>> https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general
>>
> 
> I enabled it on s390 as below and tested, it worked.
> 
> @@ -1075,7 +1075,7 @@ int is_iomem_phys_addr_s390x(unsigned long addr);
>  #define get_phys_base()                stub_true()
>  #define get_machdep_info()     get_machdep_info_s390x()
>  #define get_versiondep_info()  stub_true()
> -#define get_kaslr_offset(X)    stub_false()
> +#define get_kaslr_offset(X)    get_kaslr_offset_general(X)
>  #define vaddr_to_paddr(X)      vaddr_to_paddr_s390x(X)
> 
> But, there is still a problem that needs to be improved. In the find_kaslr_offsets(),
> the value of SYMBOL(_stext) is always 0(zero) and it is passed to the get_kaslr_offset().
> For the following code in the get_kaslr_offset_general(), it does not work as expected.
> ...
> 	if (_text <= vaddr && vaddr <= _end)
> 		return info->kaslr_offset;
> 	else
> 		return 0;

In addition, the above code confused me, it will always return 0 on s390(please refer to my logs).

Thanks.

> ...
> Here is my log:
> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:0
> 
> After applied the following patch, got the expected result.
>  int
>  find_kaslr_offsets()
>  {
> @@ -3973,6 +4042,11 @@ find_kaslr_offsets()
>          * called this function between open_vmcoreinfo() and
>          * close_vmcoreinfo()
>          */
> +       READ_SYMBOL("_stext", _stext);
> +       if (SYMBOL(_stext) == NOT_FOUND_SYMBOL) {
> +                ERRMSG("Can't get the symbol of _stext.\n");
> +                goto out;
> +       }
>         get_kaslr_offset(SYMBOL(_stext));
> 
> Here is my log:
> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:67fbc000
> 
> Basically, before using the value of SYMBOL(_stext), need to ensure that the SYMBOL(_stext) is parsed
> correctly.
> 
> Thanks.
> 
>> Thanks,
>> Kazu
>>
>>>
>>>>
>>>> Thanks,
>>>> Kazu
>>>>
>>>>>
>>>>> Thanks,
>>>>> Mikhail
>>>>>
>>>>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
>>>>>> Hi Mikhail,
>>>>>>
>>>>>> Sorry for late reply.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
>>>>>>> support has been added yet. This patch adds the arch specific function
>>>>>>> get_kaslr_offset() for s390x.
>>>>>>> Since the values in vmcoreinfo are already relocated, the patch is
>>>>>>> mainly relevant for vmlinux processing (-x option).
>>>>>>
>>>>>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
>>>>>> is supposed to return the KASLR offset only when the offset is needed to
>>>>>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
>>>>>> symbols from modules are resolved dynamically and don't need the offset.
>>>>> \>
>>>>>> This patch always returns the offset if any, as a result, I guess this patch
>>>>>> will not work as expected with module symbols in filter config file.
>>>>>>
>>>>>> So... How about making get_kaslr_offset_arm64() general for other archs
>>>>>> (get_kaslr_offset_general() or something), then using it also for s390?
>>>>>> If OK, I can do that generalization.
>>>>>>
>>>>>> Thanks,
>>>>>> Kazu
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
>>>>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
>>>>>>> ---
>>>>>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>>>>>>>  makedumpfile.h |  3 ++-
>>>>>>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/s390x.c b/arch/s390x.c
>>>>>>> index bf9d58e..892df14 100644
>>>>>>> --- a/arch/s390x.c
>>>>>>> +++ b/arch/s390x.c
>>>>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>>>>>>  	return TRUE;
>>>>>>>  }
>>>>>>>
>>>>>>> +unsigned long
>>>>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
>>>>>>> +{
>>>>>>> +	unsigned int i;
>>>>>>> +	char buf[BUFSIZE_FGETS], *endp;
>>>>>>> +
>>>>>>> +	if (!info->file_vmcoreinfo)
>>>>>>> +		return FALSE;
>>>>>>> +
>>>>>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>>>>>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>>>>>>> +		       info->name_vmcoreinfo, strerror(errno));
>>>>>>> +		return FALSE;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>>>>>>> +		i = strlen(buf);
>>>>>>> +		if (!i)
>>>>>>> +			break;
>>>>>>> +		if (buf[i - 1] == '\n')
>>>>>>> +			buf[i - 1] = '\0';
>>>>>>> +		if (strncmp(buf, STR_KERNELOFFSET,
>>>>>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
>>>>>>> +			info->kaslr_offset =
>>>>>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
>>>>>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return info->kaslr_offset;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static int
>>>>>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
>>>>>>>  {
>>>>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>>>>> index ac11e90..26f6247 100644
>>>>>>> --- a/makedumpfile.h
>>>>>>> +++ b/makedumpfile.h
>>>>>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>>>>>>>  int get_machdep_info_s390x(void);
>>>>>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>>>>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
>>>>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>>>>>>  #define find_vmemmap()		stub_false()
>>>>>>>  #define get_phys_base()		stub_true()
>>>>>>>  #define get_machdep_info()	get_machdep_info_s390x()
>>>>>>>  #define get_versiondep_info()	stub_true()
>>>>>>> -#define get_kaslr_offset(X)	stub_false()
>>>>>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>>>>>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>>>>>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>>>>>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
>>>>>>> --
>>>>>>> 2.17.1
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
>>
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-26  4:07             ` lijiang
@ 2019-12-27 21:29               ` HAGIO KAZUHITO(萩尾 一仁)
  2019-12-30 10:15                 ` lijiang
  0 siblings, 1 reply; 10+ messages in thread
From: HAGIO KAZUHITO(萩尾 一仁) @ 2019-12-27 21:29 UTC (permalink / raw)
  To: lijiang
  Cc: Mikhail Zaslonko, kexec,
	HAGIO KAZUHITO(萩尾 一仁)

Hi Lianbo,

> -----Original Message-----
> 在 2019年12月26日 11:38, lijiang 写道:
> > Hi, Kazu and Mikhail,
> >
> >> Hi Mikhail,
> >>
> >>> -----Original Message-----
> >>> Hi,
> >>>
> >>> On 12.12.2019 17:12, Kazuhito Hagio wrote:
> >>>> Hi Mikhail,
> >>>>
> >>>>> -----Original Message-----
> >>>>> Hello Kazu,
> >>>>>
> >>>>> I think we can try to generalize the kaslr offset extraction.
> >>>>> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
> >>>>> should work fine. The only concern of mine is this TODO statement:
> >>>>>
> >>>>> if (_text <= vaddr && vaddr <= _end) {
> >>>>> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> >>>>> 	return info->kaslr_offset;
> >>>>> 	} else {
> >>>>> 	/*
> >>>>> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
> >>>>> 	* address, we will have different offset
> >>>>> 	*/
> >>>>> 	return 0;
> >>>>> }
> >>>>>
> >>>>> Could you explain this one?
> >>>>
> >>>> Probably it was considered that the check would be needed to support
> >>>> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
> >>>> originally.
> >>>>
> >>>> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
> >>>> the offset we need is the one for symbol addresses in vmlinux only.
> >>>> As I said below, module symbol addresses are retrieved from vmcore.
> >>>> Other addresses should not be passed to the function for now, as far
> >>>> as I know.
> >>>>
> >>>> So I think the TODO comment is confusing, and it would be better to
> >>>> remove it or change it to something like:
> >>>> /*
> >>>>  * Returns 0 if vaddr does not need the offset to be added,
> >>>>  * e.g. for module address.
> >>>>  */
> >>>>
> >>>> But if s390 uses get_kaslr_offset() in its arch-specific code to
> >>>> adjust addresses other than kernel text address, we might need to
> >>>> modify it for s390, not generalize it.
> >>>
> >>> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
> >>> code.
> >>
> >> OK, I pushed a patch that generalizes it to my test repository.
> >> Could you enable s390 to use it and test?
> >> https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general
> >>
> >
> > I enabled it on s390 as below and tested, it worked.

Thank you for testing.

> >
> > @@ -1075,7 +1075,7 @@ int is_iomem_phys_addr_s390x(unsigned long addr);
> >  #define get_phys_base()                stub_true()
> >  #define get_machdep_info()     get_machdep_info_s390x()
> >  #define get_versiondep_info()  stub_true()
> > -#define get_kaslr_offset(X)    stub_false()
> > +#define get_kaslr_offset(X)    get_kaslr_offset_general(X)
> >  #define vaddr_to_paddr(X)      vaddr_to_paddr_s390x(X)
> >
> > But, there is still a problem that needs to be improved. In the find_kaslr_offsets(),
> > the value of SYMBOL(_stext) is always 0(zero) and it is passed to the get_kaslr_offset().
> > For the following code in the get_kaslr_offset_general(), it does not work as expected.
> > ...
> > 	if (_text <= vaddr && vaddr <= _end)
> > 		return info->kaslr_offset;
> > 	else
> > 		return 0;

I don't know why the SYMBOL(_stext) is passed to the get_kaslr_offset() there, but
since the return value of get_kaslr_offset() is not used in the find_kaslr_offsets(),
it's meaningless and not harmful. So it is not worth doing READ_SYMBOL(_stext) there
for now.

> 
> In addition, the above code confused me, it will always return 0 on s390(please refer to my logs).

The aim of get_kaslr_offset() here is only setting info->kaslr_offset to the value
from vmcoreinfo for the SYMBOL_INIT() macro.
(get_kaslr_offset() returns the kaslr offset in the resolve_config_entry().)

But yeah, the get_kaslr_offset(SYMBOL(_stext)) is confusing and not good.
Passing 0 might be a bit better..?

Thanks,
Kazu

> 
> Thanks.
> 
> > ...
> > Here is my log:
> > get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:0
> >
> > After applied the following patch, got the expected result.
> >  int
> >  find_kaslr_offsets()
> >  {
> > @@ -3973,6 +4042,11 @@ find_kaslr_offsets()
> >          * called this function between open_vmcoreinfo() and
> >          * close_vmcoreinfo()
> >          */
> > +       READ_SYMBOL("_stext", _stext);
> > +       if (SYMBOL(_stext) == NOT_FOUND_SYMBOL) {
> > +                ERRMSG("Can't get the symbol of _stext.\n");
> > +                goto out;
> > +       }
> >         get_kaslr_offset(SYMBOL(_stext));
> >
> > Here is my log:
> > get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:67fbc000
> >
> > Basically, before using the value of SYMBOL(_stext), need to ensure that the SYMBOL(_stext) is parsed
> > correctly.
> >
> > Thanks.
> >
> >> Thanks,
> >> Kazu
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>> Kazu
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Mikhail
> >>>>>
> >>>>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
> >>>>>> Hi Mikhail,
> >>>>>>
> >>>>>> Sorry for late reply.
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
> >>>>>>> support has been added yet. This patch adds the arch specific function
> >>>>>>> get_kaslr_offset() for s390x.
> >>>>>>> Since the values in vmcoreinfo are already relocated, the patch is
> >>>>>>> mainly relevant for vmlinux processing (-x option).
> >>>>>>
> >>>>>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
> >>>>>> is supposed to return the KASLR offset only when the offset is needed to
> >>>>>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
> >>>>>> symbols from modules are resolved dynamically and don't need the offset.
> >>>>> \>
> >>>>>> This patch always returns the offset if any, as a result, I guess this patch
> >>>>>> will not work as expected with module symbols in filter config file.
> >>>>>>
> >>>>>> So... How about making get_kaslr_offset_arm64() general for other archs
> >>>>>> (get_kaslr_offset_general() or something), then using it also for s390?
> >>>>>> If OK, I can do that generalization.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Kazu
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
> >>>>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> >>>>>>> ---
> >>>>>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
> >>>>>>>  makedumpfile.h |  3 ++-
> >>>>>>>  2 files changed, 34 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/s390x.c b/arch/s390x.c
> >>>>>>> index bf9d58e..892df14 100644
> >>>>>>> --- a/arch/s390x.c
> >>>>>>> +++ b/arch/s390x.c
> >>>>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
> >>>>>>>  	return TRUE;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> +unsigned long
> >>>>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
> >>>>>>> +{
> >>>>>>> +	unsigned int i;
> >>>>>>> +	char buf[BUFSIZE_FGETS], *endp;
> >>>>>>> +
> >>>>>>> +	if (!info->file_vmcoreinfo)
> >>>>>>> +		return FALSE;
> >>>>>>> +
> >>>>>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
> >>>>>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
> >>>>>>> +		       info->name_vmcoreinfo, strerror(errno));
> >>>>>>> +		return FALSE;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
> >>>>>>> +		i = strlen(buf);
> >>>>>>> +		if (!i)
> >>>>>>> +			break;
> >>>>>>> +		if (buf[i - 1] == '\n')
> >>>>>>> +			buf[i - 1] = '\0';
> >>>>>>> +		if (strncmp(buf, STR_KERNELOFFSET,
> >>>>>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
> >>>>>>> +			info->kaslr_offset =
> >>>>>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
> >>>>>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
> >>>>>>> +		}
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	return info->kaslr_offset;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  static int
> >>>>>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
> >>>>>>>  {
> >>>>>>> diff --git a/makedumpfile.h b/makedumpfile.h
> >>>>>>> index ac11e90..26f6247 100644
> >>>>>>> --- a/makedumpfile.h
> >>>>>>> +++ b/makedumpfile.h
> >>>>>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
> >>>>>>>  int get_machdep_info_s390x(void);
> >>>>>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
> >>>>>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
> >>>>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
> >>>>>>>  #define find_vmemmap()		stub_false()
> >>>>>>>  #define get_phys_base()		stub_true()
> >>>>>>>  #define get_machdep_info()	get_machdep_info_s390x()
> >>>>>>>  #define get_versiondep_info()	stub_true()
> >>>>>>> -#define get_kaslr_offset(X)	stub_false()
> >>>>>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
> >>>>>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
> >>>>>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
> >>>>>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
> >>>>>>> --
> >>>>>>> 2.17.1
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> >>
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> >
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x
  2019-12-27 21:29               ` HAGIO KAZUHITO(萩尾 一仁)
@ 2019-12-30 10:15                 ` lijiang
  0 siblings, 0 replies; 10+ messages in thread
From: lijiang @ 2019-12-30 10:15 UTC (permalink / raw)
  To: HAGIO KAZUHITO(萩尾 一仁)
  Cc: Mikhail Zaslonko, kexec

> Hi Lianbo,
> 
>> -----Original Message-----
>> 在 2019年12月26日 11:38, lijiang 写道:
>>> Hi, Kazu and Mikhail,
>>>
>>>> Hi Mikhail,
>>>>
>>>>> -----Original Message-----
>>>>> Hi,
>>>>>
>>>>> On 12.12.2019 17:12, Kazuhito Hagio wrote:
>>>>>> Hi Mikhail,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> Hello Kazu,
>>>>>>>
>>>>>>> I think we can try to generalize the kaslr offset extraction.
>>>>>>> I won't speak for other architectures, but for s390 that get_kaslr_offset_arm64()
>>>>>>> should work fine. The only concern of mine is this TODO statement:
>>>>>>>
>>>>>>> if (_text <= vaddr && vaddr <= _end) {
>>>>>>> 	DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>>>> 	return info->kaslr_offset;
>>>>>>> 	} else {
>>>>>>> 	/*
>>>>>>> 	* TODO: we need to check if it is vmalloc/vmmemmap/module
>>>>>>> 	* address, we will have different offset
>>>>>>> 	*/
>>>>>>> 	return 0;
>>>>>>> }
>>>>>>>
>>>>>>> Could you explain this one?
>>>>>>
>>>>>> Probably it was considered that the check would be needed to support
>>>>>> the whole KASLR behavior when get_kaslr_offset_x86_64() was written
>>>>>> originally.
>>>>>>
>>>>>> But in the current makedumpfile for x86_64 and arm64 supporting KASLR,
>>>>>> the offset we need is the one for symbol addresses in vmlinux only.
>>>>>> As I said below, module symbol addresses are retrieved from vmcore.
>>>>>> Other addresses should not be passed to the function for now, as far
>>>>>> as I know.
>>>>>>
>>>>>> So I think the TODO comment is confusing, and it would be better to
>>>>>> remove it or change it to something like:
>>>>>> /*
>>>>>>  * Returns 0 if vaddr does not need the offset to be added,
>>>>>>  * e.g. for module address.
>>>>>>  */
>>>>>>
>>>>>> But if s390 uses get_kaslr_offset() in its arch-specific code to
>>>>>> adjust addresses other than kernel text address, we might need to
>>>>>> modify it for s390, not generalize it.
>>>>>
>>>>> Currently, s390 doesn't use get_kaslr_offset() in its arch-specific
>>>>> code.
>>>>
>>>> OK, I pushed a patch that generalizes it to my test repository.
>>>> Could you enable s390 to use it and test?
>>>> https://github.com/k-hagio/makedumpfile/tree/add-get_kaslr_offset_general
>>>>
>>>
>>> I enabled it on s390 as below and tested, it worked.
> 
> Thank you for testing.
> 
It's my pleasure. 

>>>
>>> @@ -1075,7 +1075,7 @@ int is_iomem_phys_addr_s390x(unsigned long addr);
>>>  #define get_phys_base()                stub_true()
>>>  #define get_machdep_info()     get_machdep_info_s390x()
>>>  #define get_versiondep_info()  stub_true()
>>> -#define get_kaslr_offset(X)    stub_false()
>>> +#define get_kaslr_offset(X)    get_kaslr_offset_general(X)
>>>  #define vaddr_to_paddr(X)      vaddr_to_paddr_s390x(X)
>>>
>>> But, there is still a problem that needs to be improved. In the find_kaslr_offsets(),
>>> the value of SYMBOL(_stext) is always 0(zero) and it is passed to the get_kaslr_offset().
>>> For the following code in the get_kaslr_offset_general(), it does not work as expected.
>>> ...
>>> 	if (_text <= vaddr && vaddr <= _end)
>>> 		return info->kaslr_offset;
>>> 	else
>>> 		return 0;
> 
> I don't know why the SYMBOL(_stext) is passed to the get_kaslr_offset() there, but
> since the return value of get_kaslr_offset() is not used in the find_kaslr_offsets(),
> it's meaningless and not harmful. So it is not worth doing READ_SYMBOL(_stext) there
> for now.
> 
Sounds good.

>>
>> In addition, the above code confused me, it will always return 0 on s390(please refer to my logs).
> 
> The aim of get_kaslr_offset() here is only setting info->kaslr_offset to the value
> from vmcoreinfo for the SYMBOL_INIT() macro.
> (get_kaslr_offset() returns the kaslr offset in the resolve_config_entry().)
> 
Thanks for your explanation, Kazu.

> But yeah, the get_kaslr_offset(SYMBOL(_stext)) is confusing and not good.
> Passing 0 might be a bit better..?
> 
Yes, looks good to me.

Lianbo
> Thanks,
> Kazu
> 
>>
>> Thanks.
>>
>>> ...
>>> Here is my log:
>>> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:0
>>>
>>> After applied the following patch, got the expected result.
>>>  int
>>>  find_kaslr_offsets()
>>>  {
>>> @@ -3973,6 +4042,11 @@ find_kaslr_offsets()
>>>          * called this function between open_vmcoreinfo() and
>>>          * close_vmcoreinfo()
>>>          */
>>> +       READ_SYMBOL("_stext", _stext);
>>> +       if (SYMBOL(_stext) == NOT_FOUND_SYMBOL) {
>>> +                ERRMSG("Can't get the symbol of _stext.\n");
>>> +                goto out;
>>> +       }
>>>         get_kaslr_offset(SYMBOL(_stext));
>>>
>>> Here is my log:
>>> get_kaslr_offset_general: info->kaslr_offset: 67ebc000, _text:100000, _end:10ba000, vaddr:67fbc000
>>>
>>> Basically, before using the value of SYMBOL(_stext), need to ensure that the SYMBOL(_stext) is parsed
>>> correctly.
>>>
>>> Thanks.
>>>
>>>> Thanks,
>>>> Kazu
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Kazu
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Mikhail
>>>>>>>
>>>>>>> On 09.12.2019 23:02, Kazuhito Hagio wrote:
>>>>>>>> Hi Mikhail,
>>>>>>>>
>>>>>>>> Sorry for late reply.
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> Since kernel v5.2 KASLR is supported on s390. In makedumpfile however no
>>>>>>>>> support has been added yet. This patch adds the arch specific function
>>>>>>>>> get_kaslr_offset() for s390x.
>>>>>>>>> Since the values in vmcoreinfo are already relocated, the patch is
>>>>>>>>> mainly relevant for vmlinux processing (-x option).
>>>>>>>>
>>>>>>>> In the current implementation of makedumpfile, the get_kaslr_offset(vaddr)
>>>>>>>> is supposed to return the KASLR offset only when the offset is needed to
>>>>>>>> add to the vaddr.  So generally symbols from kernel (vmlinux) need it, but
>>>>>>>> symbols from modules are resolved dynamically and don't need the offset.
>>>>>>> \>
>>>>>>>> This patch always returns the offset if any, as a result, I guess this patch
>>>>>>>> will not work as expected with module symbols in filter config file.
>>>>>>>>
>>>>>>>> So... How about making get_kaslr_offset_arm64() general for other archs
>>>>>>>> (get_kaslr_offset_general() or something), then using it also for s390?
>>>>>>>> If OK, I can do that generalization.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Kazu
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Philipp Rudo <prudo@linux.ibm.com>
>>>>>>>>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
>>>>>>>>> ---
>>>>>>>>>  arch/s390x.c   | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>>  makedumpfile.h |  3 ++-
>>>>>>>>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/s390x.c b/arch/s390x.c
>>>>>>>>> index bf9d58e..892df14 100644
>>>>>>>>> --- a/arch/s390x.c
>>>>>>>>> +++ b/arch/s390x.c
>>>>>>>>> @@ -122,6 +122,38 @@ get_machdep_info_s390x(void)
>>>>>>>>>  	return TRUE;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +unsigned long
>>>>>>>>> +get_kaslr_offset_s390x(unsigned long vaddr)
>>>>>>>>> +{
>>>>>>>>> +	unsigned int i;
>>>>>>>>> +	char buf[BUFSIZE_FGETS], *endp;
>>>>>>>>> +
>>>>>>>>> +	if (!info->file_vmcoreinfo)
>>>>>>>>> +		return FALSE;
>>>>>>>>> +
>>>>>>>>> +	if (fseek(info->file_vmcoreinfo, 0, SEEK_SET) < 0) {
>>>>>>>>> +		ERRMSG("Can't seek the vmcoreinfo file(%s). %s\n",
>>>>>>>>> +		       info->name_vmcoreinfo, strerror(errno));
>>>>>>>>> +		return FALSE;
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>> +	while (fgets(buf, BUFSIZE_FGETS, info->file_vmcoreinfo)) {
>>>>>>>>> +		i = strlen(buf);
>>>>>>>>> +		if (!i)
>>>>>>>>> +			break;
>>>>>>>>> +		if (buf[i - 1] == '\n')
>>>>>>>>> +			buf[i - 1] = '\0';
>>>>>>>>> +		if (strncmp(buf, STR_KERNELOFFSET,
>>>>>>>>> +			    strlen(STR_KERNELOFFSET)) == 0) {
>>>>>>>>> +			info->kaslr_offset =
>>>>>>>>> +				strtoul(buf + strlen(STR_KERNELOFFSET), &endp, 16);
>>>>>>>>> +			DEBUG_MSG("info->kaslr_offset: %lx\n", info->kaslr_offset);
>>>>>>>>> +		}
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>> +	return info->kaslr_offset;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  static int
>>>>>>>>>  is_vmalloc_addr_s390x(unsigned long vaddr)
>>>>>>>>>  {
>>>>>>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>>>>>>> index ac11e90..26f6247 100644
>>>>>>>>> --- a/makedumpfile.h
>>>>>>>>> +++ b/makedumpfile.h
>>>>>>>>> @@ -1071,11 +1071,12 @@ unsigned long long vaddr_to_paddr_ppc(unsigned long vaddr);
>>>>>>>>>  int get_machdep_info_s390x(void);
>>>>>>>>>  unsigned long long vaddr_to_paddr_s390x(unsigned long vaddr);
>>>>>>>>>  int is_iomem_phys_addr_s390x(unsigned long addr);
>>>>>>>>> +unsigned long get_kaslr_offset_s390x(unsigned long vaddr);
>>>>>>>>>  #define find_vmemmap()		stub_false()
>>>>>>>>>  #define get_phys_base()		stub_true()
>>>>>>>>>  #define get_machdep_info()	get_machdep_info_s390x()
>>>>>>>>>  #define get_versiondep_info()	stub_true()
>>>>>>>>> -#define get_kaslr_offset(X)	stub_false()
>>>>>>>>> +#define get_kaslr_offset(X)	get_kaslr_offset_s390x(X)
>>>>>>>>>  #define vaddr_to_paddr(X)	vaddr_to_paddr_s390x(X)
>>>>>>>>>  #define paddr_to_vaddr(X)	paddr_to_vaddr_general(X)
>>>>>>>>>  #define is_phys_addr(X)		is_iomem_phys_addr_s390x(X)
>>>>>>>>> --
>>>>>>>>> 2.17.1
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>>
>>>
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>
>>
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-12-30 10:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 22:27 [PATCH] makedumpfile/s390: Add get_kaslr_offset() for s390x Mikhail Zaslonko
2019-12-09 22:02 ` Kazuhito Hagio
2019-12-12  9:50   ` Zaslonko Mikhail
2019-12-12 16:12     ` Kazuhito Hagio
2019-12-12 16:31       ` Zaslonko Mikhail
2019-12-17 17:46         ` Kazuhito Hagio
2019-12-26  3:38           ` lijiang
2019-12-26  4:07             ` lijiang
2019-12-27 21:29               ` HAGIO KAZUHITO(萩尾 一仁)
2019-12-30 10:15                 ` lijiang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.