Re: [PATCH bpf-next 01/10] bpf: Add initial fd-based API to attach tc BPF programs

["Toke Høiland-Jørgensen" <toke@redhat.com>]

sdf@google.com writes:

>>   	struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */
>> -		__u32		target_fd;	/* container object to attach to */
>> +		union {
>> +			__u32	target_fd;	/* container object to attach to */
>> +			__u32	target_ifindex; /* target ifindex */
>> +		};
>>   		__u32		attach_bpf_fd;	/* eBPF program to attach */
>>   		__u32		attach_type;
>>   		__u32		attach_flags;
>> -		__u32		replace_bpf_fd;	/* previously attached eBPF
>
> [..]
>
>> +		union {
>> +			__u32	attach_priority;
>> +			__u32	replace_bpf_fd;	/* previously attached eBPF
>>   						 * program to replace if
>>   						 * BPF_F_REPLACE is used
>>   						 */
>> +		};
>
> The series looks exciting, haven't had a chance to look deeply, will try
> to find some time this week.
>
> 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 we should care :)

Having "better" defaults are probably a good idea (so not everything
just ends up at priority 1 by default). However, I think ultimately the
only robust solution is to make the priority override-able. Users are
going to want to combine BPF programs in ways that their authors didn't
anticipate, so the actual priority the programs run at should not be the
sole choice of the program author.

To use the example that Daniel presented at LPC: Running datadog and
cilium at the same time broke cilium because datadog took over the
prio-1 hook point. With the bpf_link API what would change is that (a)
it would be obvious that something breaks (that is good), and (b) it
would be datadog that breaks instead of cilium (because it can no longer
just take over the hook, it'll get an error instead). However, (b) means
that the user still hasn't gotten what they wanted: the ability to run
datadog and cilium at the same time. To do this, they will need to be
able to change the priorities of one or both applications.

I know cilium at least has a configuration option to change this
somewhere, but I don't think relying on every BPF-using application to
expose this (each in their own way) is a good solution. I think of
priorities more like daemon startup at boot: this is system policy,
decided by the equivalent of the init system (and in this analogy we are
currently at the 'rc.d' stage of init system design, with the hook
priorities).

One way to resolve this is to have a central daemon that implements the
policy and does all the program loading on behalf of the users. I think
multiple such daemons exist already in more or less public and/or
complete states. However, getting everyone to agree on one is also hard,
so maybe the kernel needs to expose a mechanism for doing the actual
overriding, and then whatever daemon people run can hook into that?

Not sure what that mechanism would be? A(nother) BPF hook for overriding
priority on load? An LSM hook that rewrites the system call? (can it
already do that?) Something else?

Oh, and also, in the case of TC there's also the additional issue that
execution only chains to the next program if the current one returns
TC_ACT_UNSPEC; this should probably also be overridable somehow, for the
same reasons...

-Toke