diff mbox series

[ovs-dev] ofctrl-seqno: Do not truncate the last acked value

Message ID 20230713082335.386633-1-amusil@redhat.com
State Accepted
Headers show
Series [ovs-dev] ofctrl-seqno: Do not truncate the last acked value | expand

Checks

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

Commit Message

Ales Musil July 13, 2023, 8:23 a.m. UTC 
The requested and acked seqno values are allowed
to be uint64_t, however the values that were added
to the hmap were truncated to uint32_t. This would
lead to loss of information when the value is bigger.

Use uin64_t for the function signatures and for the
hash to prevent truncation.

Reported-at: https://bugzilla.redhat.com/2074019
Signed-off-by: Ales Musil <amusil@redhat.com>
---
 controller/ofctrl-seqno.c      | 10 +++++-----
 controller/ofctrl-seqno.h      |  2 +-
 controller/test-ofctrl-seqno.c |  9 +++++----
 tests/ovn-ofctrl-seqno.at      | 33 +++++++++++++++++++++++++++++++++
 tests/test-utils.c             | 17 +++++++++++++++++
 tests/test-utils.h             |  2 ++
 6 files changed, 63 insertions(+), 10 deletions(-)

Comments

Dumitru Ceara July 17, 2023, 1:51 p.m. UTC | #1 
On 7/13/23 10:23, Ales Musil wrote:
> The requested and acked seqno values are allowed
> to be uint64_t, however the values that were added
> to the hmap were truncated to uint32_t. This would
> lead to loss of information when the value is bigger.
> 
> Use uin64_t for the function signatures and for the
> hash to prevent truncation.
> 

Thanks for the fix, Ales!

Fixes: c93c626248c1 ("controller: Implement a generic barrier based on ofctrl cur_cfg sync.")

> Reported-at: https://bugzilla.redhat.com/2074019
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---

I applied this to main and backported it to all branches
down to 22.03.

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/controller/ofctrl-seqno.c b/controller/ofctrl-seqno.c
index 923de478a..95373aa57 100644
--- a/controller/ofctrl-seqno.c
+++ b/controller/ofctrl-seqno.c
@@ -59,7 +59,7 @@  static struct ofctrl_seqno_state *ofctrl_seqno_states;
 static void ofctrl_acked_seqnos_init(struct ofctrl_acked_seqnos *seqnos,
                                      uint64_t last_acked);
 static void ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos,
-                                    uint32_t val);
+                                    uint64_t val);
 
 /* ofctrl_seqno_update related static function prototypes. */
 static void ofctrl_seqno_update_create__(size_t seqno_type, uint64_t req_cfg);
