netdev - Re: [PATCH bpf-next 03/12] net: Add IFLA_XDP_EGRESS for XDP programs in the egress path

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]

Message-ID: <875zgncu0p.fsf@toke.dk>
Date:   Mon, 03 Feb 2020 21:09:42 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>
Cc:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
        prashantbhole.linux@...il.com, jasowang@...hat.com,
        davem@...emloft.net, jbrouer@...hat.com, mst@...hat.com,
        toshiaki.makita1@...il.com, daniel@...earbox.net,
        john.fastabend@...il.com, ast@...nel.org, kafai@...com,
        songliubraving@...com, yhs@...com, andriin@...com,
        David Ahern <dahern@...italocean.com>
Subject: Re: [PATCH bpf-next 03/12] net: Add IFLA_XDP_EGRESS for XDP programs in the egress path

David Ahern <dsahern@...il.com> writes:

> On 2/1/20 8:59 AM, Toke Høiland-Jørgensen wrote:
>> 
>> In any case an egress program will differ in:
>> 
>> - The context object (the RX-related fields will be invalid on egress,
>>   and we'll probably want to add new TX-related ones, such as HW
>>   TX-queue occupancy).
>
> Jakub has suggested that rx_queue_index can be a union with
> tx_queue_index; the former for the Rx path and the latter for the egress.
>
> The rest of the fields in xdp_md are legit for either direction.

Right, okay, ifindex and queue index could make sense in both
directions. But it would still mean that we would limit ourselves to
only adding new fields that would work on both RX and TX. Maybe that is
OK, though.

>> - The return code semantics (even if XDP_TX becomes equivalent to
>>   XDP_PASS, that is still a semantic difference from the RX side; and
>>   it's not necessarily clear whether we'll want to support REDIRECT on
>>   the egress side either, is it?)
>
> Why should REDIRECT not be allowed in the egress path? e.g., service
> chaining or capturing suspicious packets (e.g., encap with a header and
> redirect somewhere for analysis).

Implementation and deployment complexity, mostly (loops!)? I do agree
that it would be nice to allow it, I'm just not entirely convinced that
it's feasible...

>> So we'll have to disambiguate between the two different types of
>> programs. Which means that what we're discussing is really whether that
>> disambiguation should be encoded in the program type, or in the attach
>> type. IMO, making it a separate program type is a clearer and more
>> explicit UAPI. The verifier could still treat the two program types as
>> basically equivalent except for those cases where there has to be a
>> difference anyway. So it seems to me that all you are saving by using
>> attach_type instead of program type is the need for a new enum value and
>> a bunch of additions to switch statements? Or am I wildly
>> underestimating the effort to add a new program type?
>> 
>
> IMHO that is duplicating code and APIs for no real reason. XDP refers to
> fast path processing, the only difference is where the program is
> attached - Rx side or Tx side.

Sure, if we can guarantee API and semantic equivalence. I.e., if any XDP
program really *can* be attached as both an RX and TX program, I have no
objections to re-using the program type. But if it turns out that there
*is* a difference, we're making implicit subtypes anyway, in which case
I think it's better to be explicit about the difference.

-Toke