Re: [PATCH bpf-next 6/6] bpf: BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} require cgroups enabled

From: Stanislav Fomichev <sdf@fomichev.me>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Stanislav Fomichev <sdf@google.com>,
	Network Development <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Edward Cree <ecree@solarflare.com>,
	Quentin Monnet <quentin.monnet@netronome.com>
Subject: Re: [PATCH bpf-next 6/6] bpf: BPF_PROG_TYPE_CGROUP_{SKB,SOCK,SOCK_ADDR} require cgroups enabled
Date: Sat, 15 Dec 2018 12:40:15 -0800	[thread overview]
Message-ID: <20181215204015.GB20955@mini-arch.hsd1.ca.comcast.net> (raw)
In-Reply-To: <CAADnVQJE3v894ArK1G9720hZY=0YSyV9iobabssjx-b7KjspkA@mail.gmail.com>

On 12/14, Alexei Starovoitov wrote:
> On Thu, Dec 13, 2018 at 11:03 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > There is no way to exercise appropriate attach points without cgroups
> > enabled. This lets test_verifier correctly skip tests for these
> > prog_types if kernel was compiled without BPF cgroup support.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  include/linux/bpf_types.h |  2 ++
> >  net/core/filter.c         | 18 ++++++++++++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index 44d9ab4809bd..08bf2f1fe553 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -6,9 +6,11 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_SOCKET_FILTER, sk_filter)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_SCHED_CLS, tc_cls_act)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_SCHED_ACT, tc_cls_act)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_XDP, xdp)
> > +#ifdef CONFIG_CGROUP_BPF
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SKB, cg_skb)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SOCK, cg_sock)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, cg_sock_addr)
> > +#endif
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_IN, lwt_in)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_OUT, lwt_out)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_XMIT, lwt_xmit)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index f9348806e843..6a390e519431 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5315,6 +5315,7 @@ bpf_base_func_proto(enum bpf_func_id func_id)
> >         }
> >  }
> >
> > +#ifdef CONFIG_CGROUP_BPF
> >  static const struct bpf_func_proto *
> >  sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >  {
> > @@ -5364,6 +5365,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >                 return bpf_base_func_proto(func_id);
> >         }
> >  }
> > +#endif
> >
> >  static const struct bpf_func_proto *
> >  sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > @@ -5382,6 +5384,7 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >         }
> >  }
> >
> > +#ifdef CONFIG_CGROUP_BPF
> >  static const struct bpf_func_proto *
> >  cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> 
> I don't think it's worth uglifying the code like this.
> I prefer to leave it as-is.
Sure, up to you. I mostly included it for completeness sake. I tested on
two configs: the first one is allyesbpf, the second one is minimal set
of bpf features and no cgroups.

(For my usecase cgroups and hence these prog types are always enabled,
so it doesn't matter for me).