diff mbox series

[ovs-dev] dpif: Fix flow put debug message match content.

Message ID 6c2b9f49da7cb3d83d3736e356fd7c2d66749187.1725356236.git.echaudro@redhat.com
State Accepted
Commit 252ee0f182113b8826f6b6be838208864f43d159
Delegated to: Eelco Chaudron
Headers show
Series [ovs-dev] dpif: Fix flow put debug message match content. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Eelco Chaudron Sept. 3, 2024, 9:37 a.m. UTC
The odp_flow_format() function applies a wildcard mask when a
mask for a given key was not present. However, in the context of
installing flows in the datapath, the absence of a mask actually
indicates that the key should be ignored, meaning it should not
be masked at all.

To address this inconsistency, odp_flow_format() now includes an
option to skip formatting keys that are missing a mask.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/dpctl.c            |  4 ++--
 lib/dpif-netdev.c      |  6 +++---
 lib/dpif.c             |  4 ++--
 lib/odp-util.c         | 31 ++++++++++++++++++-------------
 lib/odp-util.h         |  2 +-
 lib/tnl-ports.c        |  2 +-
 ofproto/ofproto-dpif.c |  2 +-
 tests/test-odp.c       |  6 ++++--
 8 files changed, 32 insertions(+), 25 deletions(-)

Comments

Simon Horman Sept. 4, 2024, 10:38 a.m. UTC | #1
On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
> The odp_flow_format() function applies a wildcard mask when a
> mask for a given key was not present. However, in the context of
> installing flows in the datapath, the absence of a mask actually
> indicates that the key should be ignored, meaning it should not
> be masked at all.
> 
> To address this inconsistency, odp_flow_format() now includes an
> option to skip formatting keys that are missing a mask.

Hi Eelco,

Would you be able to provide some example output,
say as part of the commit message.

And, does this warrant a test?

> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

...
Eelco Chaudron Sept. 4, 2024, 11:40 a.m. UTC | #2
On 4 Sep 2024, at 12:38, Simon Horman wrote:

> On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
>> The odp_flow_format() function applies a wildcard mask when a
>> mask for a given key was not present. However, in the context of
>> installing flows in the datapath, the absence of a mask actually
>> indicates that the key should be ignored, meaning it should not
>> be masked at all.
>>
>> To address this inconsistency, odp_flow_format() now includes an
>> option to skip formatting keys that are missing a mask.
>
> Hi Eelco,
>
> Would you be able to provide some example output,
> say as part of the commit message.

I can add the following example to the commit message:

This was found during a debug session of the ‘datapath - ping between two
ports on cvlan’ test case. The log message was showing the following:

  put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
    skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
    eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
    vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
    vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x0800),
    ipv4(src=10.2.2.2,dst=10.2.2.1,proto=1,tos=0,ttl=64,frag=no),
    icmp(type=0,code=0))), actions:2

Where it should have shown the below:

  put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
    skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
    eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
    vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100)), actions:2

> And, does this warrant a test?

Good question, as this was a debug message I thought it would not
matter much. But if you feel it’s needed, I can try adding a
dp independent unit test.

>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>
> ...
Simon Horman Sept. 4, 2024, 1:54 p.m. UTC | #3
On Wed, Sep 04, 2024 at 01:40:52PM +0200, Eelco Chaudron wrote:
> 
> 
> On 4 Sep 2024, at 12:38, Simon Horman wrote:
> 
> > On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
> >> The odp_flow_format() function applies a wildcard mask when a
> >> mask for a given key was not present. However, in the context of
> >> installing flows in the datapath, the absence of a mask actually
> >> indicates that the key should be ignored, meaning it should not
> >> be masked at all.
> >>
> >> To address this inconsistency, odp_flow_format() now includes an
> >> option to skip formatting keys that are missing a mask.
> >
> > Hi Eelco,
> >
> > Would you be able to provide some example output,
> > say as part of the commit message.
> 
> I can add the following example to the commit message:
> 
> This was found during a debug session of the ‘datapath - ping between two
> ports on cvlan’ test case. The log message was showing the following:
> 
>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>     vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x0800),
>     ipv4(src=10.2.2.2,dst=10.2.2.1,proto=1,tos=0,ttl=64,frag=no),
>     icmp(type=0,code=0))), actions:2
> 
> Where it should have shown the below:
> 
>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100)), actions:2

Thanks,

Could you include that when applying the patch, or post a v2,
whichever you prefer?

