Hi Mark, Thanks for reviewing the patch.
> On 16 Jan 2023, at 21:34, Mark Michelson <mmich...@redhat.com> wrote: > > Hello Abhiram, > > I haven't taken a close look at every line of the series, but I have two > high-level questions/observations. > > First, what is the benefit of using this compared to ACL logging or drop > sampling? I had done some basic testing on ACL logging and exporting Netflow records for OVN bridge. I noticed a significant load on ovs-vswitchd when there are a large number of connections created in the bridge (70-80% CPU usage with 1000 connections per sec for Netflow and 60-70% CPU with 1000 packets per sec for ACL logging), probably due to large number of upcalls. I haven't tested specifically with drop sampling but expected a similar cost if we use 100% sampling. The alternative is to use metering with ACL logs or sampling part of the connections before exporting them but we might miss some of the connections this way. With the conntrack approach, ovs-vswitchd would not be involved and CMS can read the CT table or receive connection tracking events from nf_conntrack kernel module via ctnetlink (a single event is generated for each connection). We should be able to get all/most of the connections dropped by ACLs. However, this approach is datapath specific. There was brief discussion regarding the same in the RFC thread - [ovs-dev,RFC] northd, controller: Commit flows dropped by ACLs in a separate CT zone - http://patchwork.ozlabs.org/project/ovn/patch/20221018153342.164530-1-sangana.abhi...@nutanix.com/ > > Second, I think that if we are to accept this patch, the behavior needs to be > optional, and it needs to be opt-in. In other words, someone needs to > explicitly enable the behavior on the logical switch in order to commit > dropped connections to CT. If the goal is to use this as a debugging tool, it > should only be enabled when attempting to debug. I'm not knowledgeable about > the inner workings of CT, but committing every dropped connection to CT > sounds like a vector for a DOS exploit to me. Someone else can comment in > case I'm completely wrong here. > While the DROP CT zone is created unconditionally for each logical switch, connections are only committed if the ACL dropping/rejecting them has label set. So, user can create drop/reject ACLs without labels (default behavior today) if they don't want to use this feature. Yes, committing dropped connections is a potential DOS vector. I am planning to send another patch that limits the size of the DROP CT zone - ovs datapath already has support to limit the size of CT zones. https://github.com/openvswitch/ovs/commit/cb2a5486a3a3756ee3868da0050d737c8989770c > Thanks, > Mark Michelson Thanks, Abhiram Sangana > On 1/13/23 07:44, Abhiram Sangana wrote: >> This patch commits connections dropped/rejected by ACLs with label >> (introduced in 0e0228be (northd: Add ACL label)) to the connection >> tracking table. The dropped connections are committed in a separate >> conntrack zone so that they can be managed independently and do not >> interact with the connection tracking state of allowed connections. >> Each logical switch is assigned a new conntrack zone for committing >> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >> A new lflow action "ct_commit_drop" is introduced that commits flows >> to connection tracking table in a zone identified by >> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >> and non-empty label translates to include "ct_commit_drop" in its >> actions instead of simply dropping/rejecting the packet. >> This provides a new approach to identify connections dropped by ACLs >> besides the existing ACL logging and drop sampling approaches. >> Signed-off-by: Abhiram Sangana <sangana.abhi...@nutanix.com> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
