scripts/sorttable: ftrace: Remove place holders for weak functions in available_filter_functions

This series removes the place holder __ftrace_invalid_address___ from the available_filter_functions file.

The rewriting of the sorttable.c code to make it more manageable has already been merged:

https://git.kernel.org/torvalds/c/c0e75905caf368e19aab585d20151500e750de89

Now this is only for getting rid of the ftrace invalid function place holders.

The first patch adds arm64 sorting, which requires copying the Elf_Rela into a separate array and sorting that.

There's a slight fix patch that adds using a compare function that checks the direct values without swapping bytes as the current method will swap bytes, but the copying into the array already did the necessary swapping.

The third patch makes it always copy the section into an array, sort that, then copy it back. This allows updates to the values in one place.

The forth patch adds the option "-s <file>" to sorttable.c. Now this code is called by:

  ${NM} -S vmlinux > .tmp_vmlinux.nm-sort
  ${objtree}/scripts/sorttable -s .tmp_vmlinux.nm-sort ${1}

Where the file created by "nm -S" is read, recording the address and the associated sizes of each function. It then is sorted, and before sorting the mcount_loc table, it is scanned to make sure all symbols in the mcount_loc are within the boundaries of the functions defined by nm. If they are not, they are zeroed out, as they are most likely weak functions (I don't know what else they would be).

Since the KASLR address can be added to the values in this section, when the section is read to populate the ftrace records, if the value is zero or equal to kaslr_offset() it is skipped and not added.

Before:

~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l 551

After:

~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l 0

The last patches are fixes to ftrace accounting to handle the fact that it will likely always have skipped values (at least for x86), and to modify the code to verify that the amount of skipped and saved records do match the calculated allocations necessary.

And finally, to change the reporting of how much was allocated to reflect the freed pages that were allocated but not used due to the skipped entries.

Changes since v3: https://lore.kernel.org/all/[email protected]/

- Do not remove 'W' weak functions that are still used.

Changes since v2: https://lore.kernel.org/linux-trace-kernel/[email protected]/

- Rebased on mainline that has the rewriting of sorttable.c

- Added the code to handle the sections being stored in Elf_Rela sections as arm64 uses.

- No longer use the "ftrace_skip_sym" variable to skip over the zeroed out functions and instead just compare with kalsr_offset.

- Sort via an array and not directly in the file's section.

- Update the verification code to make sure the skipped value is correct.

- Update the output to correctly reflect what was allocated.

Changes since v1: https://lore.kernel.org/all/[email protected]/

- Replaced the last patch with 3 patches.

The first of the 3 patches removed the hack of reading System.map with properly reading the Elf symbol table to find start_mcount_loc and stop_mcount_loc.

The second patch adds the call to "nm -S vmlinux" to get the sizes of each function.

The previous last patch would just check the zeroed out values and compare them to kaslr_offset(). Instead, this time, the last patch adds a new ftrace_mcount_skip that is used to simply skip over the first entries that the sorttable.c moved to the beginning, as they were the weak functions that were found.

Steven Rostedt (6):
arm64: scripts/sorttable: Implement sorting mcount_loc at boot for arm64 scripts/sorttable: Have mcount rela sort use direct values scripts/sorttable: Always use an array for the mcount_loc sorting scripts/sorttable: Zero out weak functions in mcount_loc table
ftrace: Update the mcount_loc check of skipped entries
ftrace: Have ftrace pages output reflect freed pages

----

 arch/arm64/Kconfig      |   1 +
 kernel/trace/ftrace.c   |  45 +++++-
 scripts/link-vmlinux.sh |   4 +-
 scripts/sorttable.c     | 401 +++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 437 insertions(+), 14 deletions(-)

From: Steven Rostedt <[email protected]>

The mcount_loc section holds the addresses of the functions that get patched by ftrace when enabling function callbacks. It can contain tens of thousands of entries. These addresses must be sorted. If they are not sorted at compile time, they are sorted at boot. Sorting at boot does take some time and does have a small impact on boot performance.

x86 and arm32 have the addresses in the mcount_loc section of the ELF file. But for arm64, the section just contains zeros. The .rela.dyn Elf_Rela section holds the addresses and they get patched at boot during the relocation phase.

In order to sort these addresses, the Elf_Rela needs to be updated instead of the location in the binary that holds the mcount_loc section. Have the sorttable code, allocate an array to hold the functions, load the addresses from the Elf_Rela entries, sort them, then put them back in order into the Elf_rela entries so that they will be sorted at boot up without having to sort them during boot up.

---
 arch/arm64/Kconfig  |   1 +
 scripts/sorttable.c | 185 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 183 insertions(+), 3 deletions(-)

index 940343beb3d4..4521ecefc031 100644
@@ -217,6 +217,7 @@ config ARM64
 		if DYNAMIC_FTRACE_WITH_ARGS
 	select HAVE_SAMPLE_FTRACE_DIRECT
 	select HAVE_SAMPLE_FTRACE_DIRECT_MULTI
+	select HAVE_BUILDTIME_MCOUNT_SORT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_GUP_FAST
 	select HAVE_FTRACE_GRAPH_FUNC

2.47.2

From: Steven Rostedt <[email protected]>

The mcount_loc sorting for when the values are stored in the Elf_Rela entries uses the compare_extable() function to do the compares in the qsort(). That function does handle byte swapping if the machine being compiled for is a different endian than the host machine. But the sort_relocs() function sorts an array that pulled in the values from the Elf_Rela section and has already done the swapping.

Create two new compare functions that will sort the direct values. One will sort 32 bit values and the other will sort the 64 bit value. One of these will be assigned to a compare_values function pointer and that will be used for sorting the Elf_Rela mcount values.

---
 scripts/sorttable.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

index 4a34c275123e..f62a91d8af0a 100644
@@ -552,6 +552,28 @@ static void *sort_orctable(void *arg)
 
 #ifdef MCOUNT_SORT_ENABLED
 
+static int compare_values_64(const void *a, const void *b)
+{
+	uint64_t av = *(uint64_t *)a;
+	uint64_t bv = *(uint64_t *)b;
+
+	if (av < bv)
+		return -1;
+	return av > bv;
+}
+
+static int compare_values_32(const void *a, const void *b)
+{
+	uint32_t av = *(uint32_t *)a;
+	uint32_t bv = *(uint32_t *)b;
+
+	if (av < bv)
+		return -1;
+	return av > bv;
+}
+
+static int (*compare_values)(const void *a, const void *b);
+
 /* Only used for sorting mcount table */
 static void rela_write_addend(Elf_Rela *rela, uint64_t val)
 {

@@ -583,6 +605,8 @@ static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
 	void *vals;
 	void *ptr;
 
+	compare_values = long_size == 4 ? compare_values_32 : compare_values_64;
+
 	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
 	shentsize = ehdr_shentsize(ehdr);
 

@@ -640,7 +664,7 @@ static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
 		}
 	}
 	count = ptr - vals;