> 
> > And, does this warrant a test?
> 
> Good question, as this was a debug message I thought it would not
> matter much. But if you feel it’s needed, I can try adding a
> dp independent unit test.

I think we can pass on the test if it is just a debugging message.

> 
> >>
> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> >
> > ...

Acked-by: Simon Horman <horms@ovn.org>
Eelco Chaudron Sept. 4, 2024, 1:56 p.m. UTC | #4
On 4 Sep 2024, at 15:54, Simon Horman wrote:

> On Wed, Sep 04, 2024 at 01:40:52PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 4 Sep 2024, at 12:38, Simon Horman wrote:
>>
>>> On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
>>>> The odp_flow_format() function applies a wildcard mask when a
>>>> mask for a given key was not present. However, in the context of
>>>> installing flows in the datapath, the absence of a mask actually
>>>> indicates that the key should be ignored, meaning it should not
>>>> be masked at all.
>>>>
>>>> To address this inconsistency, odp_flow_format() now includes an
>>>> option to skip formatting keys that are missing a mask.
>>>
>>> Hi Eelco,
>>>
>>> Would you be able to provide some example output,
>>> say as part of the commit message.
>>
>> I can add the following example to the commit message:
>>
>> This was found during a debug session of the ‘datapath - ping between two
>> ports on cvlan’ test case. The log message was showing the following:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>>     vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x0800),
>>     ipv4(src=10.2.2.2,dst=10.2.2.1,proto=1,tos=0,ttl=64,frag=no),
>>     icmp(type=0,code=0))), actions:2
>>
>> Where it should have shown the below:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100)), actions:2
>
> Thanks,
>
> Could you include that when applying the patch, or post a v2,
> whichever you prefer?

I'll add it when I apply, unless there are further comments. I'll wait until Monday.

>>> And, does this warrant a test?
>>
>> Good question, as this was a debug message I thought it would not
>> matter much. But if you feel it’s needed, I can try adding a
>> dp independent unit test.
>
> I think we can pass on the test if it is just a debugging message.
>
>>
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>
>>> ...
>
> Acked-by: Simon Horman <horms@ovn.org>
Eelco Chaudron Sept. 9, 2024, 9:20 a.m. UTC | #5
On 4 Sep 2024, at 15:54, Simon Horman wrote:

> On Wed, Sep 04, 2024 at 01:40:52PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 4 Sep 2024, at 12:38, Simon Horman wrote:
>>
>>> On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
>>>> The odp_flow_format() function applies a wildcard mask when a
>>>> mask for a given key was not present. However, in the context of
>>>> installing flows in the datapath, the absence of a mask actually
>>>> indicates that the key should be ignored, meaning it should not
>>>> be masked at all.
>>>>
>>>> To address this inconsistency, odp_flow_format() now includes an
>>>> option to skip formatting keys that are missing a mask.
>>>
>>> Hi Eelco,
>>>
>>> Would you be able to provide some example output,
>>> say as part of the commit message.
>>
>> I can add the following example to the commit message:
>>
>> This was found during a debug session of the ‘datapath - ping between two
>> ports on cvlan’ test case. The log message was showing the following:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>>     vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x0800),
>>     ipv4(src=10.2.2.2,dst=10.2.2.1,proto=1,tos=0,ttl=64,frag=no),
>>     icmp(type=0,code=0))), actions:2
>>
>> Where it should have shown the below:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>>     skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>>     eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>>     vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100)), actions:2
>
> Thanks,
>
> Could you include that when applying the patch, or post a v2,
> whichever you prefer?
>
>>
>>> And, does this warrant a test?
>>
>> Good question, as this was a debug message I thought it would not
>> matter much. But if you feel it’s needed, I can try adding a
>> dp independent unit test.
>
> I think we can pass on the test if it is just a debugging message.
>
>>
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>
>>> ...
>
> Acked-by: Simon Horman <horms@ovn.org>

Thanks Simon, applied to main.
Ilya Maximets Sept. 12, 2024, 12:58 p.m. UTC | #6
On 9/3/24 11:37, Eelco Chaudron wrote:
> The odp_flow_format() function applies a wildcard mask when a
> mask for a given key was not present. However, in the context of
> installing flows in the datapath, the absence of a mask actually
> indicates that the key should be ignored, meaning it should not
> be masked at all.
> 
> To address this inconsistency, odp_flow_format() now includes an
> option to skip formatting keys that are missing a mask.
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>  lib/dpctl.c            |  4 ++--
>  lib/dpif-netdev.c      |  6 +++---
>  lib/dpif.c             |  4 ++--
>  lib/odp-util.c         | 31 ++++++++++++++++++-------------
>  lib/odp-util.h         |  2 +-
>  lib/tnl-ports.c        |  2 +-
>  ofproto/ofproto-dpif.c |  2 +-
>  tests/test-odp.c       |  6 ++++--
>  8 files changed, 32 insertions(+), 25 deletions(-)


