diff mbox series

[ovs-dev,v2,6/6] dpctl: Do not allow out of range values in ct-set-limits.

Message ID 20240930205034.65484-6-pvalerio@redhat.com
State Accepted
Commit 63a4b4d0f054b2a59fb82b8d825ca3e7dd5f28e6
Delegated to: aaron conole
Headers show
Series [ovs-dev,v2,1/6] conntrack: Correctly annotate conntrack member. | expand

Checks

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

Commit Message

Paolo Valerio Sept. 30, 2024, 8:50 p.m. UTC 
The ovs_scan() doesn't enforce in-range values and so
lsbits are stored in case of out-of-range or negative values.

This way negative or values greater than MAX_UINT32 for "default" are
all accepted in dpctl_ct_set_limits(), but they will eventually be
casted to uint32_t, whereas for zones all the values above are
considered invalid.

Align their behaviors and extend the tests for checking values out of
the range.

Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
 lib/dpctl.c             |  5 +++--
 tests/system-traffic.at | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

Comments

Eelco Chaudron Oct. 1, 2024, 7:16 a.m. UTC | #1 
On 30 Sep 2024, at 22:50, Paolo Valerio wrote:

> The ovs_scan() doesn't enforce in-range values and so
> lsbits are stored in case of out-of-range or negative values.
>
> This way negative or values greater than MAX_UINT32 for "default" are
> all accepted in dpctl_ct_set_limits(), but they will eventually be
> casted to uint32_t, whereas for zones all the values above are
> considered invalid.
>
> Align their behaviors and extend the tests for checking values out of
> the range.
>
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>

Recheck-request: github-robot
diff mbox series

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 77bf4bf53..2a700f24a 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2169,8 +2169,8 @@  dpctl_ct_set_limits(int argc, const char *argv[],
     struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits);
     int i =  dp_arg_exists(argc, argv) ? 2 : 1;
     struct ds ds = DS_EMPTY_INITIALIZER;
+    unsigned long long default_limit;
     struct dpif *dpif = NULL;
-    uint32_t default_limit;
     int error;
 
     if (i >= argc) {
@@ -2186,7 +2186,8 @@  dpctl_ct_set_limits(int argc, const char *argv[],
 
     /* Parse default limit */
     if (!strncmp(argv[i], "default=", 8)) {
-        if (ovs_scan(argv[i], "default=%"SCNu32, &default_limit)) {
+        if (str_to_ullong(argv[i] + 8, 10, &default_limit) &&
+            default_limit <= UINT32_MAX) {
             ct_dpif_push_zone_limit(&zone_limits, OVS_ZONE_LIMIT_DEFAULT_ZONE,
                                     default_limit, 0);
             i++;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index fe115d92b..bcb08b0e8 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5686,12 +5686,54 @@  priority=100,in_port=2,udp,action=ct(zone=3,commit),1
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
+dnl Test values out of range for the default limit.
+dnl Try to set a negative value.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=-1], [2], [ignore], [dnl
+ovs-vswitchd: invalid default limit (Invalid argument)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+dnl Try to set UINT32_MAX.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=4294967296], [2], [ignore], [dnl
+ovs-vswitchd: invalid default limit (Invalid argument)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+dnl Same range checks for zones.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=-1], [2], [ignore], [dnl
+ovs-vswitchd: failed to parse field limit (Invalid argument)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=4294967296], [2], [ignore], [dnl
+ovs-vswitchd: failed to parse field limit (Invalid argument)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+dnl Double check no limits have been applied.
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [],[dnl
+default limit=0
+])
+
 m4_define([UDP_PKT], [m4_join([,],
   [eth_src=50:54:00:00:00:0$1,eth_dst=50:54:00:00:00:0$2,dl_type=0x0800],
   [nw_src=10.1.1.$1,nw_dst=10.1.1.$2],
   [nw_proto=17,nw_ttl=64,nw_frag=no],
   [udp_src=1,udp_dst=$3])])
 
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=0])
+pkt=$(ovs-ofctl compose-packet --bare "UDP_PKT([1], [2], [2])")
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=${pkt} actions=resubmit(,0)"])
+
+dnl Double check the zl entry exists but no connection was added.
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [],[dnl
+default limit=0
+zone=1,limit=0,count=0
+])
+
+dnl Remove limit for zone=1.
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1])
+
 AT_CHECK([ovs-appctl dpctl/ct-set-limits default=3])
 AT_CHECK([ovs-appctl dpctl/ct-get-limits], [],[dnl
 default limit=3