-	qsort(vals, count / long_size, long_size, compare_extable);
+	qsort(vals, count / long_size, long_size, compare_values);
 
 	ptr = vals;
 	for (int i = 0; i < shnum; i++) {

2.47.2

From: Steven Rostedt <[email protected]>

The sorting of the mcount_loc section is done directly to the section for x86 and arm32 but it uses a separate array for arm64 as arm64 has the values for the mcount_loc stored in the rela sections of the vmlinux ELF file.

In order to use the same code to remove weak functions, always use a separate array to do the sorting. This requires splitting up the filling of the array into one function and the placing the contents of the array back into the rela sections or into the mcount_loc section into a separate file.

---
 scripts/sorttable.c | 122 ++++++++++++++++++++++++++++++++------------
 1 file changed, 90 insertions(+), 32 deletions(-)

index f62a91d8af0a..ec02a2852efb 100644
@@ -594,31 +594,19 @@ struct elf_mcount_loc {
 	uint64_t stop_mcount_loc;
 };
 
-/* Sort the relocations not the address itself */
-static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
+/* Fill the array with the content of the relocs */
+static int fill_relocs(void *ptr, uint64_t size, Elf_Ehdr *ehdr, uint64_t start_loc)
 {
 	Elf_Shdr *shdr_start;
 	Elf_Rela *rel;
 	unsigned int shnum;
-	unsigned int count;
+	unsigned int count = 0;
 	int shentsize;
-	void *vals;
-	void *ptr;
-
-	compare_values = long_size == 4 ? compare_values_32 : compare_values_64;
+	void *array_end = ptr + size;
 
 	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
 	shentsize = ehdr_shentsize(ehdr);
 
-	vals = malloc(long_size * size);
-	if (!vals) {
-		snprintf(m_err, ERRSTR_MAXSZ, "Failed to allocate sort array");
-		pthread_exit(m_err);
-		return NULL;
-	}
-
-	ptr = vals;
-
 	shnum = ehdr_shnum(ehdr);
 	if (shnum == SHN_UNDEF)
 		shnum = shdr_size(shdr_start);

@@ -637,22 +625,18 @@ static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
 			uint64_t offset = rela_offset(rel);
 
 			if (offset >= start_loc && offset < start_loc + size) {
-				if (ptr + long_size > vals + size) {
-					free(vals);
+				if (ptr + long_size > array_end) {
 					snprintf(m_err, ERRSTR_MAXSZ,
 						 "Too many relocations");
-					pthread_exit(m_err);
-					return NULL;
+					return -1;
 				}
 
 				/* Make sure this has the correct type */
 				if (rela_info(rel) != rela_type) {
-					free(vals);
 					snprintf(m_err, ERRSTR_MAXSZ,
 						"rela has type %lx but expected %lx\n",
 						(long)rela_info(rel), rela_type);
-					pthread_exit(m_err);
-					return NULL;
+					return -1;
 				}
 
 				if (long_size == 4)

@@ -660,13 +644,28 @@ static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
 				else
 					*(uint64_t *)ptr = rela_addend(rel);
 				ptr += long_size;
+				count++;
 			}
 		}
 	}
-	count = ptr - vals;
-	qsort(vals, count / long_size, long_size, compare_values);
+	return count;
+}
+
+/* Put the sorted vals back into the relocation elements */
+static void replace_relocs(void *ptr, uint64_t size, Elf_Ehdr *ehdr, uint64_t start_loc)
+{
+	Elf_Shdr *shdr_start;
+	Elf_Rela *rel;
+	unsigned int shnum;
+	int shentsize;
+
+	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
+	shentsize = ehdr_shentsize(ehdr);
+
+	shnum = ehdr_shnum(ehdr);
+	if (shnum == SHN_UNDEF)
+		shnum = shdr_size(shdr_start);
 
-	ptr = vals;
 	for (int i = 0; i < shnum; i++) {
 		Elf_Shdr *shdr = get_index(shdr_start, shentsize, i);
 		void *end;

@@ -689,8 +688,32 @@ static void *sort_relocs(Elf_Ehdr *ehdr, uint64_t start_loc, uint64_t size)
 			}
 		}
 	}
