[bpf-next,v4,1/4] bpf: sock ops: add netns ino and dev in bpf context

From: Alban Crequy <alban@kinvolk.io>

sockops programs can now access the network namespace inode and device
via (struct bpf_sock_ops)->netns_ino and ->netns_dev. This can be useful
to apply different policies on different network namespaces.

In the unlikely case where network namespaces are not compiled in
(CONFIG_NET_NS=n), the verifier will return netns_dev as usual and will
return 0 for netns_ino.

The generated BPF bytecode for netns_ino is loading the correct inode
number at the time of execution.

However, the generated BPF bytecode for netns_dev is loading an
immediate value determined at BPF-load-time by looking at the initial
network namespace. In practice, this works because all netns currently
use the same virtual device. If this was to change, this code would need
to be updated too.

Co-authored-by: Iago López Galeiras <iago@kinvolk.io>
Signed-off-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Iago López Galeiras <iago@kinvolk.io>

---

Changes since v1:
- add netns_dev (review from Alexei)

Changes since v2:
- replace __u64 by u64 in kernel code (review from Y Song)
- remove unneeded #else branch: program would be rejected in
  is_valid_access (review from Y Song)
- allow partial reads (<u64) (review from Y Song)

Changes since v3:
- return netns_dev unconditionally and set netns_ino to 0 if
  CONFIG_NET_NS is not enabled (review from Jakub Kicinski)
- use bpf_ctx_record_field_size and bpf_ctx_narrow_access_ok instead of
  manually deal with partial reads (review from Y Song)
- update commit message to reflect new code and remove note about
  partial reads since it was discussed in the review
- use bpf_ctx_range() and offsetofend()
---
 include/uapi/linux/bpf.h |  2 ++
 net/core/filter.c        | 70 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

