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. :-) >> I'm trying to figure out whether I need to do a detailed review myself >> or just take your acks. > > Well, I have never heard anyone say fewer eyes was better. > >> Thanks, >> >> Ben. > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
