Re: [PATCH bpf-next v2 6/8] bpf: offload: report device information for offloaded programs

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 27 Dec 2017 18:26:16 +0100, Daniel Borkmann wrote:
> 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@xmission.com>
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.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?

You're right.  Thanks for the detailed suggestion, let me try to meld
it into this series.

> 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.)