Skip to content

Payload scan#97

Merged
koolzz merged 24 commits intosdnfv:developfrom
dennisafa:payload_scan
Apr 26, 2019
Merged

Payload scan#97
koolzz merged 24 commits intosdnfv:developfrom
dennisafa:payload_scan

Conversation

@dennisafa
Copy link
Copy Markdown
Member

@dennisafa dennisafa commented Apr 9, 2019

Adding a NF that scans a packets payload for a given string.

Summary:

This NF provides the functionality to search for a string within a given UDP or TCP packet payload. Packet is forwarded to its destination NF on a match, dropped otherwise.

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality X
New NF/onvm_mgr args X
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

  • Dependencies: None

TODO before merging :

  • PR is ready for review
  • Add README
  • add "invert" option (drop if user string matches packets, forward if not)
  • test performance with PCAP

Test Plan:

Used pktgen to send UDP packets and TCP packets. Checked that the packet is forwarded on a match and dropped on a mismatch. Also checked if the packet sent is neither UDP or TCP i.e icmp

Review:

TBA

  • Sanity checks, assigned to @koolzz @kevindweb
    • Run make in /onvm and /examples directories
  • Code style, assigned to @koolzz @kevindweb
    • Run linter
    • Check everything style related
  • Code design, assigned to @koolzz @kevindweb
    • Check for memory leaks
    • Check that code is reusable
    • Code is clean, functions are concise
    • Verify that edge cases properly handled
  • Performance, assigned to @koolzz @kevindweb
    • Run Speed Tester NF, report performance.
  • Documentation, assigned to @koolzz @kevindweb
    • Check if the new changes are well documented, in both code and READMEs

@onvm
Copy link
Copy Markdown

onvm commented Apr 9, 2019

In response to PR creation

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented Apr 9, 2019

In response to PR creation

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35191985

Linter Failed

examples/payload_scan/payload_scan.c:205: If an else has a brace on one side, it should have it on both [readability/braces] [5]
examples/payload_scan/payload_scan.c:216: If an else has a brace on one side, it should have it on both [readability/braces] [5]
Total errors found: 2

@koolzz koolzz self-requested a review April 9, 2019 16:16
@dennisafa
Copy link
Copy Markdown
Member Author

@onvm one mo' time

@onvm
Copy link
Copy Markdown

onvm commented Apr 9, 2019

@onvm one mo' time

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented Apr 9, 2019

@onvm one mo' time

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35194883

Linter Passed

@kevindweb
Copy link
Copy Markdown
Contributor

Hey, great job on this, tested speed tester just in case and it actually got better than my average run (coincidence but still nice). I haven't done much with pktgen, but set it up and test early next week - just a few things, but we might want to update examples/Makefile to reflect payload as a new nf, not necessary, just helpful. Also, maybe make a readme for this new NF because all the others have one?

@dennisafa
Copy link
Copy Markdown
Member Author

Thanks kevin! Readme is part of the to-do list, i will work on that. I’ll implement your makefile suggestion.

Copy link
Copy Markdown
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Kevin about the docs/Makefile, also added some code style comments

Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c
Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c Outdated
@dennisafa
Copy link
Copy Markdown
Member Author

dennisafa commented Apr 17, 2019

Pktgen performance (Forwarded packets)

64B 128B 256B
8.4 Mpps 7.9 Mpps 4.5 Mpps

@twood02 Performance using tcp Pktgen generated packets of different lengths. I have a pcap file that I generated from wireshark for 64B packets, but need to create much more of them (using scapy I presume) to make it work.. I will upload the results here once I have them.

@koolzz
Copy link
Copy Markdown
Member

koolzz commented Apr 17, 2019

Pktgen performance

64B 128B 64B
8.4 Mpps 7.9 Mpps 4.5 Mpps
@twood02 Performance using tcp Pktgen generated packets of different lengths. I have a pcap file that I generated from wireshark for 64B packets, but need to create much more of them (using scapy I presume) to make it work.. I will upload the results here once I have them.

Good stuff just make it clear which pcap files have matches or not