Hi, Eelco.

I got a notification from oss-fuzz that this change broke the build
of the fuzzing target:
  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524

Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: error: too few arguments to function call, expected 8, have 7
Step #3 - "compile-afl-address-x86_64":    83 |         odp_flow_format(odp_key.data, odp_key.size,
Step #3 - "compile-afl-address-x86_64":       |         ~~~~~~~~~~~~~~~
Step #3 - "compile-afl-address-x86_64":    84 |                         odp_mask.data, odp_mask.size, NULL, &out, false);
Step #3 - "compile-afl-address-x86_64":       |                                                                        ^
Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 'odp_flow_format' declared here
Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const struct nlattr *key, size_t key_len,
Step #3 - "compile-afl-address-x86_64":       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Step #3 - "compile-afl-address-x86_64":   172 |                      const struct nlattr *mask, size_t mask_len,
Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Step #3 - "compile-afl-address-x86_64":   173 |                      const struct hmap *portno_names, struct ds *,
Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Step #3 - "compile-afl-address-x86_64":   174 |                      bool verbose, bool skip_no_mask);
Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Step #3 - "compile-afl-address-x86_64": 1 error generated.
Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: tests/oss-fuzz/odp_target.o] Error 1
Step #3 - "compile-afl-address-x86_64": ********************************************************************************
Step #3 - "compile-afl-address-x86_64": Failed to build.
Step #3 - "compile-afl-address-x86_64": To reproduce, run:
Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image openvswitch
Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 openvswitch
Step #3 - "compile-afl-address-x86_64": ********************************************************************************
Finished Step #3 - "compile-afl-address-x86_64"
ERROR
ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1


Could you, please, send a fix?

Best regards, Ilya Maximets.
Eelco Chaudron Sept. 12, 2024, 1:47 p.m. UTC | #7
On 12 Sep 2024, at 14:58, Ilya Maximets wrote:

> On 9/3/24 11:37, Eelco Chaudron wrote:
>> The odp_flow_format() function applies a wildcard mask when a
>> mask for a given key was not present. However, in the context of
>> installing flows in the datapath, the absence of a mask actually
>> indicates that the key should be ignored, meaning it should not
>> be masked at all.
>>
>> To address this inconsistency, odp_flow_format() now includes an
>> option to skip formatting keys that are missing a mask.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>>  lib/dpctl.c            |  4 ++--
>>  lib/dpif-netdev.c      |  6 +++---
>>  lib/dpif.c             |  4 ++--
>>  lib/odp-util.c         | 31 ++++++++++++++++++-------------
>>  lib/odp-util.h         |  2 +-
>>  lib/tnl-ports.c        |  2 +-
>>  ofproto/ofproto-dpif.c |  2 +-
>>  tests/test-odp.c       |  6 ++++--
>>  8 files changed, 32 insertions(+), 25 deletions(-)
>
>
> Hi, Eelco.
>
> I got a notification from oss-fuzz that this change broke the build
> of the fuzzing target:
>  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>
> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: error: too few arguments to function call, expected 8, have 7
> Step #3 - "compile-afl-address-x86_64":    83 |         odp_flow_format(odp_key.data, odp_key.size,
> Step #3 - "compile-afl-address-x86_64":       |         ~~~~~~~~~~~~~~~
> Step #3 - "compile-afl-address-x86_64":    84 |                         odp_mask.data, odp_mask.size, NULL, &out, false);
> Step #3 - "compile-afl-address-x86_64":       |                                                                        ^
> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 'odp_flow_format' declared here
> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const struct nlattr *key, size_t key_len,
> Step #3 - "compile-afl-address-x86_64":       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Step #3 - "compile-afl-address-x86_64":   172 |                      const struct nlattr *mask, size_t mask_len,
> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Step #3 - "compile-afl-address-x86_64":   173 |                      const struct hmap *portno_names, struct ds *,
> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Step #3 - "compile-afl-address-x86_64":   174 |                      bool verbose, bool skip_no_mask);
> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Step #3 - "compile-afl-address-x86_64": 1 error generated.
> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: tests/oss-fuzz/odp_target.o] Error 1
> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
> Step #3 - "compile-afl-address-x86_64": Failed to build.
> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image openvswitch
> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 openvswitch
> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
> Finished Step #3 - "compile-afl-address-x86_64"
> ERROR
> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1
>
>
> Could you, please, send a fix?

