Topics

[PATCH v3 bpf-next 1/3] BPF: helpers: New helper to obtain namespace data from current task

cnb
 

Hi,

Could you give me a hand with a couple of doubts?

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

---------- Forwarded message ---------
From: Carlos Antonio Neira Bustos <cneirabustos@...>
Date: Thu, Mar 21, 2019 at 7:40 AM
Subject: Re: [PATCH v3 bpf-next 1/3] BPF: helpers: New helper to obtain namespace data from current task
To: Alexei Starovoitov <alexei.starovoitov@...>
Cc: <netdev@...>, <ys114321@...>


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