Re: [ovs-dev] [patch_v6 0/5] Userspace Datapath: Add ALG support.
-Original Message- From: on behalf of Ben Pfaff Date: Thursday, August 3, 2017 at 9:58 AM To: Darrell Ball Cc: "d...@openvswitch.org" Subject: Re: [ovs-dev] [patch_v6 0/5] Userspace Datapath: Add ALG support. On Sat, Jul 15, 2017 at 12:49:51PM -0700, Darrell Ball wrote: > ALG infra is added with support for FTP and TFTP. > Both V4 and V6 are supported. Also, NAT is supported. Hi Darrell, would you mind doing a quick rebase and repost to incorporate the changes you mention for a clean Windows build? It would help me out. sure Ben; let me do that now then. Thanks Darrell Thanks, Ben. ___ 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=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=IXlLUGsj8Q-IsAclgTAOInZZ6w5LXqSvSqzFXo5GGvY&s=vm4eIF4RmjpRUgjVDlhxnDtX_Muc7D5srUcBc7ZM08c&e= ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch_v6 0/5] Userspace Datapath: Add ALG support.
On Sat, Jul 15, 2017 at 12:49:51PM -0700, Darrell Ball wrote: > ALG infra is added with support for FTP and TFTP. > Both V4 and V6 are supported. Also, NAT is supported. Hi Darrell, would you mind doing a quick rebase and repost to incorporate the changes you mention for a clean Windows build? It would help me out. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch_v6 0/5] Userspace Datapath: Add ALG support.
On 7/16/17, 11:25 PM, "Ilya Maximets" wrote: > ALG infra is added with support for FTP and TFTP. > Both V4 and V6 are supported. Also, NAT is supported. > > Three passive ftp system tests are added to complete testing > coverage of ftp for the userspace datapath, as the existing > coverage of passive ftp was limited to one part of one test > for V4 only. > Another system test is added covering tftp with NAT which > was not previously exercised. > > v5->v6: Re-instated include inadvertently removed. > Improve 2 of the new system tests in terms of > potential races. > > v4->v5: Address Ben's code review comments. > First 3 patches were committed. > > v3->v4: Fix tftp with NAT. > Add a system test covering tftp with NAT. > > v2->v3: Fix v4 passive ftp with NAT. > Fix V6 passive ftp; parse check was broken. > Add 3 tests covering v4/v6 passive ftp to > complete ALG coverage in the system tests. > > Code review caught a memory leak of the alg > string such as "ftp" that could occurs during > nat tuple exhaustion. This is a pathological > user error case whose fix was tested by > instrumentated simulation. > Code review also pointed out that a connection > context copy was unclear; this was moved to the > caller where all allocation and error cleanup is > done. > Added several lock annotations that were missing > from the original conntrack code and nat code. > Other review comments were fixed. > > v1->v2: > Mostly the addition of V6 FTP and TFTP support. > > Removed define for unused FTP server port 20. > > Add overflow checks for port numbers. > > Instead of bypassing FTP bounce exploit with > auto-correct, explicitly flag packet as invalid. > > Seq number overflow and underflow checks added. > > Darrell Ball (5): > Userspace Datapath: Add ALG infra and FTP. > Userspace Datapath: Add TFTP support. > System tests: Enable ALGs for userspace. > System tests: Add 4 new ftp and tftp tests. > NEWS: Announce userspace datapath ALG support. Hi Darrell, This is not a full review. I just wanted to ask you to rename the patches and stop using 'Userspace Datapath' or 'System tests' as a module name for changes localized to only one particular module. I commented on this already. The ‘area’ field is fiexible; ‘Area’ does not have to be a file name, although it can be. This is confirmed by the submitting-patches.rst document and related discussion with those who developed the guidelines. I am using Userspace Datapath to be consistent with the Linux kernel, which is using ‘Datapath’ for all datapath changes, including conntrack and also Windows, which is using ‘Datapath-Windows’, for all datapath changes, including Conntrack. On the other hand, I consider conntrack to be not the best choice, although others are free to use it. The reason is ‘conntrack’ is ambiguous; there is a kernel file with the exact same name, conntrack.c and a windows file named Conntrack.c. I already raised this issue for 'dpdk' prefix previously here: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DJuly_335139.html&d=DwICaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=k-1rcsJmn7U9n0rvB0yCMWjS3FiNfG52fiCwj3VQF3E&s=p2L3sQwNXBllmKqJ44iynpfpAsQEGsexTJ_N6-GhuE4&e= I mentioned to Sugesh that module naming is flexible and he is free to choose a reasonable name for individual patches per submitting-patches.rst So, about current patch-set: The first two patches are localized to lib/conntrack* files and should have 'conntrack' prefix as a module name. 3rd and 4th patches should have 'system-userspace-macros' and 'system-traffic' module names respectively. Same answer as above. The ‘Area’ name in the past for these test files has used ‘Tests’, ‘System-Tests’, ‘System-Macros’ and ‘System-Traffic’ and some others. I prefer ‘System-Tests’ as ‘Tests’ is too general IMO and ‘System-Tests’ correctly identifies an ‘Area of the code’ with a common infra and theme, spread out over a few files. However, I cannot enforce the ‘System-Tests’ area name. Such names will be more accurate and conformed to existing commits and contribution guide. Best regards, Ilya Maximets. > > NEWS |1 + > include/sparse/netinet/in.h |1 + > lib/conntrack-private.h | 35 +- > lib/conntrack.c | 1088 +++--- > lib/conntrack.h | 10 +- >
Re: [ovs-dev] [patch_v6 0/5] Userspace Datapath: Add ALG support.
> ALG infra is added with support for FTP and TFTP. > Both V4 and V6 are supported. Also, NAT is supported. > > Three passive ftp system tests are added to complete testing > coverage of ftp for the userspace datapath, as the existing > coverage of passive ftp was limited to one part of one test > for V4 only. > Another system test is added covering tftp with NAT which > was not previously exercised. > > v5->v6: Re-instated include inadvertently removed. > Improve 2 of the new system tests in terms of > potential races. > > v4->v5: Address Ben's code review comments. > First 3 patches were committed. > > v3->v4: Fix tftp with NAT. > Add a system test covering tftp with NAT. > > v2->v3: Fix v4 passive ftp with NAT. > Fix V6 passive ftp; parse check was broken. > Add 3 tests covering v4/v6 passive ftp to > complete ALG coverage in the system tests. > > Code review caught a memory leak of the alg > string such as "ftp" that could occurs during > nat tuple exhaustion. This is a pathological > user error case whose fix was tested by > instrumentated simulation. > Code review also pointed out that a connection > context copy was unclear; this was moved to the > caller where all allocation and error cleanup is > done. > Added several lock annotations that were missing > from the original conntrack code and nat code. > Other review comments were fixed. > > v1->v2: > Mostly the addition of V6 FTP and TFTP support. > > Removed define for unused FTP server port 20. > > Add overflow checks for port numbers. > > Instead of bypassing FTP bounce exploit with > auto-correct, explicitly flag packet as invalid. > > Seq number overflow and underflow checks added. > > Darrell Ball (5): > Userspace Datapath: Add ALG infra and FTP. > Userspace Datapath: Add TFTP support. > System tests: Enable ALGs for userspace. > System tests: Add 4 new ftp and tftp tests. > NEWS: Announce userspace datapath ALG support. Hi Darrell, This is not a full review. I just wanted to ask you to rename the patches and stop using 'Userspace Datapath' or 'System tests' as a module name for changes localized to only one particular module. I already raised this issue for 'dpdk' prefix previously here: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335139.html So, about current patch-set: The first two patches are localized to lib/conntrack* files and should have 'conntrack' prefix as a module name. 3rd and 4th patches should have 'system-userspace-macros' and 'system-traffic' module names respectively. Such names will be more accurate and conformed to existing commits and contribution guide. Best regards, Ilya Maximets. > > NEWS |1 + > include/sparse/netinet/in.h |1 + > lib/conntrack-private.h | 35 +- > lib/conntrack.c | 1088 > +++--- > lib/conntrack.h | 10 +- > tests/system-traffic.at | 242 + > tests/system-userspace-macros.at |7 +- > 7 files changed, 1306 insertions(+), 78 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
