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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Oct 2022 16:53:38 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf@...r.kernel.org
Cc:     razor@...ckwall.org, ast@...nel.org, andrii@...nel.org,
        martin.lau@...ux.dev, john.fastabend@...il.com,
        joannelkoong@...il.com, memxor@...il.com, joe@...ium.io,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 01/10] bpf: Add initial fd-based API to attach tc
 BPF programs

On 10/5/22 4:32 PM, Toke Høiland-Jørgensen wrote:
> Daniel Borkmann <daniel@...earbox.net> writes:
>> On 10/5/22 12:33 PM, Toke Høiland-Jørgensen wrote:
>>> Daniel Borkmann <daniel@...earbox.net> writes:
[...]
>>>> +/* (Simplified) user return codes for tc prog type.
>>>> + * A valid tc program must return one of these defined values. All other
>>>> + * return codes are reserved for future use. Must remain compatible with
>>>> + * their TC_ACT_* counter-parts. For compatibility in behavior, unknown
>>>> + * return codes are mapped to TC_NEXT.
>>>> + */
>>>> +enum tc_action_base {
>>>> +	TC_NEXT		= -1,
>>>> +	TC_PASS		= 0,
>>>> +	TC_DROP		= 2,
>>>> +	TC_REDIRECT	= 7,
>>>> +};
>>>
>>> Looking at things like this, though, I wonder if having a separate name
>>> (at least if it's too prominent) is not just going to be more confusing
>>> than not? I.e., we go out of our way to make it compatible with existing
>>> TC-BPF programs (which is a good thing!), so do we really need a
>>> separate name? Couldn't it just be an implementation detail that "it's
>>> faster now"?
>>
>> Yep, faster is an implementation detail; and developers can stick to existing
>> opcodes. I added this here given Andrii suggested to add the action codes as
>> enum so they land in vmlinux BTF. My thinking was that if we go this route,
>> we could also make them more user friendly. This part is 100% optional,
>> but for new developers it might lower the barrier a bit I was hoping given
>> it makes it clear which subset of actions BPF supports explicitly and with
>> less cryptic name.
> 
> Oh, I didn't mean that we shouldn't define these helpers; that's totally
> fine, and probably useful. Just that when everything is named 'TC'
> anyway, having a different name (like TCX) is maybe not that important
> anyway?

I thought about this initially, but then also it has nothing to do with tcx
given it can just as well be used on both old/new style attachments, thus
wanted to avoid potential confusion around this.

>>> Oh, and speaking of compatibility should 'tc' (the iproute2 binary) be
>>> taught how to display these new bpf_link attachments so that users can
>>> see that they're there?
>>
>> Sounds reasonable, I can follow-up with the iproute2 support as well.
> 
> Cool!

Thanks,
Daniel