Sent a patch… I learned something new today, as I was not aware OVS was part of this project. Should we do the build as part of the CI?

Cheers,

Eelco
Ilya Maximets Sept. 12, 2024, 2:07 p.m. UTC | #8
On 9/12/24 15:47, Eelco Chaudron wrote:
> 
> 
> On 12 Sep 2024, at 14:58, Ilya Maximets wrote:
> 
>> On 9/3/24 11:37, Eelco Chaudron wrote:
>>> The odp_flow_format() function applies a wildcard mask when a
>>> mask for a given key was not present. However, in the context of
>>> installing flows in the datapath, the absence of a mask actually
>>> indicates that the key should be ignored, meaning it should not
>>> be masked at all.
>>>
>>> To address this inconsistency, odp_flow_format() now includes an
>>> option to skip formatting keys that are missing a mask.
>>>
>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>> ---
>>>  lib/dpctl.c            |  4 ++--
>>>  lib/dpif-netdev.c      |  6 +++---
>>>  lib/dpif.c             |  4 ++--
>>>  lib/odp-util.c         | 31 ++++++++++++++++++-------------
>>>  lib/odp-util.h         |  2 +-
>>>  lib/tnl-ports.c        |  2 +-
>>>  ofproto/ofproto-dpif.c |  2 +-
>>>  tests/test-odp.c       |  6 ++++--
>>>  8 files changed, 32 insertions(+), 25 deletions(-)
>>
>>
>> Hi, Eelco.
>>
>> I got a notification from oss-fuzz that this change broke the build
>> of the fuzzing target:
>>  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>>
>> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
>> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: error: too few arguments to function call, expected 8, have 7
>> Step #3 - "compile-afl-address-x86_64":    83 |         odp_flow_format(odp_key.data, odp_key.size,
>> Step #3 - "compile-afl-address-x86_64":       |         ~~~~~~~~~~~~~~~
>> Step #3 - "compile-afl-address-x86_64":    84 |                         odp_mask.data, odp_mask.size, NULL, &out, false);
>> Step #3 - "compile-afl-address-x86_64":       |                                                                        ^
>> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 'odp_flow_format' declared here
>> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const struct nlattr *key, size_t key_len,
>> Step #3 - "compile-afl-address-x86_64":       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Step #3 - "compile-afl-address-x86_64":   172 |                      const struct nlattr *mask, size_t mask_len,
>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Step #3 - "compile-afl-address-x86_64":   173 |                      const struct hmap *portno_names, struct ds *,
>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Step #3 - "compile-afl-address-x86_64":   174 |                      bool verbose, bool skip_no_mask);
>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Step #3 - "compile-afl-address-x86_64": 1 error generated.
>> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: tests/oss-fuzz/odp_target.o] Error 1
>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>> Step #3 - "compile-afl-address-x86_64": Failed to build.
>> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image openvswitch
>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 openvswitch
>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>> Finished Step #3 - "compile-afl-address-x86_64"
>> ERROR
>> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1
>>
>>
>> Could you, please, send a fix?
> 
> Sent a patch…

Thanks!

> I learned something new today, as I was not aware OVS was part of this project.
> Should we do the build as part of the CI?

I don't remember how long does it take to build, but I remember it pulls
a pile of large container images that takes a while.  Not sure how that
will impact our CI times.

Best regards, Ilya Maximets.
Eelco Chaudron Sept. 12, 2024, 2:11 p.m. UTC | #9
On 12 Sep 2024, at 16:07, Ilya Maximets wrote:

