On Thu, Nov 30, 2017 at 11:09:05AM -0500, Aaron Conole wrote: > Aaron Conole <[email protected]> writes: > > > Ben Pfaff <[email protected]> writes: > > > >> On Mon, Nov 27, 2017 at 06:11:42PM -0500, Aaron Conole wrote: > >>> Darrell Ball <[email protected]> writes: > >>> > >>> > Some refactoring of alg support is done. > >>> > Also algs are disabled by default unless an alg specifier > >>> > is supplied; this allows for enhanced security and matches > >>> > later kernels. > >>> > Another change to allow for non-standard alg conntrol > >>> > port specification. > >>> > > >>> > > >>> > Darrell Ball (3): > >>> > conntrack: Refactor algs. > >>> > conntrack: Allow specified alg port numbers. > >>> > conntrack: Disable algs by default. > >>> > > >>> > lib/conntrack.c | 168 > >>> > ++++++++++++++++++++++++++++++++++--------------- > >>> > lib/conntrack.h | 8 +-- > >>> > lib/dpif-netdev.c | 4 +- > >>> > tests/test-conntrack.c | 6 +- > >>> > 4 files changed, 127 insertions(+), 59 deletions(-) > >>> > >>> For the series - > >>> > >>> Acked-by: Aaron Conole <[email protected]> > >> > >> Hi Aaron. Thank you for reviewing--it is time consuming work and does > >> not get enough credit. Can you describe your review process a little? > > > > Sure. For this, the original work was at: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341035.html > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341037.html > > > > Darrel and I had some discussions there, and I asked for this to be > > split out. Additionally, I had noticed while reviewing that it didn't > > seem non-standard ports were covered for helpers, so asked that such a > > change be folded in (and Darrell did). > > > > For the code: > > I applied the code, and didn't see anything obviously wrong at the end > > (although I did the whole series at once, in the v2 rather than doing > > patch-at-a-time). I could still read through the conntrack code, and > > presumably I am able to follow what is happening. > > > > Additionally, once applied, I ran `./utilities/checkpatch.py -3` to > > ensure that nothing slipped by. > > > > For the functionality: > > I ran the l7 tests via `make check-system-userspace`, and saw that the > > l7 ftp tests were passing. This will require installation of pyftpdlib > > and tftpy to execute properly (otherwise they'll be skipped). Those > > tests are explicitly using 'alg=', so should still pass (and they do). > > > > I didn't check anything with manual sets of flows. I will do that now, > > though since outlining this makes me realize I never checked the > > 'negative' case (meaning assume that no alg= is given, ensure that the > > helper really isn't assigned). I can report back on that tomorrow, if > > it makes sense. > > So, I just did a simple test - > > table=0,priority=1,action=drop > table=0,priority=10,arp,action=normal > table=0,priority=10,icmp,action=normal > table=0,priority=100,in_port=veth4,tcp,tp_dst=2121,action=ct(table=1) > table=0,priority=90,in_port=veth4,tcp,action=ct(table=2) > table=0,priority=100,in_port=veth2,tcp,action=ct(table=2) > > table=1,in_port=veth4,tcp,ct_state=+trk+new,action=ct(commit),veth2 > table=1,in_port=veth4,tcp,ct_state=+trk+est,action=veth2 > table=2,in_port=veth4,tcp,ct_state=+trk+est,action=veth2 > table=2,in_port=veth2,tcp,ct_state=+trk+est,action=veth4 > > with two namespaces. Please forgive the funky ruleset - I was checking > something else out. > > [root@wsfd-netdev61 ovs]# ovs-appctl dpctl/dump-conntrack netdev@ovs-netdev > tcp,orig=(src=192.168.1.2,dst=192.168.1.1,sport=59804,dport=2121),reply=(src=192.168.1.1,dst=192.168.1.2,sport=2121,dport=59804),protoinfo=(state=ESTABLISHED) > > And my client process: > > [root@wsfd-netdev61 ~]# ip netns exec ftp_client wget ftp://192.168.1.1:2121/ > --no-passive-ftp > --2017-11-30 10:45:46-- ftp://192.168.1.1:2121/ > => ‘.listing’ > Connecting to 192.168.1.1:2121... connected. > Logging in as anonymous ... Logged in! > ==> SYST ... done. ==> PWD ... done. > ==> TYPE I ... done. ==> CWD not needed. > ==> PORT ... > Invalid PORT. > Retrying. > > So, I'm at least cursorily happy with this initial negative test. > Perhaps I'll post it in the l7 datapath tests once this series is merged > (or Darrell can post his own since I think he's respinning). > > Hope this helps. :-)
Thanks a lot for all the review and testing. It is very helpful. I don't feel like I need to spend much time looking at these patches myself. It sounds like there's a v3 due, based on Darrell's own messages, so I'll wait for that. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
