Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ihar Hrachyshka]

On Fri, Oct 20, 2023 at 6:53 PM Ilya Maximets  wrote:

> On 10/21/23 00:40, Ihar Hrachyshka wrote:
> > On Fri, Oct 20, 2023 at 6:02 PM Ilya Maximets  > wrote:
> >
> > On 10/20/23 15:15, Ihar Hrachyshka wrote:
> > > On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets   >> 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  <
> http://dpif-netdev.at > | 45
> 
> > > >  tests/test-odp.c | 99
> +++-
> > > >  utilities/ovs-ofctl.c|  2 +-
> > > >  8 files changed, 140 insertions(+), 34 deletions(-)
> >
> > 
> >
> > > > 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.
> >
> > It supports passing L7 payload as a second argument, AFAICT.
> >
> >
> > Yes; the difference is when the payload is NOT passed, then it generates
> a random 64 byte payload. It may be fine I guess, just a difference in
> behavior. I am only concerned that perhaps it may be harder to match
> against a random payload than against a missing or hardcoded one in some
> scenarios.
>
> It's not random.  It puts incrementing bytes starting from 0.
>

Indeed. I misinterpreted the comment to flow_compose saying that

   *- If 'l7_len' is nonzero and 'l7' is null, an arbitrary payload
'l7_len'
   *  bytes long is included.

I guess arbitrary != random.


> >
> > (It may also be 

Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ilya Maximets]

On 10/21/23 00:40, Ihar Hrachyshka wrote:
> On Fri, Oct 20, 2023 at 6:02 PM Ilya Maximets  > wrote:
> 
> On 10/20/23 15:15, Ihar Hrachyshka wrote:
> > On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets    >> 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    >>
> >     > ---
> >     > 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/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.
> 
> It supports passing L7 payload as a second argument, AFAICT.
> 
> 
> Yes; the difference is when the payload is NOT passed, then it generates a 
> random 64 byte payload. It may be fine I guess, just a difference in 
> behavior. I am only concerned that perhaps it may be harder to match against 
> a random payload than against a missing or hardcoded one in some scenarios.

It's not random.  It puts incrementing bytes starting from 0.

> 
> (It may also be impossible to generate a "bare" IP packet with an empty 
> payload - which is a valid IP packet.)

Command seems to accept an empty string as a payload just fine.

>  
> 
> 
> > - We would have to introduce a --bad-csum for the command (and any 
> other features we'd like in the future).
> 
> Seems fine to me.  This one is primarily a testing command, so should

Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ihar Hrachyshka]

On Fri, Oct 20, 2023 at 6:02 PM Ilya Maximets  wrote:

> On 10/20/23 15:15, Ihar Hrachyshka wrote:
> > On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets  > 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/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.
>
> It supports passing L7 payload as a second argument, AFAICT.
>

Yes; the difference is when the payload is NOT passed, then it generates a
random 64 byte payload. It may be fine I guess, just a difference in
behavior. I am only concerned that perhaps it may be harder to match
against a random payload than against a missing or hardcoded one in some
scenarios.

(It may also be impossible to generate a "bare" IP packet with an empty
payload - which is a valid IP packet.)


>
> > - We would have to introduce a --bad-csum for the command (and any other
> features we'd like in the future).
>
> Seems fine to me.  This one is primarily a testing command, so should
> be no problem extending it.
>
> > - Perhaps also stdin pipelining to simplify the integration in test
> cases, as you suggested.
>
> Yeah, at least ovs-pcap could really use ability to read from stdin.
>
> >
> > 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.)
>
> Can't guarantee anything. :D
>
> >
> > Please confirm this is indeed how you see compose-packet evolve, and I
> will send another iteration, now for compose-packet.
>
> At this point in time, I think it's a reasonable approach.
>
> There is also a parse-packet command. It's only used in fuzzing today.
> We could teach it to parse from hex/pcap if needed, I guess.
>
> Best regards, 

Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ilya Maximets]

On 10/20/23 15:15, Ihar Hrachyshka wrote:
> On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets  > 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  >
> > ---
> > 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/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.

It supports passing L7 payload as a second argument, AFAICT.

> - We would have to introduce a --bad-csum for the command (and any other 
> features we'd like in the future).

Seems fine to me.  This one is primarily a testing command, so should
be no problem extending it.

> - Perhaps also stdin pipelining to simplify the integration in test cases, as 
> you suggested.

Yeah, at least ovs-pcap could really use ability to read from stdin.

> 
> 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.)

Can't guarantee anything. :D

> 
> Please confirm this is indeed how you see compose-packet evolve, and I will 
> send another iteration, now for compose-packet.

At this point in time, I think it's a reasonable approach.

There is also a parse-packet command. It's only used in fuzzing today.
We could teach it to parse from hex/pcap if needed, I guess.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ihar Hrachyshka]

On Thu, Oct 19, 2023 at 6:29 PM Ilya Maximets  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 
> > ---
> > 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(_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
> > +++ 

Re: [ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ilya Maximets]

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 
> ---
> 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(_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);
> +

[ovs-dev] [PATCH v3] Implement ovstest test-odp hexify-keys [--bad-csum].

[Ihar Hrachyshka]

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 
---
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(_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
---