@@ -106,11 +106,11 @@  ofctrl_acked_seqnos_destroy(struct ofctrl_acked_seqnos *seqnos)
 /* Returns true if 'val' is one of the acked sequence numbers in 'seqnos'. */
 bool
 ofctrl_acked_seqnos_contains(const struct ofctrl_acked_seqnos *seqnos,
-                             uint32_t val)
+                             uint64_t val)
 {
     struct ofctrl_ack_seqno *sn;
 
-    HMAP_FOR_EACH_WITH_HASH (sn, node, hash_int(val, 0), &seqnos->acked) {
+    HMAP_FOR_EACH_WITH_HASH (sn, node, hash_uint64(val), &seqnos->acked) {
         if (sn->seqno == val) {
             return true;
         }
@@ -213,12 +213,12 @@  ofctrl_acked_seqnos_init(struct ofctrl_acked_seqnos *seqnos,
 }
 
 static void
-ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos, uint32_t val)
+ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos, uint64_t val)
 {
     seqnos->last_acked = val;
 
     struct ofctrl_ack_seqno *sn = xmalloc(sizeof *sn);
-    hmap_insert(&seqnos->acked, &sn->node, hash_int(val, 0));
+    hmap_insert(&seqnos->acked, &sn->node, hash_uint64(val));
     sn->seqno = val;
 }
 
diff --git a/controller/ofctrl-seqno.h b/controller/ofctrl-seqno.h
index 876947c26..adcc5a0d3 100644
--- a/controller/ofctrl-seqno.h
+++ b/controller/ofctrl-seqno.h
@@ -37,7 +37,7 @@  struct ofctrl_ack_seqno {
 struct ofctrl_acked_seqnos *ofctrl_acked_seqnos_get(size_t seqno_type);
 void ofctrl_acked_seqnos_destroy(struct ofctrl_acked_seqnos *seqnos);
 bool ofctrl_acked_seqnos_contains(const struct ofctrl_acked_seqnos *seqnos,
-                                  uint32_t val);
+                                  uint64_t val);
 
 void ofctrl_seqno_init(void);
 size_t ofctrl_seqno_add_type(void);
diff --git a/controller/test-ofctrl-seqno.c b/controller/test-ofctrl-seqno.c
index b96da9d2f..cd3821e04 100644
--- a/controller/test-ofctrl-seqno.c
+++ b/controller/test-ofctrl-seqno.c
@@ -124,9 +124,10 @@  test_ofctrl_seqno_ack_seqnos(struct ovs_cmdl_context *ctx)
         }
 
         for (unsigned int j = 0; j < n_app_seqnos; j++, n_reqs++) {
-            unsigned int app_seqno;
+            unsigned long long int app_seqno;
 
-            if (!test_read_uint_value(ctx, shift++, "app_seqno", &app_seqno)) {
+            if (!test_read_ullong_value(ctx, shift++, "app_seqno",
+                                        &app_seqno)) {
                 return;
             }
             ofctrl_seqno_update_create(i, app_seqno);
@@ -138,9 +139,9 @@  test_ofctrl_seqno_ack_seqnos(struct ovs_cmdl_context *ctx)
         return;
     }
     for (unsigned int i = 0; i < n_acks; i++) {
-        unsigned int ack_seqno;
+        unsigned long long int ack_seqno;
 
-        if (!test_read_uint_value(ctx, shift++, "ack_seqno", &ack_seqno)) {
+        if (!test_read_ullong_value(ctx, shift++, "ack_seqno", &ack_seqno)) {
             return;
         }
         ofctrl_seqno_run(ack_seqno);
diff --git a/tests/ovn-ofctrl-seqno.at b/tests/ovn-ofctrl-seqno.at
index dd7e86f10..8e3428442 100644
--- a/tests/ovn-ofctrl-seqno.at
+++ b/tests/ovn-ofctrl-seqno.at
@@ -223,4 +223,37 @@  ofctrl-seqno-type: 1
   51
   52
 ])
+
+AS_BOX([Ack seqno that doesn't fit in uint32_t])
+n_types=2
+n_app_seqnos=1
+app_seqnos1="4294967296"
+app_seqnos2="4294967297"
+
+n_acks=1
+acks="1"
+AT_CHECK([ovstest test-ofctrl-seqno ofctrl_seqno_ack_seqnos true ${n_types} \
+          ${n_app_seqnos} ${app_seqnos1} ${n_app_seqnos} ${app_seqnos2} \
+          ${n_acks} ${acks}], [0], [dnl
+ofctrl-seqno-req-cfg: 2
+ofctrl-seqno-type: 0
+  last-acked 4294967296
+  4294967296
+ofctrl-seqno-type: 1
+  last-acked 0
+])
+
+n_acks=1
+acks="2"
+AT_CHECK([ovstest test-ofctrl-seqno ofctrl_seqno_ack_seqnos true ${n_types} \
+          ${n_app_seqnos} ${app_seqnos1} ${n_app_seqnos} ${app_seqnos2} \
+          ${n_acks} ${acks}], [0], [dnl
+ofctrl-seqno-req-cfg: 2
+ofctrl-seqno-type: 0
+  last-acked 4294967296
+  4294967296
+ofctrl-seqno-type: 1
+  last-acked 4294967297
+  4294967297
+])
 AT_CLEANUP
diff --git a/tests/test-utils.c b/tests/test-utils.c
index 78e63acb7..1afdc150f 100644
--- a/tests/test-utils.c
+++ b/tests/test-utils.c
@@ -61,3 +61,20 @@  test_read_value(struct ovs_cmdl_context *ctx, unsigned int index,
 
     return ctx->argv[index];
 }
+
+bool
+test_read_ullong_value(struct ovs_cmdl_context *ctx, unsigned int index,
+                       const char *descr, unsigned long long int *result)
+{
+    if (index >= ctx->argc) {
+        fprintf(stderr, "Missing %s argument\n", descr);
+        return false;
+    }
+
+    const char *arg = ctx->argv[index];
+    if (!str_to_ullong(arg, 10, result)) {
+        fprintf(stderr, "Invalid %s: %s\n", descr, arg);
+        return false;
+    }
+    return true;
+}
diff --git a/tests/test-utils.h b/tests/test-utils.h
index 910358ea1..22341cea9 100644
--- a/tests/test-utils.h
+++ b/tests/test-utils.h
@@ -24,5 +24,7 @@  bool test_read_uint_hex_value(struct ovs_cmdl_context *ctx, unsigned int index,
                               const char *descr, unsigned int *result);
 const char *test_read_value(struct ovs_cmdl_context *ctx, unsigned int index,
                             const char *descr);
+bool test_read_ullong_value(struct ovs_cmdl_context *ctx, unsigned int index,
+                            const char *descr, unsigned long long int *result);
 
 #endif /* tests/test-utils.h */