-	free(vals);
-	return NULL;
+}
+
+static int fill_addrs(void *ptr, uint64_t size, void *addrs)
+{
+	void *end = ptr + size;
+	int count = 0;
+
+	for (; ptr < end; ptr += long_size, addrs += long_size, count++) {
+		if (long_size == 4)
+			*(uint32_t *)ptr = r(addrs);
+		else
+			*(uint64_t *)ptr = r8(addrs);
+	}
+	return count;
+}
+
+static void replace_addrs(void *ptr, uint64_t size, void *addrs)
+{
+	void *end = ptr + size;
+
+	for (; ptr < end; ptr += long_size, addrs += long_size) {
+		if (long_size == 4)
+			w(*(uint32_t *)ptr, addrs);
+		else
+			w8(*(uint64_t *)ptr, addrs);
+	}
 }
 
 /* Sort the addresses stored between __start_mcount_loc to __stop_mcount_loc in vmlinux */

@@ -699,14 +722,49 @@ static void *sort_mcount_loc(void *arg)
 	struct elf_mcount_loc *emloc = (struct elf_mcount_loc *)arg;
 	uint64_t offset = emloc->start_mcount_loc - shdr_addr(emloc->init_data_sec)
 					+ shdr_offset(emloc->init_data_sec);
-	uint64_t count = emloc->stop_mcount_loc - emloc->start_mcount_loc;
+	uint64_t size = emloc->stop_mcount_loc - emloc->start_mcount_loc;
 	unsigned char *start_loc = (void *)emloc->ehdr + offset;
+	Elf_Ehdr *ehdr = emloc->ehdr;
+	void *e_msg = NULL;
+	void *vals;
+	int count;
+
+	vals = malloc(long_size * size);
+	if (!vals) {
+		snprintf(m_err, ERRSTR_MAXSZ, "Failed to allocate sort array");
+		pthread_exit(m_err);
+	}
 
 	if (sort_reloc)
-		return sort_relocs(emloc->ehdr, emloc->start_mcount_loc, count);
+		count = fill_relocs(vals, size, ehdr, emloc->start_mcount_loc);
+	else
+		count = fill_addrs(vals, size, start_loc);
+
+	if (count < 0) {
+		e_msg = m_err;
+		goto out;
+	}
+
+	if (count != size / long_size) {
+		snprintf(m_err, ERRSTR_MAXSZ, "Expected %u mcount elements but found %u\n",
+			(int)(size / long_size), count);
+		e_msg = m_err;
+		goto out;
+	}
+
+	compare_values = long_size == 4 ? compare_values_32 : compare_values_64;
+
+	qsort(vals, count, long_size, compare_values);
+
+	if (sort_reloc)
+		replace_relocs(vals, size, ehdr, emloc->start_mcount_loc);
+	else
+		replace_addrs(vals, size, start_loc);
+
+out:
+	free(vals);
 
