Ben Pfaff <[email protected]> writes:
> Hi Aaron! This will move the following fields from the L4 segment to
> the L3 segment:
>
> struct in6_addr nd_target; /* IPv6 neighbor discovery (ND) target. */
> struct eth_addr arp_sha; /* ARP/ND source hardware address. */
> struct eth_addr arp_tha; /* ARP/ND target hardware address. */
> ovs_be16 tcp_flags; /* TCP flags/ICMPv6 ND options type.
> * With L3 to avoid matching L4. */
> ovs_be16 pad2; /* Pad to 64 bits. */
> struct ovs_key_nsh nsh; /* Network Service Header keys */
>
>
> The one that stands out to me is tcp_flags. I remember that we had a
> reason that we wanted to include this in the L3 segment. I think it was
> because we might want to match on flags without matching on anything
> else in L4, but I don't remember the exact details. There is a comment
> to that effect above.
I guess the reason that would be done would be to implement a stateless
firewall using the openflow rules. I haven't seen many flow pipelines
recently that use tcp_flags, but I can see it might help those kinds of
scenarios where we want to allow/drop packets based on some kinds of
flags (or drop xmas packets before they get processed).
Do you think it will be a problem? For now, I assume most folks are
using the ct() action and that should be based on ct_state field (which
isn't impacted).
Unfortunately, tcp_flags got overloaded with nd_target, and making any
kind of change there will be an ABI breakage when we backport (which was
a consideration for moving the nd_target field, as well). In the PATCH
form, I plan to expand on that a little bit. Maybe a future series
could split this field out.
> I suggest including the list of fields being moved in the commit message
> (i.e. the code excerpt above) so that it's clearer what's changing.
ACK
> Some more below:
>
> On Wed, May 11, 2022 at 10:52:41AM -0400, Aaron Conole wrote:
>> During the sweep stage, revalidator will compare the dumped WC with a
>> generated WC - these will mismatch because the generated WC will include
>> such dependent ND fields. We will then invalidate the flow and as a side
>> effect force an upcall.
>
> I find "include" (or "exclude") a little ambiguous here. I think that
> saying that the generated or dumped WC "matches" or "does not match" on
> a particular field would be clearer. Some more like this below:
Makes sense to me.
>> By redefining the boundary, we shift these fields to the l4 subtable, and
>> cause masks to be generated by including the requisite fields. Two
>> other approaches were considered - moving the nd_target field alone
>> (which leaves arp_sha/nd_sll, arp_tha/nd_tll, and tcp_flags still as
>> issues so this was avoided) and collapsing the l3/l4 segments into one
>> (which would result in flow explosion in real scenarios, so it was also
>> discarded as a solution).
>>
>> Fixes: 476f36e83bc5 ("Classifier: Staged subtable matching.")
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2081773
>> Reported-by: Numan Siddique <[email protected]>
>> Suggested-by: Ilya Maximets <[email protected]>
>> Signed-off-by: Aaron Conole <[email protected]>
>
> Thanks for adding a test!
:)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
