Hi Mark, I have replied to your comments. Can you please have a look when you get a chance?
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. > > 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= > > >> 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
