netdev - Re: [PATCHv6 bpf-next 1/3] xdp: add a new helper for dev map multicast support

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:   Fri, 10 Jul 2020 07:46:18 -0600
From:   David Ahern <dsahern@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Jiri Benc <jbenc@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>, ast@...nel.org,
        Daniel Borkmann <daniel@...earbox.net>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
Subject: Re: [PATCHv6 bpf-next 1/3] xdp: add a new helper for dev map
 multicast support

On 7/10/20 12:55 AM, Hangbin Liu wrote:
> Hi David,
> On Thu, Jul 09, 2020 at 10:33:38AM -0600, David Ahern wrote:
>>> +bool dev_in_exclude_map(struct bpf_dtab_netdev *obj, struct bpf_map *map,
>>> +			int exclude_ifindex)
>>> +{
>>> +	struct bpf_dtab_netdev *ex_obj = NULL;
>>> +	u32 key, next_key;
>>> +	int err;
>>> +
>>> +	if (obj->dev->ifindex == exclude_ifindex)
>>> +		return true;
>>> +
>>> +	if (!map)
>>> +		return false;
>>> +
>>> +	err = devmap_get_next_key(map, NULL, &key);
>>> +	if (err)
>>> +		return false;
>>> +
>>> +	for (;;) {
>>> +		switch (map->map_type) {
>>> +		case BPF_MAP_TYPE_DEVMAP:
>>> +			ex_obj = __dev_map_lookup_elem(map, key);
>>> +			break;
>>> +		case BPF_MAP_TYPE_DEVMAP_HASH:
>>> +			ex_obj = __dev_map_hash_lookup_elem(map, key);
>>> +			break;
>>> +		default:
>>> +			break;
>>> +		}
>>> +
>>> +		if (ex_obj && ex_obj->dev->ifindex == obj->dev->ifindex)
>>
>> I'm probably missing something fundamental, but why do you need to walk
>> the keys? Why not just do a lookup on the device index?
> 
> This functions is to check if the device index is in exclude map.
> 
> The device indexes are stored as values in the map. The user could store
> the values by any key number. There is no way to lookup the device index
> directly unless loop the map and check each values we stored.

Right.

The point of DEVMAP_HASH is to allow map management where key == device
index (vs DEVMAP which for any non-trivial use case is going to require
key != device index). You could require the exclude map to be
DEVMAP_HASH and the key to be the index allowing you to do a direct
lookup. Having to roam the entire map looking for a match does not scale
and is going to have poor performance with increasing number of entries.
XDP is targeted at performance with expert level of control, so
constraints like this have to be part of the deal.