Date:   Mon, 5 Aug 2019 11:41:09 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Peter Wu <peter@...ensteyn.nl>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, Stanislav Fomichev <sdf@...gle.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH] tools: bpftool: fix reading from /proc/config.gz

Hi Peter,

Thanks for looking into this (and for the fixes)! Some comments below.

2019-08-05 01:15 UTC+0100 ~ Peter Wu <peter@...ensteyn.nl>
> /proc/config has never existed as far as I can see, but /proc/config.gz

As far as I understood (from examining Cilium [0]), /proc/config _is_
used by some distributions, such as CoreOS. This is why we look at that
location in bpftool.

[0] https://github.com/cilium/cilium/blob/master/bpf/run_probes.sh#L42

> is present on Arch Linux. Execute an external gunzip program to avoid
> linking to zlib and rework the option scanning code since a pipe is not
> seekable. This also fixes a file handle leak on some error paths.
> 
> Fixes: 4567b983f78c ("tools: bpftool: add probes for kernel configuration options")
> Cc: Quentin Monnet <quentin.monnet@...ronome.com>
> Signed-off-by: Peter Wu <peter@...ensteyn.nl>
> ---
>  tools/bpf/bpftool/feature.c | 92 +++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index d672d9086fff..e9e10f582047 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -284,34 +284,34 @@ static void probe_jit_limit(void)
>  	}
>  }
>  
> -static char *get_kernel_config_option(FILE *fd, const char *option)
> +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
> +				     char **value)

Maybe we could rename this function, and have "next" appear in it
somewhere? After your changes, it does not return the value for a
specific option anymore.

>  {
> -	size_t line_n = 0, optlen = strlen(option);
> -	char *res, *strval, *line = NULL;
> -	ssize_t n;
> +	char *sep;
> +	ssize_t linelen;

Please order the declarations in reverse-Christmas tree style.

>  
> -	rewind(fd);
> -	while ((n = getline(&line, &line_n, fd)) > 0) {
> -		if (strncmp(line, option, optlen))
> +	while ((linelen = getline(buf_p, n_p, fd)) > 0) {
> +		char *line = *buf_p;

Please leave a blank line after declarations.

> +		if (strncmp(line, "CONFIG_", 7))
>  			continue;
> -		/* Check we have at least '=', value, and '\n' */
> -		if (strlen(line) < optlen + 3)
> -			continue;
> -		if (*(line + optlen) != '=')
> +
> +		sep = memchr(line, '=', linelen);
> +		if (!sep)
>  			continue;
>  
>  		/* Trim ending '\n' */
> -		line[strlen(line) - 1] = '\0';
> +		line[linelen - 1] = '\0';
> +
> +		/* Split on '=' and ensure that a value is present. */
> +		*sep = '\0';
> +		if (!sep[1])
> +			continue;
>  
> -		/* Copy and return config option value */
> -		strval = line + optlen + 1;
> -		res = strdup(strval);
> -		free(line);
> -		return res;
> +		*value = sep + 1;
> +		return true;
>  	}
> -	free(line);
>  
> -	return NULL;
> +	return false;
>  }
>  
>  static void probe_kernel_image_config(void)
> @@ -386,31 +386,34 @@ static void probe_kernel_image_config(void)
>  		/* test_bpf module for BPF tests */
>  		"CONFIG_TEST_BPF",
>  	};
> +	char *values[ARRAY_SIZE(options)] = { };
>  	char *value, *buf = NULL;
>  	struct utsname utsn;
>  	char path[PATH_MAX];
>  	size_t i, n;
>  	ssize_t ret;
> -	FILE *fd;
> +	FILE *fd = NULL;
> +	bool is_pipe = false;

Reverse-Christmas-tree style please.

>  
>  	if (uname(&utsn))
> -		goto no_config;
> +		goto end_parse;

Just thinking, maybe if uname() fails we can skip /boot/config-$(uname
-r) but still attempt to parse /proc/config{,.gz} instead of printing
only NULL options?

>  
>  	snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
>  
>  	fd = fopen(path, "r");
>  	if (!fd && errno == ENOENT) {
> -		/* Some distributions put the config file at /proc/config, give
> -		 * it a try.
> -		 * Sometimes it is also at /proc/config.gz but we do not try
> -		 * this one for now, it would require linking against libz.
> +		/* Some distributions build with CONFIG_IKCONFIG=y and put the
> +		 * config file at /proc/config.gz. We try to invoke an external
> +		 * gzip program to avoid linking to libz.
> +		 * Hide stderr to avoid interference with the JSON output.

Because some distributions do use /proc/config, we should keep this. You
can probably add /proc/config.gz as another attempt below (or even
above) the current case?

>  		 */
> -		fd = fopen("/proc/config", "r");
> +		fd = popen("gunzip -c /proc/config.gz 2>/dev/null", "r");
> +		is_pipe = true;
>  	}
>  	if (!fd) {
>  		p_info("skipping kernel config, can't open file: %s",
>  		       strerror(errno));
> -		goto no_config;
> +		goto end_parse;
>  	}
>  	/* Sanity checks */
>  	ret = getline(&buf, &n, fd);
> @@ -418,27 +421,36 @@ static void probe_kernel_image_config(void)
>  	if (!buf || !ret) {
>  		p_info("skipping kernel config, can't read from file: %s",
>  		       strerror(errno));
> -		free(buf);
> -		goto no_config;
> +		goto end_parse;
>  	}
>  	if (strcmp(buf, "# Automatically generated file; DO NOT EDIT.\n")) {
>  		p_info("skipping kernel config, can't find correct file");
> -		free(buf);
> -		goto no_config;
> +		goto end_parse;
> +	}
> +
> +	while (get_kernel_config_option(fd, &buf, &n, &value))> +		for (i = 0; i < ARRAY_SIZE(options); i++) {
> +			if (values[i] || strcmp(buf, options[i]))

Can we have an option set multiple times in the config file? If so,
maybe have a p_info() if values are different to warn users that
conflicting values were found?

(Reading the file just once is a nice improvement over my version, by
the way, thanks!)

> +				continue;
> +
> +			values[i] = strdup(value);
> +		}
> +	}
> +
> +end_parse:
> +	if (fd) {
> +		if (is_pipe) {
> +			if (pclose(fd))
> +				p_info("failed to read /proc/config.gz");
> +		} else
> +			fclose(fd);

Please use braces on all branches of the conditional statement (else).

Thanks,
Quentin

Powered by blists - more mailing lists