On Fri, May 24, 2019 at 9:01 AM Iago López Galeiras <iago@kinvolk.io> wrote:
>
> From: Alban Crequy <alban@kinvolk.io>
>
> sockops programs can now access the network namespace inode and device
> via (struct bpf_sock_ops)->netns_ino and ->netns_dev. This can be useful
> to apply different policies on different network namespaces.
>
> In the unlikely case where network namespaces are not compiled in
> (CONFIG_NET_NS=n), the verifier will return netns_dev as usual and will
> return 0 for netns_ino.
>
> The generated BPF bytecode for netns_ino is loading the correct inode
> number at the time of execution.
>
> However, the generated BPF bytecode for netns_dev is loading an
> immediate value determined at BPF-load-time by looking at the initial
> network namespace. In practice, this works because all netns currently
> use the same virtual device. If this was to change, this code would need
> to be updated too.
>
> Co-authored-by: Iago López Galeiras <iago@kinvolk.io>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
> Signed-off-by: Iago López Galeiras <iago@kinvolk.io>
>
> ---
>
> Changes since v1:
> - add netns_dev (review from Alexei)
>
> Changes since v2:
> - replace __u64 by u64 in kernel code (review from Y Song)
> - remove unneeded #else branch: program would be rejected in
>   is_valid_access (review from Y Song)
> - allow partial reads (<u64) (review from Y Song)
>
> Changes since v3:
> - return netns_dev unconditionally and set netns_ino to 0 if
>   CONFIG_NET_NS is not enabled (review from Jakub Kicinski)
> - use bpf_ctx_record_field_size and bpf_ctx_narrow_access_ok instead of
>   manually deal with partial reads (review from Y Song)
> - update commit message to reflect new code and remove note about
>   partial reads since it was discussed in the review
> - use bpf_ctx_range() and offsetofend()
> ---
>  include/uapi/linux/bpf.h |  2 ++
>  net/core/filter.c        | 70 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 63e0cf66f01a..e64066a09a5f 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3261,6 +3261,8 @@ struct bpf_sock_ops {
>         __u32 sk_txhash;
>         __u64 bytes_received;
>         __u64 bytes_acked;
> +       __u64 netns_dev;
> +       __u64 netns_ino;
>  };
>
>  /* Definitions for bpf_sock_ops_cb_flags */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 55bfc941d17a..2b1552a8dd74 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -76,6 +76,8 @@
>  #include <net/lwtunnel.h>
>  #include <net/ipv6_stubs.h>
>  #include <net/bpf_sk_storage.h>
> +#include <linux/kdev_t.h>
> +#include <linux/proc_ns.h>
>
>  /**
>   *     sk_filter_trim_cap - run a packet through a socket filter
> @@ -6822,6 +6824,18 @@ static bool sock_ops_is_valid_access(int off, int size,
>                 }
>         } else {
>                 switch (off) {
> +               case bpf_ctx_range(struct bpf_sock_ops, netns_dev):
> +                       if (off >= offsetofend(struct bpf_sock_ops, netns_dev))
> +                               return false;

This is not needed.
#define bpf_ctx_range(TYPE, MEMBER)
         \
        offsetof(TYPE, MEMBER) ... offsetofend(TYPE, MEMBER) - 1
off should never be >= offsetofend(struct bpf_sock_ops, netns_dev).

> +
> +                       bpf_ctx_record_field_size(info, sizeof(u64));
> +                       if (!bpf_ctx_narrow_access_ok(off, size, sizeof(u64)))
> +                               return false;
> +                       break;
> +               case offsetof(struct bpf_sock_ops, netns_ino):
> +                       if (size != sizeof(u64))
> +                               return false;
> +                       break;
>                 case bpf_ctx_range_till(struct bpf_sock_ops, bytes_received,
>                                         bytes_acked):
>                         if (size != sizeof(__u64))
> @@ -7739,6 +7753,11 @@ static u32 sock_addr_convert_ctx_access(enum bpf_access_type type,
>         return insn - insn_buf;
>  }
>
> +static struct ns_common *sockops_netns_cb(void *private_data)
> +{
> +       return &init_net.ns;
> +}
> +
>  static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
>                                        const struct bpf_insn *si,
>                                        struct bpf_insn *insn_buf,
> @@ -7747,6 +7766,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
>  {
>         struct bpf_insn *insn = insn_buf;
>         int off;
> +       struct inode *ns_inode;
> +       struct path ns_path;
> +       u64 netns_dev;
> +       void *res;
>
>  /* Helper macro for adding read access to tcp_sock or sock fields. */
>  #define SOCK_OPS_GET_FIELD(BPF_FIELD, OBJ_FIELD, OBJ)                        \
> @@ -7993,6 +8016,53 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
>                 SOCK_OPS_GET_OR_SET_FIELD(sk_txhash, sk_txhash,
>                                           struct sock, type);
>                 break;
> +
> +       case bpf_ctx_range(struct bpf_sock_ops, netns_dev):
> +               /* We get the netns_dev at BPF-load-time and not at
> +                * BPF-exec-time. We assume that netns_dev is a constant.
> +                */
> +               res = ns_get_path_cb(&ns_path, sockops_netns_cb, NULL);
> +               if (IS_ERR(res)) {
> +                       netns_dev = 0;

What is the proper way to handle error here?
If we indeed get an error and netns_dev = 0, do this mean bpf program
should check netns_dev == 0 and special case it? Or since this is really
a lower probability thing we can set to 0 and bpf program's logic does not
need to specialize this one.

At least, maybe we need a little documentation for the field in uapi header
to point out this potential caveat?

> +               } else {
> +                       ns_inode = ns_path.dentry->d_inode;
> +                       netns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
> +               }
> +               *target_size = 8;
> +               *insn++ = BPF_MOV64_IMM(si->dst_reg, netns_dev);
> +               break;
> +
> +       case offsetof(struct bpf_sock_ops, netns_ino):
> +#ifdef CONFIG_NET_NS
> +               /* Loading: sk_ops->sk->__sk_common.skc_net.net->ns.inum
> +                * Type: (struct bpf_sock_ops_kern *)
> +                *       ->(struct sock *)
> +                *       ->(struct sock_common)
> +                *       .possible_net_t
> +                *       .(struct net *)
> +                *       ->(struct ns_common)
> +                *       .(unsigned int)
> +                */
> +               BUILD_BUG_ON(offsetof(struct sock, __sk_common) != 0);
> +               BUILD_BUG_ON(offsetof(possible_net_t, net) != 0);
> +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> +                                               struct bpf_sock_ops_kern, sk),
> +                                     si->dst_reg, si->src_reg,
> +                                     offsetof(struct bpf_sock_ops_kern, sk));
> +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> +                                               possible_net_t, net),
> +                                     si->dst_reg, si->dst_reg,
> +                                     offsetof(struct sock_common, skc_net));
> +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> +                                               struct ns_common, inum),
> +                                     si->dst_reg, si->dst_reg,
> +                                     offsetof(struct net, ns) +
> +                                     offsetof(struct ns_common, inum));
> +#else
> +               *insn++ = BPF_MOV64_IMM(si->dst_reg, 0);
> +#endif
> +               break;
> +
>         }
>         return insn - insn_buf;
>  }
> --
> 2.21.0
>

