| Message ID | 20230807164551.553365-8-amorenoz@redhat.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | openvswitch: add drop reasons | expand |
Adrian Moreno <amorenoz@redhat.com> writes: > Make ovs-dpctl.py support explicit drops as: > "drop" -> implicit empty-action drop > "drop(0)" -> explicit non-error action drop I also suggest a test in netlink_checks to make sure drop can't be followed by additional actions. Something like: 3,drop(0),2 which should generate a NL message that has the drop attribute with additional action data following it. > "drop(42)" -> explicit error action drop > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > .../selftests/net/openvswitch/openvswitch.sh | 25 +++++++++++++++++++ > .../selftests/net/openvswitch/ovs-dpctl.py | 21 ++++++++++++---- > 2 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh > index a10c345f40ef..c568ba1b7900 100755 > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh > @@ -217,6 +217,31 @@ test_drop_reason() { > return 1 > fi > > + # Drop UDP 6000 traffic with an explicit action and an error code. > + ovs_add_flow "test_drop_reason" dropreason \ > + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=6000)" \ > + 'drop(42)' > + # Drop UDP 7000 traffic with an explicit action with no error code. > + ovs_add_flow "test_drop_reason" dropreason \ > + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=7000)" \ > + 'drop(0)' > + > + ovs_drop_record_and_run \ > + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000 > + ovs_drop_reason_count 0x30004 # OVS_DROP_EXPLICIT_ACTION_ERROR > + if [[ "$?" -ne "1" ]]; then > + info "Did not detect expected explicit error drops: $?" > + return 1 > + fi > + > + ovs_drop_record_and_run \ > + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000 > + ovs_drop_reason_count 0x30003 # OVS_DROP_EXPLICIT_ACTION > + if [[ "$?" -ne "1" ]]; then > + info "Did not detect expected explicit drops: $?" > + return 1 > + fi > + > return 0 > } > > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > index 5fee330050c2..912dc8c49085 100644 > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > @@ -449,7 +449,7 @@ class ovsactions(nla): > elif field[0] == "OVS_ACTION_ATTR_TRUNC": > print_str += "trunc(%d)" % int(self.get_attr(field[0])) > elif field[0] == "OVS_ACTION_ATTR_DROP": > - print_str += "drop" > + print_str += "drop(%d)" % int(self.get_attr(field[0])) > elif field[1] == "flag": > if field[0] == "OVS_ACTION_ATTR_CT_CLEAR": > print_str += "ct_clear" > @@ -471,10 +471,21 @@ class ovsactions(nla): > while len(actstr) != 0: > parsed = False > if actstr.startswith("drop"): > - # for now, drops have no explicit action, so we > - # don't need to set any attributes. The final > - # act of the processing chain will just drop the packet > - return > + # If no reason is provided, the implicit drop is used (i.e no > + # action). If some reason is given, an explicit action is used. > + actstr, reason = parse_extract_field( > + actstr, > + "drop(", > + "([0-9]+)", > + lambda x: int(x, 0), > + False, > + None, > + ) > + if reason is not None: > + self["attrs"].append(["OVS_ACTION_ATTR_DROP", reason]) > + parsed = True > + else: > + return > > elif parse_starts_block(actstr, "^(\d+)", False, True): > actstr, output = parse_extract_field(
On 8/8/23 17:02, Aaron Conole wrote: > Adrian Moreno <amorenoz@redhat.com> writes: > >> Make ovs-dpctl.py support explicit drops as: >> "drop" -> implicit empty-action drop >> "drop(0)" -> explicit non-error action drop > > I also suggest a test in netlink_checks to make sure drop can't be > followed by additional actions. Something like: > > 3,drop(0),2 > > which should generate a NL message that has the drop attribute with > additional action data following it. Ack, will do. The check I've added in flow_netlink.c does not include any custom message. Just returning -EINVAL in __ovs_nla_copy_actions() ends up printing "Flow actions may not be safe on all matching packets" to dmesg. Maybe it's too generic or even misleading in some cases but I saw other action verifications do the same so I thought it might be kind of expected at this point. Do you think a custom message (in addition to the generic one) is needed? Thanks. -- Adrián > >> "drop(42)" -> explicit error action drop >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> .../selftests/net/openvswitch/openvswitch.sh | 25 +++++++++++++++++++ >> .../selftests/net/openvswitch/ovs-dpctl.py | 21 ++++++++++++---- >> 2 files changed, 41 insertions(+), 5 deletions(-) >> >> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh >> index a10c345f40ef..c568ba1b7900 100755 >> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh >> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh >> @@ -217,6 +217,31 @@ test_drop_reason() { >> return 1 >> fi >> >> + # Drop UDP 6000 traffic with an explicit action and an error code. >> + ovs_add_flow "test_drop_reason" dropreason \ >> + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=6000)" \ >> + 'drop(42)' >> + # Drop UDP 7000 traffic with an explicit action with no error code. >> + ovs_add_flow "test_drop_reason" dropreason \ >> + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=7000)" \ >> + 'drop(0)' >> + >> + ovs_drop_record_and_run \ >> + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000 >> + ovs_drop_reason_count 0x30004 # OVS_DROP_EXPLICIT_ACTION_ERROR >> + if [[ "$?" -ne "1" ]]; then >> + info "Did not detect expected explicit error drops: $?" >> + return 1 >> + fi >> + >> + ovs_drop_record_and_run \ >> + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000 >> + ovs_drop_reason_count 0x30003 # OVS_DROP_EXPLICIT_ACTION >> + if [[ "$?" -ne "1" ]]; then >> + info "Did not detect expected explicit drops: $?" >> + return 1 >> + fi >> + >> return 0 >> } >> >> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >> index 5fee330050c2..912dc8c49085 100644 >> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >> @@ -449,7 +449,7 @@ class ovsactions(nla): >> elif field[0] == "OVS_ACTION_ATTR_TRUNC": >> print_str += "trunc(%d)" % int(self.get_attr(field[0])) >> elif field[0] == "OVS_ACTION_ATTR_DROP": >> - print_str += "drop" >> + print_str += "drop(%d)" % int(self.get_attr(field[0])) >> elif field[1] == "flag": >> if field[0] == "OVS_ACTION_ATTR_CT_CLEAR": >> print_str += "ct_clear" >> @@ -471,10 +471,21 @@ class ovsactions(nla): >> while len(actstr) != 0: >> parsed = False >> if actstr.startswith("drop"): >> - # for now, drops have no explicit action, so we >> - # don't need to set any attributes. The final >> - # act of the processing chain will just drop the packet >> - return >> + # If no reason is provided, the implicit drop is used (i.e no >> + # action). If some reason is given, an explicit action is used. >> + actstr, reason = parse_extract_field( >> + actstr, >> + "drop(", >> + "([0-9]+)", >> + lambda x: int(x, 0), >> + False, >> + None, >> + ) >> + if reason is not None: >> + self["attrs"].append(["OVS_ACTION_ATTR_DROP", reason]) >> + parsed = True >> + else: >> + return >> >> elif parse_starts_block(actstr, "^(\d+)", False, True): >> actstr, output = parse_extract_field( >
Adrian Moreno <amorenoz@redhat.com> writes: > On 8/8/23 17:02, Aaron Conole wrote: >> Adrian Moreno <amorenoz@redhat.com> writes: >> >>> Make ovs-dpctl.py support explicit drops as: >>> "drop" -> implicit empty-action drop >>> "drop(0)" -> explicit non-error action drop >> I also suggest a test in netlink_checks to make sure drop can't be >> followed by additional actions. Something like: >> 3,drop(0),2 >> which should generate a NL message that has the drop attribute with >> additional action data following it. > > Ack, will do. > > The check I've added in flow_netlink.c does not include any custom > message. Just returning -EINVAL in __ovs_nla_copy_actions() ends up > printing "Flow actions may not be safe on all matching packets" to > dmesg. Maybe it's too generic or even misleading in some cases but I > saw other action verifications do the same so I thought it might be > kind of expected at this point. > > Do you think a custom message (in addition to the generic one) is needed? I think the message is fine. There could be a bigger effort at some point to try and do per-attribute rejection messages. > Thanks. > -- > Adrián > >> >>> "drop(42)" -> explicit error action drop >>> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> .../selftests/net/openvswitch/openvswitch.sh | 25 +++++++++++++++++++ >>> .../selftests/net/openvswitch/ovs-dpctl.py | 21 ++++++++++++---- >>> 2 files changed, 41 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh >>> index a10c345f40ef..c568ba1b7900 100755 >>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh >>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh >>> @@ -217,6 +217,31 @@ test_drop_reason() { >>> return 1 >>> fi >>> + # Drop UDP 6000 traffic with an explicit action and an error >>> code. >>> + ovs_add_flow "test_drop_reason" dropreason \ >>> + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=6000)" \ >>> + 'drop(42)' >>> + # Drop UDP 7000 traffic with an explicit action with no error code. >>> + ovs_add_flow "test_drop_reason" dropreason \ >>> + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=7000)" \ >>> + 'drop(0)' >>> + >>> + ovs_drop_record_and_run \ >>> + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000 >>> + ovs_drop_reason_count 0x30004 # OVS_DROP_EXPLICIT_ACTION_ERROR >>> + if [[ "$?" -ne "1" ]]; then >>> + info "Did not detect expected explicit error drops: $?" >>> + return 1 >>> + fi >>> + >>> + ovs_drop_record_and_run \ >>> + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000 >>> + ovs_drop_reason_count 0x30003 # OVS_DROP_EXPLICIT_ACTION >>> + if [[ "$?" -ne "1" ]]; then >>> + info "Did not detect expected explicit drops: $?" >>> + return 1 >>> + fi >>> + >>> return 0 >>> } >>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >>> index 5fee330050c2..912dc8c49085 100644 >>> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py >>> @@ -449,7 +449,7 @@ class ovsactions(nla): >>> elif field[0] == "OVS_ACTION_ATTR_TRUNC": >>> print_str += "trunc(%d)" % int(self.get_attr(field[0])) >>> elif field[0] == "OVS_ACTION_ATTR_DROP": >>> - print_str += "drop" >>> + print_str += "drop(%d)" % int(self.get_attr(field[0])) >>> elif field[1] == "flag": >>> if field[0] == "OVS_ACTION_ATTR_CT_CLEAR": >>> print_str += "ct_clear" >>> @@ -471,10 +471,21 @@ class ovsactions(nla): >>> while len(actstr) != 0: >>> parsed = False >>> if actstr.startswith("drop"): >>> - # for now, drops have no explicit action, so we >>> - # don't need to set any attributes. The final >>> - # act of the processing chain will just drop the packet >>> - return >>> + # If no reason is provided, the implicit drop is used (i.e no >>> + # action). If some reason is given, an explicit action is used. >>> + actstr, reason = parse_extract_field( >>> + actstr, >>> + "drop(", >>> + "([0-9]+)", >>> + lambda x: int(x, 0), >>> + False, >>> + None, >>> + ) >>> + if reason is not None: >>> + self["attrs"].append(["OVS_ACTION_ATTR_DROP", reason]) >>> + parsed = True >>> + else: >>> + return >>> elif parse_starts_block(actstr, "^(\d+)", False, >>> True): >>> actstr, output = parse_extract_field( >>
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh index a10c345f40ef..c568ba1b7900 100755 --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh @@ -217,6 +217,31 @@ test_drop_reason() { return 1 fi + # Drop UDP 6000 traffic with an explicit action and an error code. + ovs_add_flow "test_drop_reason" dropreason \ + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=6000)" \ + 'drop(42)' + # Drop UDP 7000 traffic with an explicit action with no error code. + ovs_add_flow "test_drop_reason" dropreason \ + "in_port(1),eth(),eth_type(0x0800),ipv4(src=172.31.110.10,proto=17),udp(dst=7000)" \ + 'drop(0)' + + ovs_drop_record_and_run \ + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000 + ovs_drop_reason_count 0x30004 # OVS_DROP_EXPLICIT_ACTION_ERROR + if [[ "$?" -ne "1" ]]; then + info "Did not detect expected explicit error drops: $?" + return 1 + fi + + ovs_drop_record_and_run \ + "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000 + ovs_drop_reason_count 0x30003 # OVS_DROP_EXPLICIT_ACTION + if [[ "$?" -ne "1" ]]; then + info "Did not detect expected explicit drops: $?" + return 1 + fi + return 0 } diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py index 5fee330050c2..912dc8c49085 100644 --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py @@ -449,7 +449,7 @@ class ovsactions(nla): elif field[0] == "OVS_ACTION_ATTR_TRUNC": print_str += "trunc(%d)" % int(self.get_attr(field[0])) elif field[0] == "OVS_ACTION_ATTR_DROP": - print_str += "drop" + print_str += "drop(%d)" % int(self.get_attr(field[0])) elif field[1] == "flag": if field[0] == "OVS_ACTION_ATTR_CT_CLEAR": print_str += "ct_clear" @@ -471,10 +471,21 @@ class ovsactions(nla): while len(actstr) != 0: parsed = False if actstr.startswith("drop"): - # for now, drops have no explicit action, so we - # don't need to set any attributes. The final - # act of the processing chain will just drop the packet - return + # If no reason is provided, the implicit drop is used (i.e no + # action). If some reason is given, an explicit action is used. + actstr, reason = parse_extract_field( + actstr, + "drop(", + "([0-9]+)", + lambda x: int(x, 0), + False, + None, + ) + if reason is not None: + self["attrs"].append(["OVS_ACTION_ATTR_DROP", reason]) + parsed = True + else: + return elif parse_starts_block(actstr, "^(\d+)", False, True): actstr, output = parse_extract_field(
Make ovs-dpctl.py support explicit drops as: "drop" -> implicit empty-action drop "drop(0)" -> explicit non-error action drop "drop(42)" -> explicit error action drop Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- .../selftests/net/openvswitch/openvswitch.sh | 25 +++++++++++++++++++ .../selftests/net/openvswitch/ovs-dpctl.py | 21 ++++++++++++---- 2 files changed, 41 insertions(+), 5 deletions(-)