On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> 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?
>
>
Lol! And a nice catch.
I think it's fair to say that we don't need another tool for the same.
Though compose-packet will need some adjustment to fit the use case, namely:
- It currently generates “64 random byte payload” - we may want to make it
“0" or “hardcoded pattern” for test purposes.
- We would have to introduce a --bad-csum for the command (and any other
features we'd like in the future).
- Perhaps also stdin pipelining to simplify the integration in test cases,
as you suggested.
If this is the path you'd like to see taken here, we can definitely switch
to it. (I of course hope that would be the last switch of gears.)
Please confirm this is indeed how you see compose-packet evolve, and I will
send another iteration, now for compose-packet.
Ihar
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
