Message-ID: <20190321144053.zgzb4pwnzarqdjcz@dev00>
Date:   Thu, 21 Mar 2019 11:40:55 -0300
From:   Carlos Antonio Neira Bustos <cneirabustos@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, ys114321@...il.com
Subject: Re: [PATCH v3 bpf-next 1/3] BPF: helpers: New helper to obtain
 namespace data from current task

On Wed, Mar 20, 2019 at 06:23:20PM -0700, Alexei Starovoitov wrote:
> On Wed, Mar 20, 2019 at 01:49:22PM -0300, Carlos Antonio Neira Bustos wrote:
> > 
> > This is a series of patches to introduce a new helper called bpf_get_current_pidns_info,
> > this change has been splitted into the following patches:
> > 
> > 1- Feature introduction
> > 2- Update tools/.../bpf.h
> > 3- Self tests and samples
> > 
> > 
> > From 852a65906122b05b4d1a23af868b2c245d240402 Mon Sep 17 00:00:00 2001
> > From: Carlos <cneirabustos@...il.com>
> > Date: Tue, 19 Mar 2019 19:38:48 -0300
> > Subject: [PATCH] [PATCH bpf-next 1/3] BPF: New helper to obtain namespace data
> >  from current task
> > 
> > This helper obtains the active namespace from current and returns pid, tgid,
> > device and namespace id as seen from that namespace, allowing to instrument
> > a process inside a container.
> > Device is read from /proc/self/ns/pid, as in the future it's possible that
> > different pid_ns files may belong to different devices, according
> > to the discussion between Eric Biederman and Yonghong in 2017 linux plumbers
> > conference.
> > Currently bpf_get_current_pid_tgid(), is used to do pid filtering in bcc's
> > scripts but this helper returns the pid as seen by the root namespace which is
> > fine when a bcc script is not executed inside a container.
> > When the process of interest is inside a container, pid filtering will not work
> > if bpf_get_current_pid_tgid() is used. This helper addresses this limitation
> > returning the pid as it's seen by the current namespace where the script is
> > executing.
> > 
> > This helper has the same use cases as bpf_get_current_pid_tgid() as it can be
> > used to do pid filtering even inside a container.
> > 
> > For example a bcc script using bpf_get_current_pid_tgid() (tools/funccount.py):
> > 
> >         u32 pid = bpf_get_current_pid_tgid() >> 32;
> >         if (pid != <pid_arg_passed_in>)
> >                 return 0;
> > Could be modified to use bpf_get_current_pidns_info() as follows:
> > 
> >         struct bpf_pidns pidns;
> >         bpf_get_current_pidns_info(&pidns, sizeof(struct bpf_pidns));
> >         u32 pid = pidns.tgid;
> >         u32 nsid = pidns.nsid;
> >         if ((pid != <pid_arg_passed_in>) && (nsid != <nsid_arg_passed_in>))
> >                 return 0;
> > 
> > To find out the name PID namespace id of a process, you could use this command:
> > 
> > $ ps -h -o pidns -p <pid_of_interest>
> > 
> > Or this other command:
> > 
> > $ ls -Li /proc/<pid_of_interest>/ns/pid
> > 
> > Signed-off-by: Carlos Antonio Neira Bustos <cneirabustos@...il.com>
> > -
> > ---
> >  include/linux/bpf.h      |  1 +
> >  include/uapi/linux/bpf.h | 26 ++++++++++++++++++-
> >  kernel/bpf/core.c        |  1 +
> >  kernel/bpf/helpers.c     | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  kernel/trace/bpf_trace.c |  2 ++
> >  5 files changed, 96 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a2132e09dc1c..a77f5bd77bd8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -930,6 +930,7 @@ extern const struct bpf_func_proto bpf_sk_redirect_map_proto;
> >  extern const struct bpf_func_proto bpf_spin_lock_proto;
> >  extern const struct bpf_func_proto bpf_spin_unlock_proto;
> >  extern const struct bpf_func_proto bpf_get_local_storage_proto;
> > +extern const struct bpf_func_proto bpf_get_current_pidns_info_proto;
> >  
> >  /* Shared helpers among cBPF and eBPF. */
> >  void bpf_user_rnd_init_once(void);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 3c38ac9a92a7..facc701c7873 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2366,6 +2366,18 @@ union bpf_attr {
> >   *             current value is ect (ECN capable). Works with IPv6 and IPv4.
> >   *     Return
> >   *             1 if set, 0 if not set.
> > + *
> > + * int bpf_get_current_pidns_info(struct bpf_pidns_info *pidns, u32 size_of_pidns)
> > + *	Description
> > + *		Copies into *pidns* pid, namespace id and tgid as seen by the
> > + *		current namespace and also device from /proc/self/ns/pid.
> > + *		*size_of_pidns* must be the size of *pidns*
> > + *
> > + *		This helper is used when pid filtering is needed inside a
> > + *		container as bpf_get_current_tgid() helper returns always the
> > + *		pid id as seen by the root namespace.
> > + *	Return
> > + *		0 on success -EINVAL on error.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)		\
> >  	FN(unspec),			\
> > @@ -2465,7 +2477,8 @@ union bpf_attr {
> >  	FN(spin_unlock),		\
> >  	FN(sk_fullsock),		\
> >  	FN(tcp_sock),			\
> > -	FN(skb_ecn_set_ce),
> > +	FN(skb_ecn_set_ce),		\
> > +	FN(get_current_pidns_info),
> >  
> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >   * function eBPF program intends to call
> > @@ -3152,4 +3165,15 @@ struct bpf_line_info {
> >  struct bpf_spin_lock {
> >  	__u32	val;
> >  };
> > +
> > +/* helper bpf_get_current_pidns_info will store the following
> > + * data, dev will contain major/minor from /proc/self/pid.
> > +*/
> > +struct bpf_pidns_info {
> > +	__u32 dev;
> > +	__u32 nsid;
> > +	__u32 tgid;
> > +	__u32 pid;
> > +};
> > +
> >  #endif /* _UAPI__LINUX_BPF_H__ */
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 3f08c257858e..06329fbed95f 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -2044,6 +2044,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
> >  const struct bpf_func_proto bpf_get_current_comm_proto __weak;
> >  const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
> >  const struct bpf_func_proto bpf_get_local_storage_proto __weak;
> > +const struct bpf_func_proto bpf_get_current_pidns_info __weak;
> >  
> >  const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
> >  {
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index a411fc17d265..95c3780a6ba7 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -18,6 +18,11 @@
> >  #include <linux/sched.h>
> >  #include <linux/uidgid.h>
> >  #include <linux/filter.h>
> > +#include <linux/pid_namespace.h>
> > +#include <linux/major.h>
> > +#include <linux/stat.h>
> > +#include <linux/namei.h>
> > +#include <linux/version.h>
> >  
> >  /* If kernel subsystem is allowing eBPF programs to call this function,
> >   * inside its own verifier_ops->get_func_proto() callback it should return
> > @@ -364,3 +369,65 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
> >  };
> >  #endif
> >  #endif
> > +
> > +BPF_CALL_2(bpf_get_current_pidns_info, struct bpf_pidns_info *, pidns_info, u32,
> > +	 size)
> > +{
> > +	const char *nspid = "/proc/self/ns/pid";
> > +	struct pid_namespace *pidns = NULL;
> > +	struct kstat ks;
> > +	struct path kp;
> > +	pid_t tgid = 0;
> > +	pid_t pid = 0;
> > +	int res = 0;
> > +
> > +	if (unlikely(size != sizeof(struct bpf_pidns_info)))
> > +		goto clear;
> > +
> > +	pidns = task_active_pid_ns(current);
> > +
> > +	if (unlikely(!pidns))
> > +		goto clear;
> > +
> > +	pidns_info->nsid =  pidns->ns.inum;
> > +	pid = task_pid_nr_ns(current, pidns);
> > +
> > +	if (unlikely(!pid))
> > +		goto clear;
> > +
> > +	tgid = task_tgid_nr_ns(current, pidns);
> > +
> > +	if (unlikely(!tgid))
> > +		goto clear;
> > +
> > +	pidns_info->tgid = (u32) tgid;
> > +	pidns_info->pid = (u32) pid;
> > +
> > +        kern_path(nspid, 0, &kp);
> > +
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,11,0)
> > +    res = vfs_getattr(&kp, &ks, STATX_ALL, 0);
> > +#else
> > +    res = vfs_getattr(&kp, &ks);
> > +#endif
> 
> Please access namespaces directly.
> I suspect bpf helpers cannot do vfs_getattr. Something inside might sleep
> while bpf progs are non preemptible.
> 
> Also please run ./scripts/checkpatch.pl on your patches
> and read Documentation/bpf/bpf_devel_QA.rst.
> 
Hello Alexei, 

Thanks for checking this out and also thanks for the documentation link.
At this point I have a couple of questions:

* What is the reason to not use the current namespace api instead of directly
  accessing namespaces?.

* Regarding bpf programs not being preemptible. If we add spin_locks to the
  vfs_getattr call, would that be an acceptable solution?

   spin_lock(&bpf_lock);
   res = vfs_getattr(&kp, &ks);
   spin_unlock(&bpf_lock);

Is there another way to interact with the vfs layer within a bpf helper?.

Bests

Powered by blists - more mailing lists