-	qsort(start_loc, count/long_size, long_size, compare_extable);
-	return NULL;
+	pthread_exit(e_msg);
 }
 
 /* Get the address of __start_mcount_loc and __stop_mcount_loc in System.map */

2.47.2

From: Steven Rostedt <[email protected]>

When a function is annotated as "weak" and is overridden, the code is not removed. If it is traced, the fentry/mcount location in the weak function will be referenced by the "__mcount_loc" section. This will then be added to the available_filter_functions list. Since only the address of the functions are listed, to find the name to show, a search of kallsyms is used.

Since kallsyms will return the function by simply finding the function that the address is after but before the next function, an address of a weak function will show up as the function before it. This is because kallsyms does not save names of weak functions. This has caused issues in the past, as now the traced weak function will be listed in available_filter_functions with the name of the function before it.

At best, this will cause the previous function's name to be listed twice. At worse, if the previous function was marked notrace, it will now show up as a function that can be traced. Note that it only shows up that it can be traced but will not be if enabled, which causes confusion.

https://lore.kernel.org/all/[email protected]/

The commit b39181f7c6907 ("ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function") was a workaround to this by checking the function address before printing its name. If the address was too far from the function given by the name then instead of printing the name it would
print: __ftrace_invalid_address___<invalid-offset>

The real issue is that these invalid addresses are listed in the ftrace table look up which available_filter_functions is derived from. A place holder must be listed in that file because set_ftrace_filter may take a series of indexes into that file instead of names to be able to do O(1) lookups to enable filtering (many tools use this method).

Even if kallsyms saved the size of the function, it does not remove the need of having these place holders. The real solution is to not add a weak function into the ftrace table in the first place.

To solve this, the sorttable.c code that sorts the mcount regions during the build is modified to take a "nm -S vmlinux" input, sort it, and any function listed in the mcount_loc section that is not within a boundary of the function list given by nm is considered a weak function and is zeroed out.

Note, this does not mean they will remain zero when booting as KASLR will still shift those addresses. To handle this, the entries in the mcount_loc section will be ignored if they are zero or match the kaslr_offset() value.

Before:

~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l 551

After:

~# grep __ftrace_invalid_address___ /sys/kernel/tracing/available_filter_functions | wc -l 0

---
Changes since v3: https://lore.kernel.org/[email protected]

- Do not remove 'W' weak functions that are actually used

 kernel/trace/ftrace.c   |   6 +-
 scripts/link-vmlinux.sh |   4 +-
 scripts/sorttable.c     | 128 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 134 insertions(+), 4 deletions(-)

