netdev - Re: [iproute2 v3] Read configuration files from /etc and /usr

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] [day] [month] [year] [list]
Date: Wed, 26 Jul 2023 08:11:10 +0200
From: Gioele Barabucci <gioele@...rio.it>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org
Subject: Re: [iproute2 v3] Read configuration files from /etc and /usr

Thanks Stephen for the review, I'll soon send a v4 with the fixes 
discussed below.

On 25/07/23 20:01, Stephen Hemminger wrote:
> On Tue, 25 Jul 2023 16:27:59 +0200
> Gioele Barabucci <gioele@...rio.it> wrote:
> 
>> @@ -2924,7 +2926,9 @@ static int bpf_elf_ctx_init(struct bpf_elf_ctx *ctx, const char *pathname,
>>   	}
>>   
>>   	bpf_save_finfo(ctx);
>> -	bpf_hash_init(ctx, CONFDIR "/bpf_pinning");
>> +	ret = bpf_hash_init(ctx, CONF_USR_DIR "/bpf_pinning");
>> +	if (ret == -ENOENT)
>> +		bpf_hash_init(ctx, CONF_ETC_DIR "/bpf_pinning");
>>   
> 
> Luca spotted this one, other places prefer /etc over /use but in BPF it is swapped?

Yeah, that's a mistake. Thanks for spotting it. Fixed.

>> -static void
>> +static int
>>   rtnl_hash_initialize(const char *file, struct rtnl_hash_entry **hash, int size)
>>   {
>>   	struct rtnl_hash_entry *entry;
>> @@ -67,14 +69,14 @@ rtnl_hash_initialize(const char *file, struct rtnl_hash_entry **hash, int size)
>>   
>>   	fp = fopen(file, "r");
>>   	if (!fp)
>> -		return;
>> +		return -errno;
>>   
>>   	while ((ret = fread_id_name(fp, &id, &namebuf[0]))) {
>>   		if (ret == -1) {
>>   			fprintf(stderr, "Database %s is corrupted at %s\n",
>>   					file, namebuf);
>>   			fclose(fp);
>> -			return;
>> +			return -1;
> 
> Having some errors return -errno and others return -1 is confusing.
> Perhaps -EINVAL?

The code was just relaying the -1 received from fread_id_name, but it 
makes sense to use -EINVAL instead. Changed.


>> +		/* only consider filenames not present in /etc */
>> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
>> +		if (lstat(path, &sb) == 0)
>> +			continue;
> 
> Do you want lstat() it will return 0 it is a symlink to non existent target.

Yes, lstat is the right function IMO. Other programs that follow the 
stateless pattern also use lstat. By making a symlink, the sysadmin 
stated their intention to override a setting. The code should fail 
reading the target rather than silently skipping the override.


>> +		/* load the conf file in /etc */
>> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
>> +		if (is_tab)
>> +			rtnl_tab_initialize(path, (char**)tabhash, size);
>> +		else
>> +			rtnl_hash_initialize(path, (struct rtnl_hash_entry**)tabhash, size);
> 
> I would prefer that generic function not loose all type information.
> Maybe using a union for the two possible usages? Or type specific callback instead of is_tab.

Makes sense. Changed to use a enum/union.


 >> +	/* load /usr/lib/iproute2/foo.d/X conf files, unless 
/etc/iproute2/foo.d/X exists */
 >> +	d = opendir(dirpath_usr);
 >> +	while (d && (de = readdir(d)) != NULL) {
 >> +		char path[PATH_MAX];
 >> +		size_t len;
 >> +		struct stat sb;
 >> +
 >> +		if (*de->d_name == '.')
 >> +			continue;
 >> +
 >> +		/* only consider filenames ending in '.conf' */
 >> +		len = strlen(de->d_name);
 >> +		if (len <= 5)
 >> +			continue;
 >> +		if (strcmp(de->d_name + len - 5, ".conf"))
 >> +			continue;
 >> +
 >> +		/* only consider filenames not present in /etc */
 >> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
 >> +		if (lstat(path, &sb) == 0)
 >> +			continue;
 >> +
 >> +		/* load the conf file in /usr */
 >> +		snprintf(path, sizeof(path), "%s/%s", dirpath_usr, de->d_name);
 >> +		if (is_tab)
 >> +			rtnl_tab_initialize(path, (char**)tabhash, size);
 >> +		else
 >> +			rtnl_hash_initialize(path, (struct rtnl_hash_entry**)tabhash, size);
 >> +	}
 >> +	if (d)
 >> +		closedir(d);
 >> +
 >> +	/* load /etc/iproute2/foo.d/X conf files */
 >> +	d = opendir(dirpath_etc);
 >> +	while (d && (de = readdir(d)) != NULL) {
 >> +		char path[PATH_MAX];
 >> +		size_t len;
 >> +
 >> +		if (*de->d_name == '.')
 >> +			continue;
 >> +
 >> +		/* only consider filenames ending in '.conf' */
 >> +		len = strlen(de->d_name);
 >> +		if (len <= 5)
 >> +			continue;
 >> +		if (strcmp(de->d_name + len - 5, ".conf"))
 >> +			continue;
 >> +
 >> +		/* load the conf file in /etc */
 >> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
 >> +		if (is_tab)
 >> +			rtnl_tab_initialize(path, (char**)tabhash, size);
 >> +		else
 >> +			rtnl_hash_initialize(path, (struct rtnl_hash_entry**)tabhash, size);
 >> +	}
 >> +	if (d)
 >> +		closedir(d);
 >
 > Why is code copy/pasted here instead of making a function?

Because it deals with different paths in slightly different ways, has 
different comments, and is only a few lines long, so an extra level of 
indirection may be excessive.

However I see your point and I have extracted the code into a separate 
function in v4.

 > Seems sloppy to me.

BTW, this comment seems a bit unkind, given that the current iproute2 
code carries multiple exact copies of the .d lookup code and this patch 
_reduces_ the amount of duplication.

Regards,

-- 
Gioele Barabucci