> On 9/12/24 15:47, Eelco Chaudron wrote:
>>
>>
>> On 12 Sep 2024, at 14:58, Ilya Maximets wrote:
>>
>>> On 9/3/24 11:37, Eelco Chaudron wrote:
>>>> The odp_flow_format() function applies a wildcard mask when a
>>>> mask for a given key was not present. However, in the context of
>>>> installing flows in the datapath, the absence of a mask actually
>>>> indicates that the key should be ignored, meaning it should not
>>>> be masked at all.
>>>>
>>>> To address this inconsistency, odp_flow_format() now includes an
>>>> option to skip formatting keys that are missing a mask.
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>> ---
>>>>  lib/dpctl.c            |  4 ++--
>>>>  lib/dpif-netdev.c      |  6 +++---
>>>>  lib/dpif.c             |  4 ++--
>>>>  lib/odp-util.c         | 31 ++++++++++++++++++-------------
>>>>  lib/odp-util.h         |  2 +-
>>>>  lib/tnl-ports.c        |  2 +-
>>>>  ofproto/ofproto-dpif.c |  2 +-
>>>>  tests/test-odp.c       |  6 ++++--
>>>>  8 files changed, 32 insertions(+), 25 deletions(-)
>>>
>>>
>>> Hi, Eelco.
>>>
>>> I got a notification from oss-fuzz that this change broke the build
>>> of the fuzzing target:
>>> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>>>
>>> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
>>> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: error: too few arguments to function call, expected 8, have 7
>>> Step #3 - "compile-afl-address-x86_64":    83 |         odp_flow_format(odp_key.data, odp_key.size,
>>> Step #3 - "compile-afl-address-x86_64":       |         ~~~~~~~~~~~~~~~
>>> Step #3 - "compile-afl-address-x86_64":    84 |                         odp_mask.data, odp_mask.size, NULL, &out, false);
>>> Step #3 - "compile-afl-address-x86_64":       |                                                                        ^
>>> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 'odp_flow_format' declared here
>>> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const struct nlattr *key, size_t key_len,
>>> Step #3 - "compile-afl-address-x86_64":       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Step #3 - "compile-afl-address-x86_64":   172 |                      const struct nlattr *mask, size_t mask_len,
>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Step #3 - "compile-afl-address-x86_64":   173 |                      const struct hmap *portno_names, struct ds *,
>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Step #3 - "compile-afl-address-x86_64":   174 |                      bool verbose, bool skip_no_mask);
>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Step #3 - "compile-afl-address-x86_64": 1 error generated.
>>> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: tests/oss-fuzz/odp_target.o] Error 1
>>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>>> Step #3 - "compile-afl-address-x86_64": Failed to build.
>>> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image openvswitch
>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 openvswitch
>>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>>> Finished Step #3 - "compile-afl-address-x86_64"
>>> ERROR
>>> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1
>>>
>>>
>>> Could you, please, send a fix?
>>
>> Sent a patch…
>
> Thanks!
>
>> I learned something new today, as I was not aware OVS was part of this project.
>> Should we do the build as part of the CI?
>
> I don't remember how long does it take to build, but I remember it pulls
> a pile of large container images that takes a while.  Not sure how that
> will impact our CI times.

