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 SanganaOn 17 Jan 2023, at 12:37, Abhiram Sangana <sangana.abhi...@nutanix.com> wrote: 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 - 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.
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.
Thanks, Mark MichelsonThanks, Abhiram SanganaOn 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://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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
