netdev - Re: [PATCH] tools: bpftool: fix reading from /proc/config.gz

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 6 Aug 2019 10:36:53 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Peter Wu <peter@...ensteyn.nl>
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Stanislav Fomichev <sdf@...ichev.me>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH] tools: bpftool: fix reading from /proc/config.gz

Hi Peter,

2019-08-06 00:54 UTC+0100 ~ Peter Wu <peter@...ensteyn.nl>
> Hi all,
> 
> Thank you for your quick feedback, I will address them in the next
> revision.
> 
> On Mon, Aug 05, 2019 at 11:41:09AM +0100, Quentin Monnet wrote:
> 
>> 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
> 
> This comment[1] says "CoreOS uses /proc/config", but I think that is a
> typo and is supposed to say "/proc/config.gz". The original feature
> request[2] uses "/boot/config" as example.
> 
>  [1]: https://github.com/cilium/cilium/pull/1065
>  [2]: https://github.com/cilium/cilium/issues/891
> 
> Given that "/proc/config.gz" is the standard since at least v2.6.12-rc2,
> and the official kernel has no mention of "/proc/config", I would like
> to skip the latter. If someone has a need for this and it is not covered
> by either /boot/config-$(uname -r) or /proc/config.gz, they could submit
> a patch for it with links to documentation. How about that?

Ok, did a bit of research on my side as well, and I couldn't find a
solid reference to /proc/config either, so it seems you are correct.
Let's drop /proc/config for now. Thanks for investigating that!

> 
>>> -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.
> 
> I have changed it to "read_next_kernel_config_option", let me know if
> you prefer an alternative.
> 

Sounds good to me.

>>>  {
>>> -	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.
> 
> Does this refer to the type, name, or full line length? I did not find
> this in CodingStyle, the closest I could get is:
> https://lore.kernel.org/patchwork/patch/732076/
> 
> I will assume the line length for now.

I am unsure this is in the CodingStyle, but fairly certain that this is
a convention for at least network-related code. And yes, as I understand
it refers to the length of the line.

> 
>>>  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.
> 
> Even if that means moving lines? Something like this?
> 
>         char path[PATH_MAX];
>    +    bool is_pipe = false;
>    +    FILE *fd = NULL;
>         size_t i, n;
>         ssize_t ret;
>    -    FILE *fd;

Yes, that's the idea (although "is_pipe" should be at the top in that
case, above "path" -- and this applies to your v2 patch, by the way).

> 
>>>  	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?
> 
> Good idea, I'll try a bit harder if uname falls for whatever reason.

Thanks!

> 
>> 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?
> 
> I doubt it is actually in use, it looks like a typo in the original PR.
> This post only lists /proc/config.gz, /boot/config and
> /boot/config-$(uname -r): https://superuser.com/questions/287371
> 
>>> +	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?
> 
> scripts/kconfig/merge_config.sh seems to apply a merge strategy,
> overwriting earlier values and warning about it. However this should be
> rare given that it ended up at /proc/config.gz. For now I will favor
> simplicity over complexity and keep the old situation. Let me know if
> you prefer otherwise.

Understood, let's keep it that way for now.

Thanks,
Quentin