Yes, it takes quite some time to pull the containers and build. Maybe we can just build it standalone, let me investigate, as some of the tools are available on Fedora, and they might also be there on Ubuntu.
Ilya Maximets Sept. 12, 2024, 2:21 p.m. UTC | #10
On 9/12/24 16:11, Eelco Chaudron wrote:
> 
> 
> On 12 Sep 2024, at 16:07, Ilya Maximets wrote:
> 
>> On 9/12/24 15:47, Eelco Chaudron wrote:
>>>
>>>
>>> On 12 Sep 2024, at 14:58, Ilya Maximets wrote:
>>>
>>>> On 9/3/24 11:37, Eelco Chaudron wrote:
>>>>> The odp_flow_format() function applies a wildcard mask when a
>>>>> mask for a given key was not present. However, in the context of
>>>>> installing flows in the datapath, the absence of a mask actually
>>>>> indicates that the key should be ignored, meaning it should not
>>>>> be masked at all.
>>>>>
>>>>> To address this inconsistency, odp_flow_format() now includes an
>>>>> option to skip formatting keys that are missing a mask.
>>>>>
>>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>>> ---
>>>>>  lib/dpctl.c            |  4 ++--
>>>>>  lib/dpif-netdev.c      |  6 +++---
>>>>>  lib/dpif.c             |  4 ++--
>>>>>  lib/odp-util.c         | 31 ++++++++++++++++++-------------
>>>>>  lib/odp-util.h         |  2 +-
>>>>>  lib/tnl-ports.c        |  2 +-
>>>>>  ofproto/ofproto-dpif.c |  2 +-
>>>>>  tests/test-odp.c       |  6 ++++--
>>>>>  8 files changed, 32 insertions(+), 25 deletions(-)
>>>>
>>>>
>>>> Hi, Eelco.
>>>>
>>>> I got a notification from oss-fuzz that this change broke the build
>>>> of the fuzzing target:
>>>> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>>>>
>>>> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
>>>> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: error: too few arguments to function call, expected 8, have 7
>>>> Step #3 - "compile-afl-address-x86_64":    83 |         odp_flow_format(odp_key.data, odp_key.size,
>>>> Step #3 - "compile-afl-address-x86_64":       |         ~~~~~~~~~~~~~~~
>>>> Step #3 - "compile-afl-address-x86_64":    84 |                         odp_mask.data, odp_mask.size, NULL, &out, false);
>>>> Step #3 - "compile-afl-address-x86_64":       |                                                                        ^
>>>> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 'odp_flow_format' declared here
>>>> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const struct nlattr *key, size_t key_len,
>>>> Step #3 - "compile-afl-address-x86_64":       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Step #3 - "compile-afl-address-x86_64":   172 |                      const struct nlattr *mask, size_t mask_len,
>>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Step #3 - "compile-afl-address-x86_64":   173 |                      const struct hmap *portno_names, struct ds *,
>>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Step #3 - "compile-afl-address-x86_64":   174 |                      bool verbose, bool skip_no_mask);
>>>> Step #3 - "compile-afl-address-x86_64":       |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Step #3 - "compile-afl-address-x86_64": 1 error generated.
>>>> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: tests/oss-fuzz/odp_target.o] Error 1
>>>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>>>> Step #3 - "compile-afl-address-x86_64": Failed to build.
>>>> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
>>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image openvswitch
>>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 openvswitch
>>>> Step #3 - "compile-afl-address-x86_64": ********************************************************************************
>>>> Finished Step #3 - "compile-afl-address-x86_64"
>>>> ERROR
>>>> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1
>>>>
>>>>
>>>> Could you, please, send a fix?
>>>
>>> Sent a patch…
>>
>> Thanks!
>>
>>> I learned something new today, as I was not aware OVS was part of this project.
>>> Should we do the build as part of the CI?
>>
>> I don't remember how long does it take to build, but I remember it pulls
>> a pile of large container images that takes a while.  Not sure how that
>> will impact our CI times.
> 
> Yes, it takes quite some time to pull the containers and build. Maybe we can just
> build it standalone, let me investigate, as some of the tools are available on Fedora,
> and they might also be there on Ubuntu.
> 

Maybe we could contribute something to oss-fuzz, e.g. an argument for the
pull_images command to specify a language, so it doesn't pull images that
are not needed.  The java image for example is huge and we don't have
jave code in OVS.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index f764cf164..77bf4bf53 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -877,7 +877,7 @@  format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
         ds_put_cstr(ds, ", ");
     }
     odp_flow_format(f->key, f->key_len, f->mask, f->mask_len, ports, ds,
-                    dpctl_p->verbosity);
+                    dpctl_p->verbosity, false);
     ds_put_cstr(ds, ", ");
 
     dpif_flow_stats_format(&f->stats, ds);
@@ -2883,7 +2883,7 @@  dpctl_normalize_actions(int argc, const char *argv[],
 
     ds_clear(&s);
     odp_flow_format(keybuf.data, keybuf.size, NULL, 0, NULL,
-                    &s, dpctl_p->verbosity);
+                    &s, dpctl_p->verbosity, false);
     dpctl_print(dpctl_p, "input flow: %s\n", ds_cstr(&s));
 
     error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow, &error_s);
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f0594e5f5..f6d6d01cf 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3039,7 +3039,7 @@  log_netdev_flow_change(const struct dp_netdev_flow *flow,
     ds_put_cstr(&ds, " ");
     odp_flow_format(key_buf.data, key_buf.size,
                     mask_buf.data, mask_buf.size,
-                    NULL, &ds, false);
+                    NULL, &ds, false, true);
     if (old_actions) {
         ds_put_cstr(&ds, ", old_actions:");
         format_odp_actions(&ds, old_actions->actions, old_actions->size,
@@ -3859,7 +3859,7 @@  dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
 
                 ds_init(&s);
                 odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s,
-                                true);
+                                true, true);
                 VLOG_ERR("internal error parsing flow mask %s (%s)",
                 ds_cstr(&s), odp_key_fitness_to_string(fitness));
                 ds_destroy(&s);
@@ -3888,7 +3888,7 @@  dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
                 struct ds s;
 
                 ds_init(&s);
