Re: [PATCH v7 0/6] Add eBPF hooks for cgroups

On Sat, Oct 29, 2016 at 01:59:23PM +0900, Lorenzo Colitti wrote:

On Sat, Oct 29, 2016 at 1:51 PM, Alexei Starovoitov
[off-list ref] wrote:

quoted

quoted

What's the use case for egress?

We (android networking) are currently looking at implementing network
accounting via eBPF in order to replace the out-of-tree xt_qtaguid
code. A per-cgroup eBPF program run on all traffic would be great. But
when we looked at this patchset we realized it would not be useful for
accounting purposes because even if a packet is counted here, it might
still be dropped by netfilter hooks.

don't use out-of-tree and instead drop using this mechanism or
any other in-kernel method? ;)

Getting rid of out-of-tree code is the goal, yes. We do have a
requirement that things continue to work, though. Accounting for a
packet in ip{,6}_output is not correct if that packet ends up being
dropped by iptables later on.

understood.
it could be solved by swapping the order of cgroup_bpf_run_filter()
and NF_INET_POST_ROUTING in patch 5. It was proposed some time back, but
the current patch, I think, is more symmetrical.
cgroup+bpf runs after nf hook on rx and runs before it on tx.
imo it's more consistent.
Regardless of this choice... are you going to backport cgroupv2 to
android? Because this set is v2 only.

quoted

We (facebook infrastructure) have been using iptables and bpf networking
together with great success. They nicely co-exist and complement each other.
There is no need to reinvent the wheel if existing solution works.
iptables are great for their purpose.

That doesn't really answer my "what is the use case for egress"
question though, right? Or are you saying "we use this, but we can't
talk about how we use it"?

if the question is "why patch 4 alone is not enough and patch 5 is needed"?
Then it's symmetrical access. Accounting for RX only is a half done job.

quoted

there is iptables+cBPF support. It's being used in some cases already.

Adding eBPF support to the xt_bpf iptables code would be an option for
what we want to do, yes. I think this requires that the eBPF map to be
an fd that is available to the process that exec()s iptables, but we
could do that.

yes. that's certainly doable, but sooner or later such approach will hit
scalability issue when number of cgroups is large. Same issue we saw
with cls_bpf and bpf_skb_under_cgroup(). Hence this patch set was needed
that is centered around cgroups instead of hooks. Note, unlike, tc and nf
there is no way to attach to a hook. The bpf program is attached to a cgroup.
It's an important distinction vs everything that currently exists in the stack.

On Sat, Oct 29, 2016 at 3:24 PM, Alexei Starovoitov
[off-list ref] wrote:

it could be solved by swapping the order of cgroup_bpf_run_filter()
and NF_INET_POST_ROUTING in patch 5. It was proposed some time back, but
the current patch, I think, is more symmetrical.
cgroup+bpf runs after nf hook on rx and runs before it on tx.
imo it's more consistent.

I guess what I was trying to say was: what does doing this filtering
in ip_output give you over running this from the netfilter hooks?
Doing this filtering in netfilter is much more general because there
can be complex rules both before and after the filtering is applied. I
hadn't thought of the scalability issue you note below though.

For accounting you probably want to run after the hooks, both for
ingress and for egress, because the hooks can do all sorts of stuff
like drop packets, change packet sizes, reroute them to different
interfaces, etc. Do you see use cases where you want to run before the
hooks?

Regardless of this choice... are you going to backport cgroupv2 to
android? Because this set is v2 only.

Certainly anything that can't easily be backported to, say,
android-4.4 is not really feasible in the short term. I don't think we
use network cgroups at all, so if v2 network cgroups can coexist with
v1 cgroups of other types (which what little I've read seems to
indicate) then that should be possible.

yes. that's certainly doable, but sooner or later such approach will hit
scalability issue when number of cgroups is large. Same issue we saw
with cls_bpf and bpf_skb_under_cgroup(). Hence this patch set was needed
that is centered around cgroups instead of hooks. Note, unlike, tc and nf
there is no way to attach to a hook. The bpf program is attached to a cgroup.
It's an important distinction vs everything that currently exists in the stack.

