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 |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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> ...
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> > > ...
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>
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>
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.
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.
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
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.
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.
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 --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);
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(-)