Comment thread examples/payload_scan/README.md Outdated
Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c
@koolzz koolzz self-requested a review April 20, 2019 17:30
Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c
@koolzz
Copy link
Copy Markdown
Member

koolzz commented Apr 21, 2019

@dennisafa I'm assuming this is ready for merging? How can I test it? What string where you using?

@dennisafa
Copy link
Copy Markdown
Member Author

dennisafa commented Apr 21, 2019

@koolzz yes this is ready. Run pktgen and set the search string to “xyz”, as each pktgen packets’ payload contains that.
Edit: you may test both UDP and TCP packets by editing the lua config file. Test a protocol different from tcp or udp as an edge case.

Copy link
Copy Markdown
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I approve, have left some style nits though would merge after you fix those.

I tested with speed_tester instead, using a pcap file. Works as expected. The only concern is search strings that have spaces in them, which as I have discovered is a current bug in the launch scripts as it butchers special character args. I'll submit a bug report for that but its unrelated to this pr.

Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c Outdated
Comment thread examples/payload_scan/payload_scan.c Outdated
@koolzz
Copy link
Copy Markdown
Member

koolzz commented Apr 26, 2019

@onvm sanity check

@onvm
Copy link
Copy Markdown

onvm commented Apr 26, 2019

@onvm sanity check

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented Apr 26, 2019

@onvm sanity check

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35198882

Linter Failed

examples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/aes_encrypt/aes.h:185: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/flow_table.h:63: #endif line should be "#endif // FLOW_TABLE_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/msgbuf.h:71: #endif line should be "#endif // MSGBUF_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/openflow.h:969: #endif line should be "#endif // OPENFLOW_H" [build/header_guard] [5]
examples/flow_table/openflow.h:50: Using deprecated casting style. Use static_cast(...) instead [readability/casting] [4]
examples/flow_table/openflow.h:569: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:634: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:771: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:804: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:865: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:926: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 8
examples/flow_table/sdn.c:334: { should almost always be at the end of the previous line [whitespace/braces] [4]
examples/flow_table/sdn.c:365: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
examples/flow_table/sdn.h:104: #endif line should be "#endif // SDN_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/sdn_pkt_list.h:122: #endif line should be "#endif // SDN_PKT_LIST_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/setupconn.h:53: #endif line should be "#endif // SETUPCONN_H" [build/header_guard] [5]
Total errors found: 1
examples/payload_scan/payload_scan.c:95: Lines should be <= 120 characters long [whitespace/line_length] [5]
examples/payload_scan/payload_scan.c:114: Almost always, snprintf is better than strcpy [runtime/printf] [4]
examples/payload_scan/payload_scan.c:126: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 3
examples/speed_tester/speed_tester.c:364: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
onvm/onvm_mgr/main.c:109: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
onvm/onvm_mgr/onvm_init.c:107: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:114: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:116: { should almost always be at the end of the previous line [whitespace/braces] [4]
Total errors found: 3
onvm/onvm_mgr/onvm_pkt.c:68: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_common.h:366: #endif line should be "#endif // ONVM_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_config_common.h:208: #endif line should be "#endif // ONVM_CONFIG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_msg_common.h:61: #endif line should be "#endif // ONVM_MSG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_nflib.c:523: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_pkt_common.c:98: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_sc_common.h:70: #endif line should be "#endif // ONVM_SC_COMMON_H" [build/header_guard] [5]
onvm/onvm_nflib/onvm_sc_common.h:70: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
onvm/onvm_nflib/onvm_sc_mgr.h:79: #endif line should be "#endif // ONVM_SC_MGR_H" [build/header_guard] [5]
Total errors found: 1

@koolzz
Copy link
Copy Markdown
Member

koolzz commented Apr 26, 2019

I'll merge but we gotta fix the few small lint issues before release

@koolzz koolzz merged commit aae9fa3 into sdnfv:develop Apr 26, 2019
@dennisafa dennisafa deleted the payload_scan branch April 27, 2019 02:52
@koolzz koolzz added this to the ONVM 19.05 Release milestone May 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants