recordmcount: bugfix and amend

PATCH In review Li Bin 3 participants 10 messages Oct 30, 2015 – Nov 3, 2015

Share & tools

Li Bin

author

2015-10-30 08:31 UTC #0 lore raw

cover recordmcount: bugfix and amend

Li Bin (3):
recordmcount: fix endianness handling bug for nop_mcount
recordmcount: x86: assign a meaningful value to rel_type_nop
recordmcount: arm64: replace the ignored mcount call into nop

 scripts/recordmcount.c | 26 +++++++++++++++++++++++++-
 scripts/recordmcount.h |  5 +++--
 2 files changed, 28 insertions(+), 3 deletions(-)

Signature · 1 line

1.7.12.4

Li Bin

author

2015-10-30 08:31 UTC #1 lore raw

PATCH 1/3 recordmcount: fix endianness handling bug for nop_mcount

In nop_mcount, shdr->sh_offset and welp->r_offset should handle endianness properly, otherwise it will trigger Segmentation fault if the recordmcount main and file.o have different endianness.

Cc <[email protected]> # 3.0+

Signed-off-by Li Bin <[email protected]>

---
 scripts/recordmcount.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

scripts/recordmcount.h +3 -2

index 49b582a..dda9dba 100644
@@ -376,8 +376,9 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
 			mcountsym = get_mcountsym(sym0, relp, str0);
 
 		if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