index 728ecda6e8d4..e3f89924f603 100644
@@ -7004,6 +7004,7 @@ static int ftrace_process_locs(struct module *mod,
 	unsigned long count;
 	unsigned long *p;
 	unsigned long addr;
+	unsigned long kaslr;
 	unsigned long flags = 0; /* Shut up gcc */
 	int ret = -ENOMEM;
 

@@ -7052,6 +7053,9 @@ static int ftrace_process_locs(struct module *mod,
 		ftrace_pages->next = start_pg;
 	}
 
+	/* For zeroed locations that were shifted for core kernel */
+	kaslr = !mod ? kaslr_offset() : 0;
+
 	p = start;
 	pg = start_pg;
 	while (p < end) {

@@ -7063,7 +7067,7 @@ static int ftrace_process_locs(struct module *mod,
 		 * object files to satisfy alignments.
 		 * Skip any NULL pointers.
 		 */
-		if (!addr) {
+		if (!addr || addr == kaslr) {
 			skipped++;
 			continue;
 		}

index 56a077d204cf..59b07fe6fd00 100755
@@ -177,12 +177,14 @@ mksysmap()
 
 sorttable()
 {
-	${objtree}/scripts/sorttable ${1}
+	${NM} -S ${1} > .tmp_vmlinux.nm-sort
+	${objtree}/scripts/sorttable -s .tmp_vmlinux.nm-sort ${1}
 }
 
 cleanup()
 {
 	rm -f .btf.*
+	rm -f .tmp_vmlinux.nm-sort
 	rm -f System.map
 	rm -f vmlinux
 	rm -f vmlinux.map

2.47.2

From: Steven Rostedt <[email protected]>

Now that weak functions turn into skipped entries, update the check to make sure the amount that was allocated would fit both the entries that were allocated as well as those that were skipped.

---
 kernel/trace/ftrace.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

index e3f89924f603..55d28c060784 100644
@@ -7111,7 +7111,29 @@ static int ftrace_process_locs(struct module *mod,
 
 	/* We should have used all pages unless we skipped some */
 	if (pg_unuse) {
-		WARN_ON(!skipped);
+		unsigned long pg_remaining, remaining;
+		unsigned long skip;
+
+		/* Count the number of entries unused and compare it to skipped. */
+		pg_remaining = (ENTRIES_PER_PAGE << pg->order) - pg->index;
+
+		if (!WARN(skipped < pg_remaining, "Extra allocated pages for ftrace")) {
+
+			skip = skipped - pg_remaining;
+
+			for (pg = pg_unuse; pg; pg = pg->next) {
+				remaining += 1 << pg->order;
+			}
+
+			skip = DIV_ROUND_UP(skip, ENTRIES_PER_PAGE);
+
+			/*
+			 * Check to see if the number of pages remaining would
+			 * just fit the number of entries skipped.
+			 */
+			WARN(skip != remaining, "Extra allocated pages for ftrace: %lu with %lu skipped",
+			     remaining, skipped);
+		}
 		/* Need to synchronize with ftrace_location_range() */
 		synchronize_rcu();
 		ftrace_free_pages(pg_unuse);

2.47.2

From: Steven Rostedt <[email protected]>

The amount of memory that ftrace uses to save the descriptors to manage the functions it can trace is shown at output. But if there are a lot of functions that are skipped because they were weak or the architecture added holes into the tables, then the extra pages that were allocated are freed. But these freed pages are not reflected in the numbers shown, and they can even be inconsistent with what is reported:

ftrace: allocating 57482 entries in 225 pages
ftrace: allocated 224 pages with 3 groups

The above shows the number of original entries that are in the mcount_loc section and the pages needed to save them (225), but the second output reflects the number of pages that were actually used. The two should be consistent as:

ftrace: allocating 56739 entries in 224 pages
ftrace: allocated 224 pages with 3 groups

The above also shows the accurate number of entires that were actually stored and does not include the entries that were removed.

---
 kernel/trace/ftrace.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

index 55d28c060784..fa7c3417d995 100644
@@ -7006,6 +7006,7 @@ static int ftrace_process_locs(struct module *mod,
 	unsigned long addr;
 	unsigned long kaslr;
 	unsigned long flags = 0; /* Shut up gcc */
+	unsigned long pages;
 	int ret = -ENOMEM;
 
 	count = end - start;

@@ -7013,6 +7014,8 @@ static int ftrace_process_locs(struct module *mod,
 	if (!count)
 		return 0;
 
+	pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
+
 	/*
 	 * Sorting mcount in vmlinux at build time depend on
 	 * CONFIG_BUILDTIME_MCOUNT_SORT, while mcount loc in

@@ -7125,6 +7128,8 @@ static int ftrace_process_locs(struct module *mod,
 				remaining += 1 << pg->order;
 			}
 
+			pages -= remaining;
+
 			skip = DIV_ROUND_UP(skip, ENTRIES_PER_PAGE);
 
 			/*

@@ -7138,6 +7143,13 @@ static int ftrace_process_locs(struct module *mod,
 		synchronize_rcu();
 		ftrace_free_pages(pg_unuse);
 	}
+
+	if (!mod) {
+		count -= skipped;
+		pr_info("ftrace: allocating %ld entries in %ld pages\n",
+			count, pages);
+	}
+
 	return ret;
 }
 

@@ -7783,9 +7795,6 @@ void __init ftrace_init(void)
 		goto failed;
 	}
 
-	pr_info("ftrace: allocating %ld entries in %ld pages\n",
-		count, DIV_ROUND_UP(count, ENTRIES_PER_PAGE));
-
 	ret = ftrace_process_locs(NULL,
 				  __start_mcount_loc,
 				  __stop_mcount_loc);

2.47.2

Hi Steven,

Since you asked me to test this on s390: seems to work with HAVE_BUILDTIME_MCOUNT_SORT enabled; the ftrace selftests still work as before.

My tests found a bug (forgot to initialize a variable) and I will be sending a v5 soon.

You may also want to enable: CONFIG_FTRACE_SORT_STARTUP_TEST

That will make ftrace test to see if the mcount table is indeed sorted at boot up.

-- Steve

Great!

I'm guessing by just adding the support in s390 with what is upstream as well as what is in my for-next would work? You can just add that for the next merge window then.

Expect this code to be in linux-next later this week.

-- Steve

Yes, both variants work.

It is already in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=fa1518875286c94111bdaf1c7bae188c9c426c6b

Thanks for making aware of this!