On Fri, May 24, 2019 at 7:48 PM Y Song <ys114321@gmail.com> wrote:
>
> On Fri, May 24, 2019 at 9:01 AM Iago López Galeiras <iago@kinvolk.io> wrote:
> >
> > From: Alban Crequy <alban@kinvolk.io>
> >
> > sockops programs can now access the network namespace inode and device
> > via (struct bpf_sock_ops)->netns_ino and ->netns_dev. This can be useful
> > to apply different policies on different network namespaces.
> >
> > In the unlikely case where network namespaces are not compiled in
> > (CONFIG_NET_NS=n), the verifier will return netns_dev as usual and will
> > return 0 for netns_ino.
> >
> > The generated BPF bytecode for netns_ino is loading the correct inode
> > number at the time of execution.
> >
> > However, the generated BPF bytecode for netns_dev is loading an
> > immediate value determined at BPF-load-time by looking at the initial
> > network namespace. In practice, this works because all netns currently
> > use the same virtual device. If this was to change, this code would need
> > to be updated too.
> >
> > Co-authored-by: Iago López Galeiras <iago@kinvolk.io>
> > Signed-off-by: Alban Crequy <alban@kinvolk.io>
> > Signed-off-by: Iago López Galeiras <iago@kinvolk.io>
> >
> > ---
> >
> > Changes since v1:
> > - add netns_dev (review from Alexei)
> >
> > Changes since v2:
> > - replace __u64 by u64 in kernel code (review from Y Song)
> > - remove unneeded #else branch: program would be rejected in
> >   is_valid_access (review from Y Song)
> > - allow partial reads (<u64) (review from Y Song)
> >
> > Changes since v3:
> > - return netns_dev unconditionally and set netns_ino to 0 if
> >   CONFIG_NET_NS is not enabled (review from Jakub Kicinski)
> > - use bpf_ctx_record_field_size and bpf_ctx_narrow_access_ok instead of
> >   manually deal with partial reads (review from Y Song)
> > - update commit message to reflect new code and remove note about
> >   partial reads since it was discussed in the review
> > - use bpf_ctx_range() and offsetofend()
> > ---
> >  include/uapi/linux/bpf.h |  2 ++
> >  net/core/filter.c        | 70 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 63e0cf66f01a..e64066a09a5f 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3261,6 +3261,8 @@ struct bpf_sock_ops {
> >         __u32 sk_txhash;
> >         __u64 bytes_received;
> >         __u64 bytes_acked;
> > +       __u64 netns_dev;
> > +       __u64 netns_ino;
> >  };
> >
> >  /* Definitions for bpf_sock_ops_cb_flags */
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 55bfc941d17a..2b1552a8dd74 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -76,6 +76,8 @@
> >  #include <net/lwtunnel.h>
> >  #include <net/ipv6_stubs.h>
> >  #include <net/bpf_sk_storage.h>
> > +#include <linux/kdev_t.h>
> > +#include <linux/proc_ns.h>
> >
> >  /**
> >   *     sk_filter_trim_cap - run a packet through a socket filter
> > @@ -6822,6 +6824,18 @@ static bool sock_ops_is_valid_access(int off, int size,
> >                 }
> >         } else {
> >                 switch (off) {
> > +               case bpf_ctx_range(struct bpf_sock_ops, netns_dev):
> > +                       if (off >= offsetofend(struct bpf_sock_ops, netns_dev))
> > +                               return false;
>
> This is not needed.
> #define bpf_ctx_range(TYPE, MEMBER)
>          \
>         offsetof(TYPE, MEMBER) ... offsetofend(TYPE, MEMBER) - 1
> off should never be >= offsetofend(struct bpf_sock_ops, netns_dev).
>

Right, I'll remove it.

> > +
> > +                       bpf_ctx_record_field_size(info, sizeof(u64));
> > +                       if (!bpf_ctx_narrow_access_ok(off, size, sizeof(u64)))
> > +                               return false;
> > +                       break;
> > +               case offsetof(struct bpf_sock_ops, netns_ino):
> > +                       if (size != sizeof(u64))
> > +                               return false;
> > +                       break;
> >                 case bpf_ctx_range_till(struct bpf_sock_ops, bytes_received,
> >                                         bytes_acked):
> >                         if (size != sizeof(__u64))
> > @@ -7739,6 +7753,11 @@ static u32 sock_addr_convert_ctx_access(enum bpf_access_type type,
> >         return insn - insn_buf;
> >  }
> >
> > +static struct ns_common *sockops_netns_cb(void *private_data)
> > +{
> > +       return &init_net.ns;
> > +}
> > +
> >  static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> >                                        const struct bpf_insn *si,
> >                                        struct bpf_insn *insn_buf,
> > @@ -7747,6 +7766,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> >  {
> >         struct bpf_insn *insn = insn_buf;
> >         int off;
> > +       struct inode *ns_inode;
> > +       struct path ns_path;
> > +       u64 netns_dev;
> > +       void *res;
> >
> >  /* Helper macro for adding read access to tcp_sock or sock fields. */
> >  #define SOCK_OPS_GET_FIELD(BPF_FIELD, OBJ_FIELD, OBJ)                        \
> > @@ -7993,6 +8016,53 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> >                 SOCK_OPS_GET_OR_SET_FIELD(sk_txhash, sk_txhash,
> >                                           struct sock, type);
> >                 break;
> > +
> > +       case bpf_ctx_range(struct bpf_sock_ops, netns_dev):
> > +               /* We get the netns_dev at BPF-load-time and not at
> > +                * BPF-exec-time. We assume that netns_dev is a constant.
> > +                */
> > +               res = ns_get_path_cb(&ns_path, sockops_netns_cb, NULL);
> > +               if (IS_ERR(res)) {
> > +                       netns_dev = 0;
>
> What is the proper way to handle error here?
> If we indeed get an error and netns_dev = 0, do this mean bpf program
> should check netns_dev == 0 and special case it? Or since this is really
> a lower probability thing we can set to 0 and bpf program's logic does not
> need to specialize this one.
>
> At least, maybe we need a little documentation for the field in uapi header
> to point out this potential caveat?
>

As far as I can tell, this function can only error with ENOMEM when allocating
a new inode or dentry, which is very unlikely. I don't think bpf programs
should check for this. Also, for sockops programs worst case is usually that
the connection goes through the usual networking stack so it shouldn't be
dangerous.

I can add a comment like this to the field in the uapi header file:

    ...
    /*
     * netns_dev might be zero if there's an error getting it
     * when loading the BPF program. This is very unlikely.
     */
    __u64 netns_dev;
    ...

What do you think?




> > +               } else {
> > +                       ns_inode = ns_path.dentry->d_inode;
> > +                       netns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
> > +               }
> > +               *target_size = 8;
> > +               *insn++ = BPF_MOV64_IMM(si->dst_reg, netns_dev);
> > +               break;
> > +
> > +       case offsetof(struct bpf_sock_ops, netns_ino):
> > +#ifdef CONFIG_NET_NS
> > +               /* Loading: sk_ops->sk->__sk_common.skc_net.net->ns.inum
> > +                * Type: (struct bpf_sock_ops_kern *)
> > +                *       ->(struct sock *)
> > +                *       ->(struct sock_common)
> > +                *       .possible_net_t
> > +                *       .(struct net *)
> > +                *       ->(struct ns_common)
> > +                *       .(unsigned int)
> > +                */
> > +               BUILD_BUG_ON(offsetof(struct sock, __sk_common) != 0);
> > +               BUILD_BUG_ON(offsetof(possible_net_t, net) != 0);
> > +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> > +                                               struct bpf_sock_ops_kern, sk),
> > +                                     si->dst_reg, si->src_reg,
> > +                                     offsetof(struct bpf_sock_ops_kern, sk));
> > +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> > +                                               possible_net_t, net),
> > +                                     si->dst_reg, si->dst_reg,
> > +                                     offsetof(struct sock_common, skc_net));
> > +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
> > +                                               struct ns_common, inum),
> > +                                     si->dst_reg, si->dst_reg,
> > +                                     offsetof(struct net, ns) +
> > +                                     offsetof(struct ns_common, inum));
> > +#else
> > +               *insn++ = BPF_MOV64_IMM(si->dst_reg, 0);
> > +#endif
> > +               break;
> > +
> >         }
> >         return insn - insn_buf;
> >  }
> > --
> > 2.21.0
> >



--
Iago López Galeiras

Kinvolk GmbH | Adalbertstr. 6a, 10999 Berlin
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000