-			if (make_nop)
-				ret = make_nop((void *)ehdr, shdr->sh_offset + relp->r_offset);
+			if (make_nop) {
+				ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
+			}
 			if (warn_on_notrace_sect && !once) {
 				printf("Section %s has mcount callers being ignored\n",
 				       txtname);

Signature · 1 line

1.7.12.4

libin

author

2015-10-30 08:45 UTC #2 lore raw

Re: [PATCH 1/3] recordmcount: fix endianness handling bug for nop_mcount

Quoted 4 lines · Li Bin

In nop_mcount, shdr->sh_offset and welp->r_offset should handle endianness properly, otherwise it will trigger Segmentation fault if the recordmcount main and file.o have different endianness.

For more information, please refer to

	https://lkml.org/lkml/2015/10/29/258

Quoted 23 lines

Cc <[email protected]> # 3.0+

Signed-off-by Li Bin <[email protected]>

---
 scripts/recordmcount.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

scripts/recordmcount.h +3 -2

index 49b582a..dda9dba 100644
@@ -376,8 +376,9 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
 			mcountsym = get_mcountsym(sym0, relp, str0);
 
 		if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
-			if (make_nop)
-				ret = make_nop((void *)ehdr, shdr->sh_offset + relp->r_offset);
+			if (make_nop) {
+				ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
+			}
 			if (warn_on_notrace_sect && !once) {
 				printf("Section %s has mcount callers being ignored\n",
 				       txtname);

libin

author

2015-10-31 02:07 UTC #4 lore raw

Re: [PATCH 1/3] recordmcount: fix endianness handling bug for nop_mcount

Quoted 21 lines · Sergei Shtylyov

Hello.
Quoted 17 lines · Li Bin
In nop_mcount, shdr->sh_offset and welp->r_offset should handle endianness properly, otherwise it will trigger Segmentation fault if the recordmcount main and file.o have different endianness.
Cc <[email protected]> # 3.0+
Signed-off-by Li Bin <[email protected]>
---
  scripts/recordmcount.h | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
scripts/recordmcount.h +0 -0
index 49b582a..dda9dba 100644
@@ -376,8 +376,9 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
              mcountsym = get_mcountsym(sym0, relp, str0);
 

Quoted 8 lines

scripts/recordmcount.h static void nop_mcount(Elf_Shdr const *const relhdr, · line 381

Quoted 6 lines

scripts/recordmcount.h +3 -2

          if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
-            if (make_nop)
-                ret = make_nop((void *)ehdr, shdr->sh_offset + relp->r_offset);
+            if (make_nop) {
+                ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
+            }

{} not needed here.

Oh, yes. When I cleaned the debugging code, forgot delete the {}.

Thanks,
Li Bin

Quoted 6 lines

MBR, Sergei
.

Steven Rostedt

2015-11-02 19:33 UTC #5 lore raw

Re: [PATCH 1/3] recordmcount: fix endianness handling bug for nop_mcount

Quoted 33 lines · On Sat, 31 Oct 2015 10:07:38 +0800 libin <[email protected]> wrote:

Quoted 21 lines · Sergei Shtylyov
Hello.
Quoted 17 lines · Li Bin
In nop_mcount, shdr->sh_offset and welp->r_offset should handle endianness properly, otherwise it will trigger Segmentation fault if the recordmcount main and file.o have different endianness.
Cc <[email protected]> # 3.0+
Signed-off-by Li Bin <[email protected]>
---
  scripts/recordmcount.h | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
scripts/recordmcount.h +0 -0
index 49b582a..dda9dba 100644
@@ -376,8 +376,9 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
              mcountsym = get_mcountsym(sym0, relp, str0);
 
Quoted 8 lines
scripts/recordmcount.h static void nop_mcount(Elf_Shdr const *const relhdr, · line 381
Quoted 6 lines
scripts/recordmcount.h +3 -2
          if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
-            if (make_nop)
-                ret = make_nop((void *)ehdr, shdr->sh_offset + relp->r_offset);
+            if (make_nop) {
+                ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
+            }
{} not needed here.

Quoted 3 lines

Oh, yes. When I cleaned the debugging code, forgot delete the {}.

Is there a new version of this patch, or should I just take this and remove the '{}' myself?

I already added the x86 update.

-- Steve

libin

author

2015-11-03 00:58 UTC #6 lore raw

PATCH 1/1 recordmcount: fix endianness handling bug for nop_mcount

In nop_mcount, shdr->sh_offset and welp->r_offset should handle endianness properly, otherwise it will trigger Segmentation fault if the recordmcount main and file.o have different endianness.

Cc <[email protected]> # 3.0+

Signed-off-by Li Bin <[email protected]>

---
 scripts/recordmcount.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

scripts/recordmcount.h +1 -1

index 49b582a..b9897e2 100644
@@ -377,7 +377,7 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
 
 		if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
 			if (make_nop)
-				ret = make_nop((void *)ehdr, shdr->sh_offset + relp->r_offset);
+				ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
 			if (warn_on_notrace_sect && !once) {
 				printf("Section %s has mcount callers being ignored\n",
 				       txtname);

Signature · 1 line

1.7.1

Li Bin

author

2015-10-30 08:31 UTC #7 lore raw

PATCH 2/3 recordmcount: x86: assign a meaningful value to rel_type_nop

In reply to Li Bin

Although, the default value of rel_type_nop is zero, and the value of R_386_NONE/R_X86_64_NONE is zero too, but it should be assigned a meaningful value explicitly, otherwise it looks confused.

Assign R_386_NONE to rel_type_nop for 386, assign R_X86_64_NONE to rel_type_nop for x86_64.

Signed-off-by Li Bin <[email protected]>

---
 scripts/recordmcount.c | 2 ++
 1 file changed, 2 insertions(+)

scripts/recordmcount.c +2 -0

index 3d1984e..8cc020b 100644
@@ -345,6 +345,7 @@ do_file(char const *const fname)
 		break;
 	case EM_386:
 		reltype = R_386_32;
+		rel_type_nop = R_386_NONE;
 		make_nop = make_nop_x86;
 		ideal_nop = ideal_nop5_x86_32;
 		mcount_adjust_32 = -1;

@@ -371,6 +372,7 @@ do_file(char const *const fname)
 		make_nop = make_nop_x86;
 		ideal_nop = ideal_nop5_x86_64;
 		reltype = R_X86_64_64;
+		rel_type_nop = R_X86_64_NONE;
 		mcount_adjust_64 = -1;
 		break;
 	}  /* end switch */

Signature · 1 line

1.7.12.4

Li Bin

author

2015-10-30 08:31 UTC #8 lore raw

PATCH 3/3 recordmcount: arm64: replace the ignored mcount call into nop

In reply to Li Bin

By now, the recordmcount only records the function that in following sections:
.text/.ref.text/.sched.text/.spinlock.text/.irqentry.text/ .kprobes.text/.text.unlikely

For the function that not in these sections, the call mcount will be in place and not be replaced when kernel boot up. And it will bring performance overhead, such as do_mem_abort (in .exception.text section). This patch make the call mcount to nop for this case in recordmcount.

Cc <[email protected]> # 3.18+

Signed-off-by Li Bin <[email protected]>

---
 scripts/recordmcount.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

scripts/recordmcount.c +23 -1

index 8cc020b..698768b 100644
@@ -42,6 +42,7 @@
 
 #ifndef EM_AARCH64
 #define EM_AARCH64	183
+#define R_AARCH64_NONE		0
 #define R_AARCH64_ABS64	257
 #endif

@@ -160,6 +161,22 @@ static int make_nop_x86(void *map, size_t const offset)
 	return 0;
 }
 
+static unsigned char ideal_nop4_arm64[4] = {0x1f, 0x20, 0x03, 0xd5};
+static int make_nop_arm64(void *map, size_t const offset)
+{
+	uint32_t *ptr;
+
+	ptr = map + offset;
+	/* bl <_mcount> is 0x94000000 before relocation */
+	if (*ptr != 0x94000000)
+		return -1;
+
+	/* Convert to nop */
+	ulseek(fd_map, offset, SEEK_SET);
+	uwrite(fd_map, ideal_nop, 4);
+	return 0;
+}
+
 /*
  * Get the whole file as a programming convenience in order to avoid
  * malloc+lseek+read+free of many pieces.  If successful, then mmap

@@ -354,7 +371,12 @@ do_file(char const *const fname)
 			 altmcount = "__gnu_mcount_nc";
 			 break;
 	case EM_AARCH64:
-			 reltype = R_AARCH64_ABS64; gpfx = '_'; break;
+			reltype = R_AARCH64_ABS64;
+			make_nop = make_nop_arm64;
+			rel_type_nop = R_AARCH64_NONE;
+			ideal_nop = ideal_nop4_arm64;
+			gpfx = '_';
+			break;
 	case EM_IA_64:	 reltype = R_IA64_IMM64;   gpfx = '_'; break;
 	case EM_METAG:	 reltype = R_METAG_ADDR32;
 			 altmcount = "_mcount_wrapper";

Signature · 1 line

1.7.12.4

libin

author Acked-by Will Deacon on v1 3/3

2015-10-30 08:44 UTC #9 lore raw

Re: [PATCH 3/3] recordmcount: arm64: replace the ignored mcount call into nop

Quoted 5 lines · Li Bin

By now, the recordmcount only records the function that in following sections:
.text/.ref.text/.sched.text/.spinlock.text/.irqentry.text/ .kprobes.text/.text.unlikely

Quoted 8 lines

For the function that not in these sections, the call mcount will be in place and not be replaced when kernel boot up. And it will bring performance overhead, such as do_mem_abort (in .exception.text section). This patch make the call mcount to nop for this case in recordmcount.
Cc <[email protected]> # 3.18+
Signed-off-by Li Bin <[email protected]>

Acked-by Will Deacon <[email protected]>

https://lkml.org/lkml/2015/10/28/301

Quoted 54 lines

---
 scripts/recordmcount.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

scripts/recordmcount.c +23 -1

index 8cc020b..698768b 100644
@@ -42,6 +42,7 @@
 
 #ifndef EM_AARCH64
 #define EM_AARCH64	183
+#define R_AARCH64_NONE		0
 #define R_AARCH64_ABS64	257
 #endif

@@ -160,6 +161,22 @@ static int make_nop_x86(void *map, size_t const offset)
 	return 0;
 }
 
+static unsigned char ideal_nop4_arm64[4] = {0x1f, 0x20, 0x03, 0xd5};
+static int make_nop_arm64(void *map, size_t const offset)
+{
+	uint32_t *ptr;
+
+	ptr = map + offset;
+	/* bl <_mcount> is 0x94000000 before relocation */
+	if (*ptr != 0x94000000)
+		return -1;
+
+	/* Convert to nop */
+	ulseek(fd_map, offset, SEEK_SET);
+	uwrite(fd_map, ideal_nop, 4);
+	return 0;
+}
+
 /*
  * Get the whole file as a programming convenience in order to avoid
  * malloc+lseek+read+free of many pieces.  If successful, then mmap

@@ -354,7 +371,12 @@ do_file(char const *const fname)
 			 altmcount = "__gnu_mcount_nc";
 			 break;
 	case EM_AARCH64:
-			 reltype = R_AARCH64_ABS64; gpfx = '_'; break;
+			reltype = R_AARCH64_ABS64;
+			make_nop = make_nop_arm64;
+			rel_type_nop = R_AARCH64_NONE;
+			ideal_nop = ideal_nop4_arm64;
+			gpfx = '_';
+			break;
 	case EM_IA_64:	 reltype = R_IA64_IMM64;   gpfx = '_'; break;
 	case EM_METAG:	 reltype = R_METAG_ADDR32;
 			 altmcount = "_mcount_wrapper";