Ah, I see. Out of curiosity, what was the first scaling limitation you
hit? eBPF program length? eBPF map size?

On 10/29/2016 05:34 PM, Lorenzo Colitti wrote:

On Sat, Oct 29, 2016 at 3:24 PM, Alexei Starovoitov
[off-list ref] wrote:

quoted

it could be solved by swapping the order of cgroup_bpf_run_filter()
and NF_INET_POST_ROUTING in patch 5. It was proposed some time back, but
the current patch, I think, is more symmetrical.
cgroup+bpf runs after nf hook on rx and runs before it on tx.
imo it's more consistent.

I guess what I was trying to say was: what does doing this filtering
in ip_output give you over running this from the netfilter hooks?
Doing this filtering in netfilter is much more general because there
can be complex rules both before and after the filtering is applied. I
hadn't thought of the scalability issue you note below though.

For accounting you probably want to run after the hooks, both for
ingress and for egress, because the hooks can do all sorts of stuff
like drop packets, change packet sizes, reroute them to different
interfaces, etc. Do you see use cases where you want to run before the
hooks?

Fwiw, not sure if swapping brings much, even after netfilter there could
be complex processing that would potentially drop, mangle, redirect, etc
from tc layer (egress or from qdisc itself). But also at even lower layers
(although rather unlikely, but not impossible), for example in drivers or
shortly before passing skb to them during segmentation (GSO), etc.
Eventually, for that you'd need to monitor various things, and the cgroup
one is just at higher layers with different semantics.

quoted

Regardless of this choice... are you going to backport cgroupv2 to
android? Because this set is v2 only.

Certainly anything that can't easily be backported to, say,
android-4.4 is not really feasible in the short term. I don't think we
use network cgroups at all, so if v2 network cgroups can coexist with
v1 cgroups of other types (which what little I've read seems to
indicate) then that should be possible.

quoted

yes. that's certainly doable, but sooner or later such approach will hit
scalability issue when number of cgroups is large. Same issue we saw
with cls_bpf and bpf_skb_under_cgroup(). Hence this patch set was needed
that is centered around cgroups instead of hooks. Note, unlike, tc and nf
there is no way to attach to a hook. The bpf program is attached to a cgroup.
It's an important distinction vs everything that currently exists in the stack.

Ah, I see. Out of curiosity, what was the first scaling limitation you
hit? eBPF program length? eBPF map size?

The scalability issue is not really program length or map size from eBPF
side in this context. While for v1, you have the bpf_get_cgroup_classid()
helper available on egress (not ingress though) that can scale with larger
number of cgroups since it works on the user-defined net_cls tagging, but
for v2, bpf_skb_under_cgroup() was initially introduced, which can only test
whether the sk's v2 cgroup related to the skb is in the sub-hierarchy of
a specific cgroup that is provided via maps. Effectively, when you have a
larger number of v2 cgroups that boolean test will not scale and you need
to linearly test through various cgroups. It's good enough when need to
special case only few cgroups in the v2 hierarchy on egress. Idea was that
attaching to cgroup itself would resolve this from a different angle for
egress and also ingress in a complementary way, but also seems to open up
for various other use-cases at the same time as seen from various patches
on the list.

Cheers,
Daniel

On Sun, Oct 30, 2016 at 5:29 AM, Daniel Borkmann [off-list ref] wrote:

Fwiw, not sure if swapping brings much, even after netfilter there could
be complex processing that would potentially drop, mangle, redirect, etc
from tc layer (egress or from qdisc itself). But also at even lower layers

I agree lots of stuff can still happen after the netfilter hooks have
finished. But it seems to me that netfilter hooks are more flexible
and have more access to more data (L2 headers, output devices,
conntrack state entries, etc. etc.) than this hook. So it seems to me
that this hook would be more useful if it ran after netfilter on
egress.

That way, if you want to modify the packet or do something
sophisticated in netfilter, you can still use the eBPF hook on the
results of that operation, and if you don't want to run netfilter, you
can write netfilter rules to skip the packet (and maybe still fix it
up later, perhaps in another netfilter chain).