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.kicin...@netronome.com>
> Reviewed-by: Simon Horman <simon.hor...@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.mon...@netronome.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
