| 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
| ||
|
Message-ID: <09ae3b1b-e79b-98c0-e1d1-31fcdab9df40@iogearbox.net>
Date: Wed, 27 Dec 2017 18:26:16 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Jakub Kicinski <jakub.kicinski@...ronome.com>,
netdev@...r.kernel.org, alexei.starovoitov@...il.com
Cc: ktkhai@...tuozzo.com, oss-drivers@...ronome.com,
"Eric W . Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH bpf-next v2 6/8] bpf: offload: report device information
for offloaded programs
On 12/21/2017 10:01 PM, Jakub Kicinski wrote:
> Report to the user ifindex and namespace information of offloaded
> programs. If device has disappeared return -ENODEV. Specify the
> namespace using dev/inode combination.
>
> CC: Eric W. Biederman <ebiederm@...ssion.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>
> ---
> v2:
> - take RTNL lock to grab a coherent snapshot of device state
> (ifindex vs name space) and avoid races with name space
> moves (based on Eric's comment on Kirill's patch to
> peernet2id_alloc()).
> ---
> fs/nsfs.c | 2 +-
> include/linux/bpf.h | 2 ++
> include/linux/proc_ns.h | 1 +
> include/uapi/linux/bpf.h | 3 +++
> kernel/bpf/offload.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c | 6 ++++++
> tools/include/uapi/linux/bpf.h | 3 +++
> 7 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nsfs.c b/fs/nsfs.c
> index 7c6f76d29f56..e50628675935 100644
> --- a/fs/nsfs.c
> +++ b/fs/nsfs.c
> @@ -51,7 +51,7 @@ static void nsfs_evict(struct inode *inode)
> ns->ops->put(ns);
> }
>
> -static void *__ns_get_path(struct path *path, struct ns_common *ns)
> +void *__ns_get_path(struct path *path, struct ns_common *ns)
> {
> struct vfsmount *mnt = nsfs_mnt;
> struct dentry *dentry;
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9a916ab34299..7810ae57b357 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -531,6 +531,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
>
> int bpf_prog_offload_compile(struct bpf_prog *prog);
> void bpf_prog_offload_destroy(struct bpf_prog *prog);
> +int bpf_prog_offload_info_fill(struct bpf_prog_info *info,
> + struct bpf_prog *prog);
>
> #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL)
> int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr);
> diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
> index 2ff18c9840a7..1733359cf713 100644
> --- a/include/linux/proc_ns.h
> +++ b/include/linux/proc_ns.h
> @@ -76,6 +76,7 @@ static inline int ns_alloc_inum(struct ns_common *ns)
>
> extern struct file *proc_ns_fget(int fd);
> #define get_proc_ns(inode) ((struct ns_common *)(inode)->i_private)
> +extern void *__ns_get_path(struct path *path, struct ns_common *ns);
> extern void *ns_get_path(struct path *path, struct task_struct *task,
> const struct proc_ns_operations *ns_ops);
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d01f1cb3cfc0..72b37fc3bc0c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -921,6 +921,9 @@ struct bpf_prog_info {
> __u32 nr_map_ids;
> __aligned_u64 map_ids;
> char name[BPF_OBJ_NAME_LEN];
> + __u32 ifindex;
> + __u64 netns_dev;
> + __u64 netns_ino;
> } __attribute__((aligned(8)));
>
> struct bpf_map_info {
> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 1e6064ea3609..4d50000bd1e3 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>
> #include <linux/rwsem.h>
>
> @@ -181,6 +183,48 @@ 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;
> + struct inode *ns_inode;
> + struct path ns_path;
> + int ifindex, err;
> + struct net *net;
> +
> +again:
> + rtnl_lock();
> + down_read(&bpf_devs_lock);
> +
> + offload = prog->aux->offload;
> + if (!offload) {
> + up_read(&bpf_devs_lock);
> + rtnl_unlock();
> + return -ENODEV;
> + }
> +
> + ifindex = offload->netdev->ifindex;
> + net = dev_net(offload->netdev);
> + get_net(net); /* __ns_get_path() drops the reference */
> +
> + up_read(&bpf_devs_lock);
> + rtnl_unlock();
> +
> + err = PTR_ERR_OR_ZERO(__ns_get_path(&ns_path, &net->ns));
> + if (err) {
> + if (err == -EAGAIN)
> + goto again;
> + return err;
> + }
> + ns_inode = ns_path.dentry->d_inode;
> +
> + info->ifindex = ifindex;
> + info->netns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
> + info->netns_ino = ns_inode->i_ino;
I think here, we're still missing a:
path_put(&ns_path);
Otherwise we're leaking that reference, no?
Looking at perf, it does similar fetch of dev/ino pair through
perf_fill_ns_link_info(). Only difference is that task / ns_ops
are used to lookup the actual ns from a task, which we already
have in our case (netns) through netdev.
I've been thinking, could we make this more generic and move the
bits into fs/nsfs.c by having the retry itself in ns_get_path_cb()?
What I mean is:
void *ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb,
void *private_data)
{
struct ns_common *ns;
void *ret;
again:
ns = ns_get_cb(private_data);
if (!ns)
return ERR_PTR(-ENOENT);
ret = __ns_get_path(path, ns);
if (IS_ERR(ret) && PTR_ERR(ret) == -EAGAIN)
goto again;
return ret;
}
And then in bpf_prog_offload_info_fill(), we can just move the fetching
and holding ref of net into the callback and do something similar to:
struct path ns_path;
struct inode *ns_inode;
struct cb_info = {
.offload = prog->aux->offload,
.info = info,
};
void *ret;
ret = ns_get_path_cb(&ns_path, bpf_prog_offload_get_cb, &cb_info);
if (!ret) {
ns_inode = ns_path.dentry->d_inode;
info->netns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
info->netns_ino = ns_inode->i_ino;
path_put(&ns_path);
}
And bpf_prog_offload_get_cb() would either return &net->ns in case
of valid offload and can fill in the ifindex into info since we hold
rtnl there, or otherwise return NULL, so we get -ENOENT back.
This would allow to keep __ns_get_path() (including retry) private and
have a more generic, reusable API similar to ns_get_path() that just
has a cb for getting ns_common.
(Rest of the series with feedback from Kirill included looks good to me.)
> + return 0;
> +}
> +
> const struct bpf_prog_ops bpf_offload_prog_ops = {
> };
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 7d9f5b0f0e49..20444fd678d0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1624,6 +1624,12 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> return -EFAULT;
> }
>
> + if (bpf_prog_is_dev_bound(prog->aux)) {
> + err = bpf_prog_offload_info_fill(&info, prog);
> + if (err)
> + return err;
> + }
> +
> done:
> if (copy_to_user(uinfo, &info, info_len) ||
> put_user(info_len, &uattr->info.info_len))
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index db1b0923a308..4e8c60acfa32 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -921,6 +921,9 @@ struct bpf_prog_info {
> __u32 nr_map_ids;
> __aligned_u64 map_ids;
> char name[BPF_OBJ_NAME_LEN];
> + __u32 ifindex;
> + __u64 netns_dev;
> + __u64 netns_ino;
> } __attribute__((aligned(8)));
>
> struct bpf_map_info {
>
Powered by blists - more mailing lists