> On 3 Feb 2023, at 16:08, Mark Michelson <[email protected]> wrote: > > On 1/25/23 05:36, Abhiram Sangana wrote: >> Hi Mark, >> I have replied to your comments. Can you please have a look when you get a >> chance? > > I had a look at the code itself, and from a purely mechanical perspective, I > can't see anything wrong with it. I have some high level comments down below, > though. > >> Thanks, >> Abhiram Sangana >>> On 17 Jan 2023, at 12:37, Abhiram Sangana <[email protected]> >>> wrote: >>> >>> Hi Mark, >>> >>> Thanks for reviewing the patch. >>> >>>> On 16 Jan 2023, at 21:34, Mark Michelson <[email protected]> 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 - >>> https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_ovn_patch_20221018153342.164530-2D1-2Dsangana.abhiram-40nutanix.com_&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=um7MDrMKGeU4Ozd9VvKIJQ5cVpVizh_lH5ILPVlt2zE&e= >>> >>>> >>>> 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. > > IMO, this is not good enough. As a user I would be surprised to find that > additional entries are added to conntrack simply because I created a label > for my ACL. I think that another option needs to be created that specifically > enables committing dropped packets to CT.
Sure - should we add a global option to commit connections dropped by ACLs or a per logical_switch option? Also, given that `label` field was not supported for ACLs with action as “drop” or “reject” before, should we bump the NB DB schema version? > >>> >>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_commit_cb2a5486a3a3756ee3868da0050d737c8989770c&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=9jkE_C4UAA8pOWcIzNbymLs0ai6Am90PvwTE1oFjHCc&e= > > If the explicit option to enable committing dropped packets to conntrack is > added, then I think we can move forward without needing this OVS patch in > place. Got it. Thanks, Abhiram Sangana > >>> >>>> 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 <[email protected]> >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=ti2yvyC3UL6J5DeC-5EO7Et54ZMOzaaNNPp02UYpSCc&e= _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