-                odp_flow_format(key, key_len, NULL, 0, NULL, &s, true);
+                odp_flow_format(key, key_len, NULL, 0, NULL, &s, true, false);
                 VLOG_ERR("internal error parsing flow key %s", ds_cstr(&s));
                 ds_destroy(&s);
             }
diff --git a/lib/dpif.c b/lib/dpif.c
index 245c8b5ee..a064f717f 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1805,7 +1805,7 @@  log_flow_message(const struct dpif *dpif, int error,
         odp_format_ufid(ufid, &ds);
         ds_put_cstr(&ds, " ");
     }
-    odp_flow_format(key, key_len, mask, mask_len, NULL, &ds, true);
+    odp_flow_format(key, key_len, mask, mask_len, NULL, &ds, true, true);
     if (stats) {
         ds_put_cstr(&ds, ", ");
         dpif_flow_stats_format(stats, &ds);
@@ -1906,7 +1906,7 @@  log_execute_message(const struct dpif *dpif,
         }
         ds_put_format(&ds, " on packet %s", packet);
         ds_put_format(&ds, " with metadata ");
-        odp_flow_format(md.data, md.size, NULL, 0, NULL, &ds, true);
+        odp_flow_format(md.data, md.size, NULL, 0, NULL, &ds, true, false);
         ds_put_format(&ds, " mtu %d", execute->mtu);
         vlog(module, error ? VLL_WARN : VLL_DBG, "%s", ds_cstr(&ds));
         ds_destroy(&ds);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 7c80a7e88..ee1868202 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -4269,7 +4269,7 @@  mask_empty(const struct nlattr *ma)
 static void
 format_odp_key_attr__(const struct nlattr *a, const struct nlattr *ma,
                       const struct hmap *portno_names, struct ds *ds,
-                      bool verbose)
+                      bool verbose, bool skip_no_mask)
 {
     enum ovs_key_attr attr = nl_attr_type(a);
     char namebuf[OVS_KEY_ATTR_BUFSIZE];
@@ -4285,10 +4285,10 @@  format_odp_key_attr__(const struct nlattr *a, const struct nlattr *ma,
         if (ma && nl_attr_get_size(ma) && nl_attr_get_size(a)) {
             odp_flow_format(nl_attr_get(a), nl_attr_get_size(a),
                             nl_attr_get(ma), nl_attr_get_size(ma), NULL, ds,
-                            verbose);
+                            verbose, skip_no_mask);
         } else if (nl_attr_get_size(a)) {
             odp_flow_format(nl_attr_get(a), nl_attr_get_size(a), NULL, 0, NULL,
-                            ds, verbose);
+                            ds, verbose, skip_no_mask);
         }
         break;
 
@@ -4596,7 +4596,7 @@  format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
 {
     if (check_attr_len(ds, a, ma, ovs_flow_key_attr_lens,
                         OVS_KEY_ATTR_MAX, false)) {
-        format_odp_key_attr__(a, ma, portno_names, ds, verbose);
+        format_odp_key_attr__(a, ma, portno_names, ds, verbose, false);
     }
 }
 
@@ -4710,13 +4710,16 @@  odp_format_ufid(const ovs_u128 *ufid, struct ds *ds)
 }
 
 /* Appends to 'ds' a string representation of the 'key_len' bytes of
- * OVS_KEY_ATTR_* attributes in 'key'. If non-null, additionally formats the
- * 'mask_len' bytes of 'mask' which apply to 'key'. If 'portno_names' is
- * non-null, translates odp port number to its name. */
+ * OVS_KEY_ATTR_* attributes in 'key'.  If non-null, additionally formats the
+ * 'mask_len' bytes of 'mask' which apply to 'key'.  If 'portno_names' is
+ * non-null, translates odp port number to its name.  If 'skip_no_mask' is set
+ * to true, OVS_KEY_ATTR_* entries without a mask will not be printed, even
+ * when verbose mode is 'true'. */
 void
 odp_flow_format(const struct nlattr *key, size_t key_len,
                 const struct nlattr *mask, size_t mask_len,
-                const struct hmap *portno_names, struct ds *ds, bool verbose)
+                const struct hmap *portno_names, struct ds *ds, bool verbose,
+                bool skip_no_mask)
 {
     if (key_len) {
         const struct nlattr *a;
@@ -4734,7 +4737,8 @@  odp_flow_format(const struct nlattr *key, size_t key_len,
                                                         attr_type)
                                        : NULL);
             if (!check_attr_len(ds, a, ma, ovs_flow_key_attr_lens,
-                                OVS_KEY_ATTR_MAX, false)) {
+                                OVS_KEY_ATTR_MAX, false)
+                || (skip_no_mask && !ma)) {
                 continue;
             }
 
