On 10/19/23 16:11, Ihar Hrachyshka wrote:
> The command reads a flow string and an optional additional payload on
> stdin and produces a hex-string representation of the corresponding
> frame on stdout. It may receive more than a single input line.
>
> The command may be useful in tests, where we may need to produce hex
> frame payloads to compare observed frames against.
>
> As an example of the tool use, a single test case is converted to it.
> The test uses both normal and --bad-csum behavior of the tool,
> confirming it works as advertised.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> ---
> v1: - initial version.
> v2: - convert from appctl to ovstest.
> - add --bad-csum support.
> - drop separate test case and man for the tool.
> v3: - fix build warnings on restricted ovs_be16 ++.
> ---
> lib/flow.c | 16 +++++-
> lib/flow.h | 2 +-
> lib/netdev-dummy.c | 4 +-
> ofproto/ofproto-dpif-trace.c | 2 +-
> ofproto/ofproto-dpif.c | 4 +-
> tests/dpif-netdev.at | 45 ++++++++--------
> tests/test-odp.c | 99 +++++++++++++++++++++++++++++++++++-
> utilities/ovs-ofctl.c | 2 +-
> 8 files changed, 140 insertions(+), 34 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index fe226cf0f..cd7832710 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3306,6 +3306,8 @@ packet_expand(struct dp_packet *p, const struct flow
> *flow, size_t size)
> * (This is useful only for testing, obviously, and the packet isn't really
> * valid. Lots of fields are just zeroed.)
> *
> + * If 'bad_csum' is true, the final IP checksum is invalid.
> + *
> * For packets whose protocols can encapsulate arbitrary L7 payloads, 'l7'
> and
> * 'l7_len' determine that payload:
> *
> @@ -3318,7 +3320,7 @@ packet_expand(struct dp_packet *p, const struct flow
> *flow, size_t size)
> * from 'l7'. */
> void
> flow_compose(struct dp_packet *p, const struct flow *flow,
> - const void *l7, size_t l7_len)
> + const void *l7, size_t l7_len, bool bad_csum)
> {
> /* Add code to this function (or its callees) for emitting new fields or
> * protocols. (This isn't essential, so it can be skipped for initial
> @@ -3370,7 +3372,17 @@ flow_compose(struct dp_packet *p, const struct flow
> *flow,
> /* Checksum has already been zeroed by put_zeros call. */
> ip->ip_csum = csum(ip, sizeof *ip);
>
> - dp_packet_ol_set_ip_csum_good(p);
> + if (bad_csum) {
> + /* Internet checksum is a sum complement to zero, so any other
> + * value will result in an invalid checksum. Here, we flip one
> + * bit.
> + */
> + ip->ip_csum ^= (OVS_FORCE ovs_be16) 0x1;
> + dp_packet_ip_checksum_bad(p);
> + } else {
> + dp_packet_ol_set_ip_csum_good(p);
> + }
> +
> pseudo_hdr_csum = packet_csum_pseudoheader(ip);
> flow_compose_l4_csum(p, flow, pseudo_hdr_csum);
> } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> diff --git a/lib/flow.h b/lib/flow.h
> index a9d026e1c..75a9be3c1 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -127,7 +127,7 @@ void flow_set_mpls_bos(struct flow *, int idx, uint8_t
> stack);
> void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse);
>
> void flow_compose(struct dp_packet *, const struct flow *,
> - const void *l7, size_t l7_len);
> + const void *l7, size_t l7_len, bool bad_csum);
> void packet_expand(struct dp_packet *, const struct flow *, size_t size);
>
> bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t
> *nw_proto,
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 1a54add87..9ead449e1 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -1758,7 +1758,7 @@ eth_from_flow_str(const char *s, size_t packet_size,
>
> packet = dp_packet_new(0);
> if (packet_size) {
> - flow_compose(packet, flow, NULL, 0);
> + flow_compose(packet, flow, NULL, 0, false);
> if (dp_packet_size(packet) < packet_size) {
> packet_expand(packet, flow, packet_size);
> } else if (dp_packet_size(packet) > packet_size){
> @@ -1766,7 +1766,7 @@ eth_from_flow_str(const char *s, size_t packet_size,
> packet = NULL;
> }
> } else {
> - flow_compose(packet, flow, NULL, 64);
> + flow_compose(packet, flow, NULL, 64, false);
> }
>
> ofpbuf_uninit(&odp_key);
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 527e2f17e..b86e7fe07 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -440,7 +440,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> if (generate_packet) {
> /* Generate a packet, as requested. */
> packet = dp_packet_new(0);
> - flow_compose(packet, flow, l7, l7_len);
> + flow_compose(packet, flow, l7, l7_len, false);
> } else if (packet) {
> /* Use the metadata from the flow and the packet argument to
> * reconstruct the flow. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ba5706f6a..9e8faf829 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1255,7 +1255,7 @@ check_ct_eventmask(struct dpif_backer *backer)
>
> /* Compose a dummy UDP packet. */
> dp_packet_init(&packet, 0);
> - flow_compose(&packet, &flow, NULL, 64);
> + flow_compose(&packet, &flow, NULL, 64, false);
>
> /* Execute the actions. On older datapaths this fails with EINVAL, on
> * newer datapaths it succeeds. */
> @@ -1348,7 +1348,7 @@ check_ct_timeout_policy(struct dpif_backer *backer)
>
> /* Compose a dummy UDP packet. */
> dp_packet_init(&packet, 0);
> - flow_compose(&packet, &flow, NULL, 64);
> + flow_compose(&packet, &flow, NULL, 64, false);
>
> /* Execute the actions. On older datapaths this fails with EINVAL, on
> * newer datapaths it succeeds. */
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 85119fb81..4a3c2b53c 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -746,19 +746,25 @@ OVS_VSWITCHD_START(
> # Modify the ip_dst addr to force changing the IP csum.
> AT_CHECK([ovs-ofctl add-flow br1
> in_port=p1,actions=mod_nw_dst:192.168.1.1,output:p2])
>
> +flow_s="\
> +eth(src=8a:bf:7e:2f:05:84,dst=0a:8f:39:4f:e0:73),eth_type(0x0800),\
> +ipv4(src=192.168.123.2,dst=192.168.123.1,proto=6,tos=0,ttl=64,frag=no),\
> +tcp(src=54392,dst=5201),tcp_flags(ack)"
> +
> +good_frame=$(echo ${flow_s} | ovstest test-odp hexify-keys)
> +
> # Check if no offload remains ok.
> AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> -])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}])
>
> # Checksum should change to 0x990 with ip_dst changed to 192.168.1.1
> # by the datapath while processing the packet.
> +flow_expected=$(echo ${flow_s} | sed 's/192.168.123.1/192.168.1.1/g')
> +good_expected=$(echo ${flow_expected} | ovstest test-odp hexify-keys)
> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> ])
>
> # Check if packets entering the datapath with csum offloading
> @@ -766,12 +772,9 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> # in the datapath and not by the netdev.
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> -])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}])
> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> ])
>
> # Check if packets entering the datapath with csum offloading
> @@ -779,36 +782,30 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> # by the datapath.
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}
> ])
> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> ])
>
> # Push a packet with bad csum and offloading disabled to check
> # if the datapath updates the csum, but does not fix the issue.
> +bad_frame=$(echo ${flow_s} | ovstest test-odp hexify-keys --bad-csum)
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f03c0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> -])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> -0a8f394fe0738abf7e2f058408004500003433e0400040060904c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> +bad_expected=$(echo ${flow_expected} | ovstest test-odp hexify-keys
> --bad-csum)
> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${bad_expected}
> ])
>
> # Push a packet with bad csum and offloading enabled to check
> # if the driver updates and fixes the csum.
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f03c0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> -])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> diff --git a/tests/test-odp.c b/tests/test-odp.c
> index 0ddfd4070..073e1031a 100644
> --- a/tests/test-odp.c
> +++ b/tests/test-odp.c
> @@ -19,6 +19,7 @@
> #include "odp-util.h"
> #include <stdio.h>
> #include "openvswitch/dynamic-string.h"
> +#include "dp-packet.h"
> #include "flow.h"
> #include "openvswitch/match.h"
> #include "openvswitch/ofpbuf.h"
> @@ -155,6 +156,96 @@ parse_actions(void)
> return 0;
> }
>
> +static int
> +hexify_keys(bool bad_csum)
> +{
> + int exit_code = 0;
> + struct ds in;
> +
> + uint8_t *l7 = NULL;
> + size_t l7_len = 0;
> +
> + char *error = NULL;
> + struct dp_packet *packet = NULL;
> +
> + ds_init(&in);
> + vlog_set_levels_from_string_assert("odp_util:console:dbg");
> + while (!ds_get_test_line(&in, stdin)) {
> + const char *line = ds_cstr(&in);
> +
> + struct flow flow;
> + struct ofpbuf odp_key;
> + struct ofpbuf odp_mask;
> + ofpbuf_init(&odp_key, 0);
> + ofpbuf_init(&odp_mask, 0);
> +
> + /* Handle trailing hex payload after a space char, if present. */
> + const char *first_space = strchr(line, ' ');
> + if (first_space) {
> + const char *hex_payload = first_space + 1;
> + struct dp_packet payload;
> + memset(&payload, 0, sizeof payload);
> + dp_packet_init(&payload, 0);
> + if (dp_packet_put_hex(&payload, hex_payload, NULL)[0] != '\0') {
> + dp_packet_uninit(&payload);
> + error = xstrdup("Trailing garbage in payload data");
> + goto next;
> + }
> + l7_len = dp_packet_size(&payload);
> + l7 = dp_packet_steal_data(&payload);
> + ds_truncate(&in, first_space - line);
> + }
> +
> + /* Parse flow string. */
> + if (odp_flow_from_string(
> + ds_cstr(&in), NULL, &odp_key, &odp_mask, &error)) {
> + goto next;
> + }
> + if (odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, &error)
> + == ODP_FIT_ERROR) {
> + goto next;
> + }
> +
> + /* Flow to binary. */
> + packet = dp_packet_new(0);
> + flow_compose(packet, &flow, l7, l7_len, bad_csum);
> +
> + /* Binary to hex string. */
> + struct ds result = DS_EMPTY_INITIALIZER;
> + for (int i = 0; i < dp_packet_size(packet); i++) {
> + uint8_t val = ((uint8_t *) dp_packet_data(packet))[i];
> + /* Don't use ds_put_hex because it adds 0x prefix as well as
> + * it doesn't guarantee even number of payload characters, which
> is
> + * important elsewhere (e.g. in `receive` command. */
> + ds_put_format(&result, "%02" PRIx8, val);
> + }
> +
> + puts(ds_cstr(&result));
> +next:
> + ds_destroy(&result);
> + if (error) {
> + printf("%s", error);
> + free(error);
> + exit_code = 1;
> + }
> +
> + dp_packet_delete(packet);
> + packet = NULL;
> + free(l7);
> + l7 = NULL;
> +
> + ofpbuf_uninit(&odp_mask);
> + ofpbuf_uninit(&odp_key);
> + ds_destroy(&in);
> +
> + if (exit_code) {
> + break;
> + }
> + }
> +
> + return exit_code;
> +}
> +
> static int
> parse_filter(char *filter_parse)
> {
> @@ -247,8 +338,14 @@ test_odp_main(int argc, char *argv[])
> exit_code = parse_actions();
> } else if (argc == 3 && !strcmp(argv[1], "parse-filter")) {
> exit_code =parse_filter(argv[2]);
> + } else if (argc == 2 && !strcmp(argv[1], "hexify-keys")) {
> + exit_code = hexify_keys(false);
> + } else if (argc == 3 && !strcmp(argv[1], "hexify-keys") &&
> + !strcmp(argv[2], "--bad-csum")) {
> + exit_code = hexify_keys(true);
> } else {
> - ovs_fatal(0, "usage: %s parse-keys | parse-wc-keys | parse-actions",
> argv[0]);
> + ovs_fatal(0, "usage: %s parse-keys | parse-wc-keys | parse-actions |
> "
> + "hexify-keys [--bad-csum]", argv[0]);
> }
>
> exit(exit_code);
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 24d0941cf..31da8c2d4 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -4989,7 +4989,7 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx)
Hmm. This is interesting. Looks like we have already a commnd with mostly
the same behavior, except that it is using OpenFlow format insted of datapath
flow format. Can we just use/extend ovs-ofctl compose-packet?
It supports pcap output, so we can use ovs-pcap tool to get the hex bytes
in a way they can be fed into netdev-dummy/receive. (We may want to extend
ovs-pcap to allow reading from stdin to make that processeasier.)
Instead of
eth(src=8a:bf:7e:2f:05:84,dst=0a:8f:39:4f:e0:73),eth_type(0x0800),\
ipv4(src=192.168.123.2,dst=192.168.123.1,proto=6,tos=0,ttl=64,frag=no),\
tcp(src=54392,dst=5201),tcp_flags(ack)"
We'll just need to write OpenFlow:
eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800,\
nw_src=192.168.123.2,nw_dst=192.168.123.1,nw_proto=6,nw_ttl=64,nw_frag=no,\
tp_src=8,tp_dst=80,tcp_flags=ack
or
tcp,eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,\
nw_src=192.168.123.2,nw_dst=192.168.123.1,nw_ttl=64,nw_frag=no,\
tp_src=8,tp_dst=80,tcp_flags=ack
What do you think? Is there a specific reson for using odp format over OF?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
