Re: [PATCH v4][makedumpfile 6/7] Add makedumpfile extensions support

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Tao Liu <ltao@redhat.com> writes:
> The extensions can be specified by makedumpfile cmdline parameter as
> "--extension", followed by extension's filename or absolute path. If
> filename is give, then "./extenisons" and "/usr/lib64/makedumpfile/extensions/"
> will be searched.
>
> The procedures of extensions are as follows:
>
> Step 0: Every extensions will declare which kernel symbol/types they needed
> during programming. This info will be stored within .init_ksyms/ktypes section.
> Also extension will have a callback function for makedumpfile to call.
>
> Step 1: Register .init_ksyms and .init_ktypes sections of makedumpfile
> itself and extension's .so files, then tell kallsyms/btf subcomponent that which
> kernel symbols/types will be resolved. And callbacks are also registered.
>
> Step 2: Init kernel/module's btf/kallsyms on demand. Any un-needed kenrel
> modules will be skipped.
>
> Step 3: During btf/kallsyms parsing, the needed info will be filled. For
> syms/types which are defined via INIT_OPT(...) macro, these are optinal
> syms/types, it won't fail at parsing step if any are missing, instead, they
> need to be checked within extension_init() of each extensions; For         
> syms/types which defined via INIT_(...) macro, these are must-have syms/types,
> if any missing, the extension will fail at this step and as a result
> this extension will be skipped.
>
> After this step, required kernel symbol value and kernel types size/offset
> are resolved, the extensions are ready to go.
>
> Step 4: When makedumpfile doing page filtering, in addition to its
> original filtering mechanism, it will call extensions callbacks for advice
> whether the page should be included/excluded.
>
> Suggested-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Signed-off-by: Tao Liu <ltao@redhat.com>
> ---
>  Makefile            |   7 +-
>  extension.c         | 300 ++++++++++++++++++++++++++++++++++++++++++++
>  extension.h         |  12 ++
>  extensions/Makefile |  10 ++
>  makedumpfile.c      |  38 +++++-
>  makedumpfile.h      |   2 +

I think the manual page will need updating with this change.
Documentation updates could probably be done in a separate patch, though.

>  6 files changed, 363 insertions(+), 6 deletions(-)
>  create mode 100644 extension.c
>  create mode 100644 extension.h
>  create mode 100644 extensions/Makefile
>
> diff --git a/Makefile b/Makefile
> index 320677d..1bb67d9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -45,7 +45,7 @@ CFLAGS_ARCH += -m32
>  endif
>  
>  SRC_BASE = makedumpfile.c makedumpfile.h diskdump_mod.h sadump_mod.h sadump_info.h
> -SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c cache.c tools.c printk.c detect_cycle.c kallsyms.c btf_info.c
> +SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c cache.c tools.c printk.c detect_cycle.c kallsyms.c btf_info.c extension.c
>  OBJ_PART=$(patsubst %.c,%.o,$(SRC_PART))
>  SRC_ARCH = arch/arm.c arch/arm64.c arch/x86.c arch/x86_64.c arch/ia64.c arch/ppc64.c arch/s390x.c arch/ppc.c arch/sparc64.c arch/mips64.c arch/loongarch64.c arch/riscv64.c
>  OBJ_ARCH=$(patsubst %.c,%.o,$(SRC_ARCH))
> @@ -126,6 +126,7 @@ eppic_makedumpfile.so: extension_eppic.c
>  
>  clean:
>  	rm -f $(OBJ) $(OBJ_PART) $(OBJ_ARCH) makedumpfile makedumpfile.8 makedumpfile.conf.5
> +	$(MAKE) -C extensions clean
>  
>  install:
>  	install -m 755 -d ${DESTDIR}/${SBINDIR} ${DESTDIR}/usr/share/man/man5 ${DESTDIR}/usr/share/man/man8
> @@ -135,3 +136,7 @@ install:
>  	mkdir -p ${DESTDIR}/usr/share/makedumpfile/eppic_scripts
>  	install -m 644 -D $(VPATH)makedumpfile.conf ${DESTDIR}/usr/share/makedumpfile/makedumpfile.conf.sample
>  	install -m 644 -t ${DESTDIR}/usr/share/makedumpfile/eppic_scripts/ $(VPATH)eppic_scripts/*
> +
> +.PHONY: extensions
> +extensions:
> +	$(MAKE) -C extensions CC=$(CC)
> \ No newline at end of file
> diff --git a/extension.c b/extension.c
> new file mode 100644
> index 0000000..35e2756
> --- /dev/null
> +++ b/extension.c
> @@ -0,0 +1,300 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <dirent.h>
> +#include <dlfcn.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include "kallsyms.h"
> +#include "btf_info.h"
> +#include "extension.h"
> +
> +typedef int (*callback_fn)(unsigned long, const void *);
> +
> +struct extension_handle_cb {
> +	void *handle;
> +	callback_fn cb;
> +};
> +
> +/* Extension .so extension_handle_cb array */
> +static struct extension_handle_cb **handle_cbs = NULL;
> +static int handle_cbs_len = 0;
> +static int handle_cbs_cap = 0;
> +
> +/* Extension option array */
> +static char **extension_opts = NULL;
> +static int extension_opts_len = 0;
> +static int extension_opts_cap = 0;
> +
> +static const char *dirs[] = {
> +	"/usr/lib64/makedumpfile/extensions/",
> +	"./extensions/",
> +};
> +
> +void add_extension_opts(char *opt)
> +{
> +	if (!add_to_arr((void ***)&extension_opts, &extension_opts_len,
> +			&extension_opts_cap, opt))
> +		/*
> +		 * If fail, print error info and skip the extension.
> +		*/
> +		 fprintf(stderr, "%s: Fail to add extension %s\n", __func__, opt);
> +}
> +
> +static bool init_kallsyms_btf(void)
> +{
> +	int count;
> +	bool ret = false;
> +	/* We will load module's btf/kallsyms on demand */
> +	bool init_ksyms_module = false;
> +	bool init_ktypes_module = false;
> +
> +	if (check_ksyms_require_modname("vmlinux", &count)) {
> +		if (!init_kernel_kallsyms())
> +			goto out;
> +		if (count >= 2)
> +			init_ksyms_module = true;
> +	}
> +	if (check_ktypes_require_modname("vmlinux", &count)) {
> +		if (!init_kernel_btf())
> +			goto out;
> +		if (count >= 2)
> +			init_ktypes_module = true;
> +	}
> +	if (init_ksyms_module && !init_module_kallsyms())
> +		goto out;
> +	if (init_ktypes_module && !init_module_btf())
> +		goto out;	
> +	ret = true;
> +out:
> +	return ret;
> +}
> +
> +static void cleanup_kallsyms_btf(void)
> +{
> +	cleanup_kallsyms();
> +	cleanup_btf();
> +}
> +
> +static void load_extensions(void)
> +{
> +	char path[512];
> +	int len, i, j;
> +	void *handle;
> +	struct extension_handle_cb *ehc;
> +
> +	for (i = 0; i < extension_opts_len; i++) {
> +		handle = NULL;
> +		if (!extension_opts[i])
> +			continue;
> +		if ((len = strlen(extension_opts[i])) <= 3 ||
> +		    (strcmp(extension_opts[i] + len - 3, ".so") != 0)) {
> +			fprintf(stderr, "%s: Skip invalid extension: %s\n",
> +				__func__, extension_opts[i]);
> +			continue;
> +		}
> +
> +		if (extension_opts[i][0] == '/') {
> +			/* Path & filename */
> +			snprintf(path, sizeof(path), "%s", extension_opts[i]);
> +			handle = dlopen(path, RTLD_NOW);
> +			if (!handle) {
> +				fprintf(stderr, "%s: Failed to load %s\n",
> +					__func__, dlerror());
> +				continue;
> +			}
> +		} else {
> +			/* Only filename */
> +			for (j = 0; j < sizeof(dirs) / sizeof(char *); j++) {
> +				snprintf(path, sizeof(path), "%s", dirs[j]);
> +				len = strlen(path);
> +				snprintf(path + len, sizeof(path) - len, "%s",
> +					extension_opts[i]);
> +				if (access(path, F_OK) == 0) {
> +					handle = dlopen(path, RTLD_NOW);
> +					if (handle)
> +						break;
> +					else
> +						fprintf(stderr, "%s: Failed to load %s\n",
> +							__func__, dlerror());					
> +				}
> +			}
> +			if (!handle && j >= sizeof(dirs) / sizeof(char *)) {
> +				fprintf(stderr, "%s: Not found %s\n",
> +					__func__, extension_opts[i]);
> +				continue;				
> +			}
> +		}
> +
> +		if (dlsym(handle, "extension_init") == NULL) {
> +			fprintf(stderr, "%s: Skip extension %s: No extension_init()\n",
> +				__func__, path);
> +			dlclose(handle);
> +			continue;
> +		}
> +
> +		if ((ehc = malloc(sizeof(struct extension_handle_cb))) == NULL) {
> +			fprintf(stderr, "%s: Skip extension %s: No memory\n",
> +				__func__, path);
> +			dlclose(handle);
> +			continue;			
> +		}
> +
> +		ehc->handle = handle;
> +		ehc->cb = dlsym(handle, "extension_callback");
> +
> +		if (!add_to_arr((void ***)&handle_cbs, &handle_cbs_len, &handle_cbs_cap, ehc)) {
> +			fprintf(stderr, "%s: Failed to load %s\n", __func__,
> +				extension_opts[i]);
> +			free(ehc);
> +			dlclose(handle);
> +			continue;
> +		}
> +		printf("Loaded extension: %s\n", path);
> +	}
> +}
> +
> +static bool register_extension_sections(void)
> +{
> +	char *start, *stop;
> +	int i;
> +	bool ret = false;
> +
> +	for (i = 0; i < handle_cbs_len; i++) {
> +		start = dlsym(handle_cbs[i]->handle, "__start_init_ksyms");
> +		stop = dlsym(handle_cbs[i]->handle, "__stop_init_ksyms");
> +		if (!register_ksym_section(start, stop))
> +			goto out;
> +
> +		start = dlsym(handle_cbs[i]->handle, "__start_init_ktypes");
> +		stop = dlsym(handle_cbs[i]->handle, "__stop_init_ktypes");
> +		if (!register_ktype_section(start, stop))
> +			goto out;
> +	}
> +	ret = true;
> +out:
> +	return ret;
> +}
> +
> +void cleanup_extensions(void)
> +{
> +	for (int i = 0; i < handle_cbs_len; i++) {
> +		dlclose(handle_cbs[i]->handle);
> +		free(handle_cbs[i]);
> +	}
> +	if (handle_cbs) {
> +		free(handle_cbs);
> +		handle_cbs = NULL;
> +	}
> +	handle_cbs_len = 0;
> +	handle_cbs_cap = 0;
> +	if (extension_opts) {
> +		free(extension_opts);
> +		extension_opts = NULL;
> +	}
> +	extension_opts_len = 0;
> +	extension_opts_cap = 0;
> +
> +	cleanup_kallsyms_btf();
> +}
> +
> +static bool check_required_ksyms_all_resolved(void *handle)
> +{
> +	char *start, *stop;
> +	struct ksym_info **p;
> +	bool ret = true;
> +
> +	start = dlsym(handle, "__start_init_ksyms");
> +	stop = dlsym(handle, "__stop_init_ksyms");
> +
> +	for (p = (struct ksym_info **)start;
> +	     p < (struct ksym_info **)stop;
> +	     p++) {
> +		if ((*p)->sym_required && !SYM_EXIST(*p)) {
> +			ret = false;
> +			fprintf(stderr, "Symbol %s in %s not found\n",
> +				(*p)->symname, (*p)->modname);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static bool check_required_ktypes_all_resolved(void *handle)
> +{
> +	char *start, *stop;
> +	struct ktype_info **p;
> +	bool ret = true;
> +
> +	start = dlsym(handle, "__start_init_ktypes");
> +	stop = dlsym(handle, "__stop_init_ktypes");
> +
> +	for (p = (struct ktype_info **)start;
> +	     p < (struct ktype_info **)stop;
> +	     p++) {
> +		if (!TYPE_EXIST(*p)) {
> +			if ((*p)->member_required) {
> +				ret = false;
> +				fprintf(stderr, "Member %s of struct %s in %s not found\n",
> +					(*p)->member_name, (*p)->struct_name, (*p)->modname);
> +			} else if ((*p)->struct_required) {
> +				ret = false;
> +				fprintf(stderr, "Struct %s in %s not found\n",
> +					(*p)->struct_name, (*p)->modname);
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static bool extension_runnable(void *handle)
> +{
> +	return check_required_ksyms_all_resolved(handle) &&
> +		check_required_ktypes_all_resolved(handle);
> +}
> +
> +void init_extensions(void)
> +{
> +	/* Entry of extension init */
> +	void (*init)(void);
> +
> +	load_extensions();
> +	if (!register_extension_sections())
> +		goto fail;
> +	if (!init_kallsyms_btf()) 
> +		goto fail;
> +	for (int i = 0; i < handle_cbs_len; i++) {
> +		if (extension_runnable(handle_cbs[i]->handle)) {
> +			init = dlsym(handle_cbs[i]->handle, "extension_init");
> +			init();
> +		} else {
> +			fprintf(stderr, "%s: Skip %dth extension\n",
> +				__func__, i + 1);
> +		}
> +	}
> +	return;
> +fail:
> +	fprintf(stderr, "%s: fail & skip all extensions\n", __func__);
> +	cleanup_extensions();
> +}
> +
> +int run_extension_callback(unsigned long pfn, const void *pcache)
> +{
> +	int result;
> +	int ret = PG_UNDECID;
> +
> +	for (int i = 0; i < handle_cbs_len; i++) {
> +		if (handle_cbs[i]->cb) {
> +			result = handle_cbs[i]->cb(pfn, pcache);
> +			if (result == PG_INCLUDE) {
> +				ret = result;
> +				goto out;
> +			} else if (result == PG_EXCLUDE) {
> +				ret = result;
> +			}
> +		}
> +	}
> +out:
> +	return ret;
> +}
> \ No newline at end of file
> diff --git a/extension.h b/extension.h
> new file mode 100644
> index 0000000..dc5902e
> --- /dev/null
> +++ b/extension.h
> @@ -0,0 +1,12 @@
> +#ifndef _EXTENSION_H
> +#define _EXTENSION_H
> +
> +enum {
> +	PG_INCLUDE,	// Exntesion will keep the page
> +	PG_EXCLUDE,	// Exntesion will discard the page
> +	PG_UNDECID,	// Exntesion makes no decision
> +};
> +int run_extension_callback(unsigned long pfn, const void *pcache);
> +void init_extensions(void);
> +void cleanup_extensions(void);
> +#endif /* _EXTENSION_H */
> \ No newline at end of file
> diff --git a/extensions/Makefile b/extensions/Makefile
> new file mode 100644
> index 0000000..b8bbfbc
> --- /dev/null
> +++ b/extensions/Makefile
> @@ -0,0 +1,10 @@
> +CC ?= gcc
> +CONTRIB_SO :=
> +
> +all: $(CONTRIB_SO)
> +
> +$(CONTRIB_SO): %.so: %.c
> +	$(CC) -O2 -g -fPIC -shared -Wl,-T,../makedumpfile.ld -o $@ $^
> +
> +clean:
> +	rm -f $(CONTRIB_SO)
> \ No newline at end of file
> diff --git a/makedumpfile.c b/makedumpfile.c
> index dba3628..ef7468f 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -28,6 +28,7 @@
>  #include <assert.h>
>  #include <zlib.h>
>  #include "kallsyms.h"
> +#include "extension.h"
>  
>  struct symbol_table	symbol_table;
>  struct size_table	size_table;
> @@ -102,6 +103,7 @@ mdf_pfn_t pfn_free;
>  mdf_pfn_t pfn_hwpoison;
>  mdf_pfn_t pfn_offline;
>  mdf_pfn_t pfn_elf_excluded;
> +mdf_pfn_t pfn_extension;
>  
>  mdf_pfn_t num_dumped;
>  
> @@ -6459,6 +6461,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>  	unsigned int order_offset, dtor_offset;
>  	unsigned long flags, mapping, private = 0;
>  	unsigned long compound_dtor, compound_head = 0;
> +	int filter_pg;
>  
>  	/*
>  	 * If a multi-page exclusion is pending, do it first
> @@ -6531,6 +6534,14 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>  			pfn_read_end   = pfn + pfn_mm - 1;
>  		}
>  
> +		/*
> +		 * Include pages that specified by user via
> +		 * makedumpfile extensions
> +		 */
> +		filter_pg = run_extension_callback(pfn, pcache);
> +		if (filter_pg == PG_INCLUDE)
> +			continue;
> +
>  		flags   = ULONG(pcache + OFFSET(page.flags));
>  		_count  = UINT(pcache + OFFSET(page._refcount));
>  		mapping = ULONG(pcache + OFFSET(page.mapping));
> @@ -6687,6 +6698,14 @@ check_order:
>  		else if (isOffline(flags, _mapcount)) {
>  			pfn_counter = &pfn_offline;
>  		}
> +		/*
> +		 * Exclude pages that specified by user via
> +		 * makedumpfile extensions
> +		 */
> +		else if (filter_pg == PG_EXCLUDE) {
> +			nr_pages = 1;
> +			pfn_counter = &pfn_extension;
> +		}
>  		/*
>  		 * Unexcludable page
>  		 */
> @@ -8234,7 +8253,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_page)
>  	 */
>  	if (info->flag_cyclic) {
>  		pfn_zero = pfn_cache = pfn_cache_private = 0;
> -		pfn_user = pfn_free = pfn_hwpoison = pfn_offline = 0;
> +		pfn_user = pfn_free = pfn_hwpoison = pfn_offline = pfn_extension = 0;
>  		pfn_memhole = info->max_mapnr;
>  	}
>  
> @@ -9579,7 +9598,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data *cd_header, struct cache_d
>  		 * Reset counter for debug message.
>  		 */
>  		pfn_zero = pfn_cache = pfn_cache_private = 0;
> -		pfn_user = pfn_free = pfn_hwpoison = pfn_offline = 0;
> +		pfn_user = pfn_free = pfn_hwpoison = pfn_offline = pfn_extension = 0;
>  		pfn_memhole = info->max_mapnr;
>  
>  		/*
> @@ -10528,7 +10547,7 @@ print_report(void)
>  	pfn_original = info->max_mapnr - pfn_memhole;
>  
>  	pfn_excluded = pfn_zero + pfn_cache + pfn_cache_private
> -	    + pfn_user + pfn_free + pfn_hwpoison + pfn_offline;
> +	    + pfn_user + pfn_free + pfn_hwpoison + pfn_offline + pfn_extension;
>  
>  	REPORT_MSG("\n");
>  	REPORT_MSG("Original pages  : 0x%016llx\n", pfn_original);
> @@ -10544,6 +10563,7 @@ print_report(void)
>  	REPORT_MSG("    Free pages              : 0x%016llx\n", pfn_free);
>  	REPORT_MSG("    Hwpoison pages          : 0x%016llx\n", pfn_hwpoison);
>  	REPORT_MSG("    Offline pages           : 0x%016llx\n", pfn_offline);
> +	REPORT_MSG("    Extension filter pages  : 0x%016llx\n", pfn_extension);
>  	REPORT_MSG("  Remaining pages  : 0x%016llx\n",
>  	    pfn_original - pfn_excluded);
>  
> @@ -10584,7 +10604,7 @@ print_mem_usage(void)
>  	pfn_original = info->max_mapnr - pfn_memhole;
>  
>  	pfn_excluded = pfn_zero + pfn_cache + pfn_cache_private
> -	    + pfn_user + pfn_free + pfn_hwpoison + pfn_offline;
> +	    + pfn_user + pfn_free + pfn_hwpoison + pfn_offline + pfn_extension;
>  	shrinking = (pfn_original - pfn_excluded) * 100;
>  	shrinking = shrinking / pfn_original;
>  	total_size = info->page_size * pfn_original;
> @@ -10878,6 +10898,7 @@ create_dumpfile(void)
>  	}
>  
>  	print_vtop();
> +	init_extensions();
>  
>  	num_retry = 0;
>  retry:
> @@ -10888,8 +10909,11 @@ retry:
>  			&& !gather_filter_info())
>  		return FALSE;
>  
> -	if (!create_dump_bitmap())
> +	if (!create_dump_bitmap()) {
> +		cleanup_extensions();
>  		return FALSE;
> +	}
> +	cleanup_extensions();

If a retry happens, then all of the data related to extensions will be
cleared: including the list of extensions, the callbacks, etc.
Functionally, extensions will be disabled on the retry. Is that
intentional?

If not, then I think we'll need to move this cleanup so that it happens
when we exit the retry loop (either by an early return, or on success).

If this is intentional, then it probably ought to be documented.

Thanks,
Stephen

>  	if (info->flag_split) {
>  		if ((status = writeout_multiple_dumpfiles()) == FALSE)
> @@ -12130,6 +12154,7 @@ static struct option longopts[] = {
>  	{"check-params", no_argument, NULL, OPT_CHECK_PARAMS},
>  	{"dry-run", no_argument, NULL, OPT_DRY_RUN},
>  	{"show-stats", no_argument, NULL, OPT_SHOW_STATS},
> +	{"extension", required_argument, NULL, OPT_EXTENSION},
>  	{0, 0, 0, 0}
>  };
>  
> @@ -12317,6 +12342,9 @@ main(int argc, char *argv[])
>  		case OPT_SHOW_STATS:
>  			flag_show_stats = TRUE;
>  			break;
> +		case OPT_EXTENSION:
> +			add_extension_opts(optarg);
> +			break;
>  		case '?':
>  			MSG("Commandline parameter is invalid.\n");
>  			MSG("Try `makedumpfile --help' for more information.\n");
> diff --git a/makedumpfile.h b/makedumpfile.h
> index 0f13743..d880ae7 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -2747,6 +2747,7 @@ struct elf_prstatus {
>  #define OPT_CHECK_PARAMS        OPT_START+18
>  #define OPT_DRY_RUN             OPT_START+19
>  #define OPT_SHOW_STATS          OPT_START+20
> +#define OPT_EXTENSION           OPT_START+21
>  
>  /*
>   * Function Prototype.
> @@ -2777,5 +2778,6 @@ int write_and_check_space(int fd, void *buf, size_t buf_size,
>  int open_dump_file(void);
>  int dump_lockless_dmesg(void);
>  unsigned long long memparse(char *ptr, char **retptr);
> +void add_extension_opts(char *opt);
>  
>  #endif /* MAKEDUMPFILE_H */
> -- 
> 2.47.0