@@ -4765,7 +4769,8 @@  odp_flow_format(const struct nlattr *key, size_t key_len,
                 if (!first_field) {
                     ds_put_char(ds, ',');
                 }
-                format_odp_key_attr__(a, ma, portno_names, ds, verbose);
+                format_odp_key_attr__(a, ma, portno_names, ds, verbose,
+                                      skip_no_mask);
                 first_field = false;
             } else if (attr_type == OVS_KEY_ATTR_ETHERNET
                        && !has_packet_type_key) {
@@ -4836,7 +4841,7 @@  void
 odp_flow_key_format(const struct nlattr *key,
                     size_t key_len, struct ds *ds)
 {
-    odp_flow_format(key, key_len, NULL, 0, NULL, ds, true);
+    odp_flow_format(key, key_len, NULL, 0, NULL, ds, true, false);
 }
 
 static bool
@@ -7747,7 +7752,7 @@  parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
             struct ds s;
 
             ds_init(&s);
-            odp_flow_format(key, key_len, NULL, 0, NULL, &s, true);
+            odp_flow_format(key, key_len, NULL, 0, NULL, &s, true, false);
             VLOG_ERR("internal error parsing flow key %s (%s)",
                      ds_cstr(&s), odp_key_fitness_to_string(fitness));
             ds_destroy(&s);
@@ -7767,7 +7772,7 @@  parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
 
             ds_init(&s);
             odp_flow_format(key, key_len, mask, mask_len, NULL, &s,
-                            true);
+                            true, true);
             VLOG_ERR("internal error parsing flow mask %s (%s)",
                      ds_cstr(&s), odp_key_fitness_to_string(fitness));
             ds_destroy(&s);
diff --git a/lib/odp-util.h b/lib/odp-util.h
index e454dbfcd..339ffc14f 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -171,7 +171,7 @@  void odp_format_ufid(const ovs_u128 *ufid, struct ds *);
 void odp_flow_format(const struct nlattr *key, size_t key_len,
                      const struct nlattr *mask, size_t mask_len,
                      const struct hmap *portno_names, struct ds *,
-                     bool verbose);
+                     bool verbose, bool skip_no_mask);
 void odp_flow_key_format(const struct nlattr *, size_t, struct ds *);
 int odp_flow_from_string(const char *s, const struct simap *port_names,
                          struct ofpbuf *, struct ofpbuf *, char **errorp);
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index bb0b0b0c5..a1dec89d4 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -347,7 +347,7 @@  tnl_port_show_v(struct ds *ds)
         mask_len = buf.size;
 
         /* build string. */
-        odp_flow_format(key, key_len, mask, mask_len, NULL, ds, false);
+        odp_flow_format(key, key_len, mask, mask_len, NULL, ds, false, false);
         ds_put_format(ds, "\n");
     }
 }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d3c353b9d..4659d8a3b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6874,7 +6874,7 @@  ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
             ds_put_cstr(&ds, " ");
         }
         odp_flow_format(f.key, f.key_len, f.mask, f.mask_len,
-                        portno_names, &ds, verbosity);
+                        portno_names, &ds, verbosity, false);
         ds_put_cstr(&ds, ", ");
         dpif_flow_stats_format(&f.stats, &ds);
         ds_put_cstr(&ds, ", actions:");
diff --git a/tests/test-odp.c b/tests/test-odp.c
index 0ddfd4070..1e64241ad 100644
--- a/tests/test-odp.c
+++ b/tests/test-odp.c
@@ -105,7 +105,8 @@  parse_keys(bool wc_keys)
         ds_init(&out);
         if (wc_keys) {
             odp_flow_format(odp_key.data, odp_key.size,
-                            odp_mask.data, odp_mask.size, NULL, &out, false);
+                            odp_mask.data, odp_mask.size, NULL, &out, false,
+                            false);
         } else {
             odp_flow_key_format(odp_key.data, odp_key.size, &out);
         }
@@ -219,7 +220,8 @@  parse_filter(char *filter_parse)
         /* Convert odp_key to string. */
         ds_init(&out);
         odp_flow_format(odp_key.data, odp_key.size,
-                        odp_mask.data, odp_mask.size, NULL, &out, false);
+                        odp_mask.data, odp_mask.size, NULL, &out, false,
+                        false);
         puts(ds_cstr(&out));
         ds_destroy(&out);