linux-kernel - Re: [RFC] bpf: offload: report device information for offloaded programs

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:   Thu, 30 Nov 2017 16:19:13 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, oss-drivers@...ronome.com,
        alexei.starovoitov@...il.com, daniel@...earbox.net,
        "Eric W . Biederman" <ebiederm@...ssion.com>
Subject: Re: [RFC] bpf: offload: report device information for offloaded
 programs

Hi, Jakub,

please, read comments below.

On 30.11.2017 03:22, Jakub Kicinski wrote:
> Report to the user ifindex and namespace information of offloaded
> programs.  Always set dev_bound to true if program was loaded for
> a device which has been since removed.  Specify the namespace
> using dev/inode combination.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Simon Horman <simon.horman@...ronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>
> ---
>  fs/nsfs.c                      |  2 +-
>  include/linux/bpf.h            |  2 ++
>  include/linux/proc_ns.h        |  1 +
>  include/uapi/linux/bpf.h       |  5 +++++
>  kernel/bpf/offload.c           | 34 ++++++++++++++++++++++++++++++++++
>  kernel/bpf/syscall.c           |  6 ++++++
>  tools/include/uapi/linux/bpf.h |  5 +++++
>  7 files changed, 54 insertions(+), 1 deletion(-)

[snip]

> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 8455b89d1bbf..da98349c647d 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -16,9 +16,11 @@
>  #include <linux/bpf.h>
>  #include <linux/bpf_verifier.h>
>  #include <linux/bug.h>
> +#include <linux/kdev_t.h>
>  #include <linux/list.h>
>  #include <linux/netdevice.h>
>  #include <linux/printk.h>
> +#include <linux/proc_ns.h>
>  #include <linux/rtnetlink.h>
>  
>  /* protected by RTNL */
> @@ -164,6 +166,38 @@ int bpf_prog_offload_compile(struct bpf_prog *prog)
>  	return bpf_prog_offload_translate(prog);
>  }
>  
> +int bpf_prog_offload_info_fill(struct bpf_prog_info *info,
> +			       struct bpf_prog *prog)
> +{
> +	struct bpf_dev_offload *offload = prog->aux->offload;
> +	struct inode *ns_inode;
> +	struct path ns_path;
> +	struct net *net;
> +	int ret = 0;
> +	void *ptr;
> +
> +	info->dev_bound = 1;
> +
> +	rtnl_lock();

rtnl_lock() is too big lock and it is already overused in kernel.
Can't we use smaller lock in this driver to protect bpf_prog_offload_devs?
I suppose rwlock would be appropriate for that.

(Then, we may completely remove rtnl_lock() from bpf_prog_offload_init()
and use readlocked dev_base_lock for __dev_get_by_index() instead and
the new small_rwlock to link in the list.

Not sure about bpf_prog_offload_verifier_prep() and bpf_prog_offload_translate()
and which context expect net_device_ops->ndo_bpf users. Either they need rtnl
or not).

Then the below hunk:

> +	if (!offload->netdev)
> +		goto out;
> +
> +	net = dev_net(offload->netdev);
> +	get_net(net); /* __ns_get_path() drops the reference */

will be:

	read_lock(&small_rwlock);
	if (!offload->netdev)
		goto out;

	net = dev_net(offload->netdev);
	get_net(net); /* __ns_get_path() drops the reference */
	read_unlock(&small_rwlock);

and rtnl_lock() won't be touched.

> +	ptr = __ns_get_path(&ns_path, &net->ns);
> +	ret = PTR_ERR_OR_ZERO(ptr);
> +	if (ret)
> +		goto out;
> +	ns_inode = ns_path.dentry->d_inode;
> +
> +	info->ns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
> +	info->ns_inode = ns_inode->i_ino;
> +	info->ifindex = offload->netdev->ifindex;
> +out:
> +	rtnl_unlock();
> +	return ret;
> +}
> +

[snip]

Kirill