On 10/05, Daniel Borkmann wrote:
On 10/5/22 2:55 AM, sdf@xxxxxxxxxx wrote:
> On 10/05, Daniel Borkmann wrote:
[...]
>
> The series looks exciting, haven't had a chance to look deeply, will try
> to find some time this week.
Great, thanks!
> We've chatted briefly about priority during the talk, let's maybe
discuss
> it here more?
>
> I, as a user, still really have no clue about what priority to use.
> We have this problem at tc, and we'll seemingly have the same problem
> here? I guess it's even more relevant in k8s because internally at G we
> can control the users.
>
> Is it worth at least trying to provide some default bands / guidance?
>
> For example, having SEC('tc/ingress') receive attach_priority=124 by
> default? Maybe we can even have something like 'tc/ingress_first' get
> attach_priority=1 and 'tc/ingress_last' with attach_priority=254?
> (the names are arbitrary, we can do something better)
>
> ingress_first/ingress_last can be used by some monitoring jobs. The rest
> can use default 124. If somebody really needs a custom priority, then
they
> can manually use something around 124/2 if they need to trigger before
the
> 'default' priority or 124+124/2 if they want to trigger after?
>
> Thoughts? Is it worth it? Do we care?
I think guidance is needed, yes, I can add a few paragraphs to the libbpf
header file where we also have the tc BPF link API. I had a brief
discussion
around this also with developers from datadog as they also use the infra
via tc BPF. Overall, its a hard problem, and I don't think there's a good
generic solution. The '*_last' is implied by prio=0, so that kernel auto-
allocates it, and for libbpf we could add an API for it where the user
does not need to specify prio specifically. The 'appending' is reasonable
to me given if an application explicitly requests to be added as first
(and e.g. enforces policy through tc BPF), but some other 3rd party
application
prepends itself as first, it can bypass the former, which would be too
easy
to shoot yourself in the foot. Overall the issue in tc land is that
ordering
matters, skb packet data could be mangled (e.g. IPs NATed), skb fields can
be mangled, and we can have redirect actions (dev A vs. B); the only way
I'd
see were this is possible if somewhat verifier can annotate the prog when
it didn't observe any writes to skb, and no redirect was in play. Then
you've
kind of replicated the constraints similar to tracing where the attachment
can say that ordering doesn't matter if all the progs are in same style.
Otherwise, explicit corporation is needed as is today with rest of tc (or
as Toke did in libxdp) with multi-attach. In the specific case I mentioned
at LPC, it can be made to work given one of the two is only observing
traffic
at the layer, e.g. it could get prepended if there is guarantee that all
return codes are tc_act_unspec so that there is no bypass and then you'll
see all traffic or appended to see only traffic which made it past the
policy. So it all depends on the applications installing programs, but to
solve it generically is not possible given ordering and conflicting
actions.
So, imho, an _append() API for libbpf can be added along with guidance for
developers when to use _append() vs explicit prio.
Agreed, it's a hard problem to solve, especially from the kernel side.
Ideally, as Toke mentions on the side thread, there should be some kind
of system daemon or some other place where the ordering is described.
But let's start with at least some guidance on the current prio.
Might be also a good idea to narrow down the prio range to 0-65k for
now? Maybe in the future we'll have some special PRIO_MONITORING_BEFORE_ALL
and PRIO_MONITORING_AFTER_ALL that trigger regardless of TC_ACT_UNSPEC?
I agree with Toke that it's another problem with the current action based
chains that's worth solving somehow (compared to, say, cgroup programs).
Thanks,
Daniel
> > ����� };
>
> > ����� struct { /* anonymous struct used by BPF_PROG_TEST_RUN command
*/
> > @@ -1452,7 +1460,10 @@ union bpf_attr {
> > ����� } info;
>
> > ����� struct { /* anonymous struct used by BPF_PROG_QUERY command */
> > -������� __u32������� target_fd;��� /* container object to query */
> > +������� union {
> > +����������� __u32��� target_fd;��� /* container object to query */
> > +����������� __u32��� target_ifindex; /* target ifindex */
