netdev - Re: [PATCH bpf-next 6/9] bpf: offload: allow program and map sharing per-ASIC

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]

Message-ID: <20180717030046.4ylnjs3lkdar4uld@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 16 Jul 2018 20:00:47 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     daniel@...earbox.net, oss-drivers@...ronome.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 6/9] bpf: offload: allow program and map sharing
 per-ASIC

On Mon, Jul 16, 2018 at 07:37:20PM -0700, Jakub Kicinski wrote:
> Create a higher-level entity to represent a device/ASIC to allow
> programs and maps to be shared between device ports.  The extra
> work is required to make sure we don't destroy BPF objects as
> soon as the netdev for which they were loaded gets destroyed,
> as other ports may still be using them.  When netdev goes away
> all of its BPF objects will be moved to other netdevs of the
> device, and only destroyed when last netdev is unregistered.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>
..
> -bool bpf_offload_dev_match(struct bpf_prog *prog, struct bpf_map *map)
> +static bool __bpf_offload_dev_match(struct bpf_prog *prog,
> +				    struct net_device *netdev)
>  {
> -	struct bpf_offloaded_map *offmap;
> +	struct bpf_offload_netdev *ondev1, *ondev2;
>  	struct bpf_prog_offload *offload;
> -	bool ret;
>  
>  	if (!bpf_prog_is_dev_bound(prog->aux))
>  		return false;
> -	if (!bpf_map_is_dev_bound(map))
> -		return bpf_map_offload_neutral(map);
>  
> -	down_read(&bpf_devs_lock);
>  	offload = prog->aux->offload;
> +	if (!offload)
> +		return false;
> +	if (offload->netdev == netdev)
> +		return true;
> +
> +	ondev1 = bpf_offload_find_netdev(offload->netdev);
> +	ondev2 = bpf_offload_find_netdev(netdev);
> +
> +	return ondev1 && ondev2 && ondev1->offdev == ondev2->offdev;
> +}
> +
> +bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev)
> +{
> +	bool ret;
> +
> +	down_read(&bpf_devs_lock);
> +	ret = __bpf_offload_dev_match(prog, netdev);
> +	up_read(&bpf_devs_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(bpf_offload_dev_match);
> +
> +bool bpf_offload_match(struct bpf_prog *prog, struct bpf_map *map)
> +{
> +	struct bpf_offloaded_map *offmap;
> +	bool ret;
> +
> +	if (!bpf_map_is_dev_bound(map))
> +		return bpf_map_offload_neutral(map);
>  	offmap = map_to_offmap(map);
>  
> -	ret = offload && offload->netdev == offmap->netdev;
> +	down_read(&bpf_devs_lock);
> +	ret = __bpf_offload_dev_match(prog, offmap->netdev);
>  	up_read(&bpf_devs_lock);
>  
>  	return ret;
>  }
>  
..
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9e2bf834f13a..2c5b923eef75 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5054,7 +5054,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>  	}
>  
>  	if ((bpf_prog_is_dev_bound(prog->aux) || bpf_map_is_dev_bound(map)) &&
> -	    !bpf_offload_dev_match(prog, map)) {
> +	    !bpf_offload_match(prog, map)) {

I'm confused with new names and renaming.
May be split renaming into separate patch?
Should new bpf_offload_match() be called bpf_offload_prog_map_match() ?
or some other name?
May be adding comments to these functions will make it clear...