diff mbox series

[ovs-dev,RFC,v3,1/4] lib/tc: make pedit mask calculations byte order agnostic

Message ID 1548678550-6661-2-git-send-email-pieter.jansenvanvuuren@netronome.com
State Accepted
Delegated to: Simon Horman
Headers show
Series extend ovs-tc offload for more pedit action | expand

Commit Message

Pieter Jansen van Vuuren Jan. 28, 2019, 12:29 p.m. UTC 
pedit allows setting entire words with an optional mask and OVS
makes use of such masks to allow setting fields that do not span
entire words.

The struct tc_pedit_key structure, which is part of the kernel
ABI, uses host byte order fields to store the mask and value for
a pedit action, however, these fields contain values in network
byte order.

In order to allow static analysis tools to check for endianness
problems this patch adds a local version of struct tc_pedit_key
which uses big endian types and refactors the relevant code as
appropriate.

In the course of making this change it became apparent that the
calculation of masks was occurring using host byte order although
the values are in network byte order. This patch also fixes that
problem by shifting values in host byte order and then converting
them to network byte order. It is believe this fixes a bug on big
endian systems although we are not in a position to test that.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/sparse/automake.mk             |  3 ++-
 include/sparse/linux/tc_act/tc_pedit.h | 29 ++++++++++++++++++++++++++
 lib/tc.c                               | 22 +++++++++----------
 3 files changed, 42 insertions(+), 12 deletions(-)
 create mode 100644 include/sparse/linux/tc_act/tc_pedit.h
diff mbox series

Patch

diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
index 985ee6a2f..4c7b17783 100644
--- a/include/sparse/automake.mk
+++ b/include/sparse/automake.mk
@@ -26,4 +26,5 @@  noinst_HEADERS += \
         include/sparse/sys/sysmacros.h \
         include/sparse/sys/types.h \
         include/sparse/sys/wait.h \
-        include/sparse/threads.h
+        include/sparse/threads.h \
+        include/sparse/linux/tc_act/tc_pedit.h
diff --git a/include/sparse/linux/tc_act/tc_pedit.h b/include/sparse/linux/tc_act/tc_pedit.h
new file mode 100644
index 000000000..ca5c1f1fe
--- /dev/null
+++ b/include/sparse/linux/tc_act/tc_pedit.h
@@ -0,0 +1,29 @@ 
+#ifndef FIX_LINUX_TC_PEDIT_H
+#define FIX_LINUX_TC_PEDIT_H
+
+#ifndef __CHECKER__
+#error "Use this header only with sparse.  It is not a correct implementation."
+#endif
+
+#include_next <linux/tc_act/tc_pedit.h>
+
+/* Fixes endianness of 'mask' and 'val' members. */
+#define tc_pedit_key rpl_tc_pedit_key
+struct rpl_tc_pedit_key {
+    ovs_be32        mask;  /* AND */
+    ovs_be32        val;   /* XOR */
+    __u32           off;   /* offset */
+    __u32           at;
+    __u32           offmask;
+    __u32           shift;
+};
+
+#define tc_pedit_sel rpl_tc_pedit_sel
+struct rpl_tc_pedit_sel {
+    tc_gen;
+    unsigned char           nkeys;
+    unsigned char           flags;
+    struct tc_pedit_key     keys[0];
+};
+
+#endif
diff --git a/lib/tc.c b/lib/tc.c
index b19f075f2..ae5017c17 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -825,17 +825,17 @@  nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
             if ((keys->off >= mf && keys->off < mf + sz)
                 || (keys->off + 3 >= mf && keys->off + 3 < mf + sz)) {
                 int diff = flower_off + (keys->off - mf);
-                uint32_t *dst = (void *) (rewrite_key + diff);
-                uint32_t *dst_m = (void *) (rewrite_mask + diff);
-                uint32_t mask = ~(keys->mask);
+                ovs_be32 *dst = (void *) (rewrite_key + diff);
+                ovs_be32 *dst_m = (void *) (rewrite_mask + diff);
+                ovs_be32 mask = ~(keys->mask);
                 uint32_t zero_bits;
 
                 if (keys->off < mf) {
                     zero_bits = 8 * (mf - keys->off);
-                    mask &= UINT32_MAX << zero_bits;
+                    mask &= htonl(UINT32_MAX >> zero_bits);
                 } else if (keys->off + 4 > mf + m->size) {
                     zero_bits = 8 * (keys->off + 4 - mf - m->size);
-                    mask &= UINT32_MAX >> zero_bits;
+                    mask &= htonl(UINT32_MAX << zero_bits);
                 }
 
                 *dst_m |= mask;
@@ -1725,8 +1725,8 @@  nl_msg_put_act_cookie(struct ofpbuf *request, struct tc_cookie *ck) {
  * (as we read entire words). */
 static void
 calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m,
-             int *cur_offset, int *cnt, uint32_t *last_word_mask,
-             uint32_t *first_word_mask, uint32_t **mask, uint32_t **data)
+             int *cur_offset, int *cnt, ovs_be32 *last_word_mask,
+             ovs_be32 *first_word_mask, ovs_be32 **mask, ovs_be32 **data)
 {
     int start_offset, max_offset, total_size;
     int diff, right_zero_bits, left_zero_bits;
@@ -1742,8 +1742,8 @@  calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m,
 
     *cur_offset = start_offset;
     *cnt = (total_size / 4) + (total_size % 4 ? 1 : 0);
-    *last_word_mask = UINT32_MAX >> right_zero_bits;
-    *first_word_mask = UINT32_MAX << left_zero_bits;
+    *last_word_mask = htonl(UINT32_MAX << right_zero_bits);
+    *first_word_mask = htonl(UINT32_MAX >> left_zero_bits);
     *data = (void *) (rewrite_key + m->flower_offset - diff);
     *mask = (void *) (rewrite_mask + m->flower_offset - diff);
 }
@@ -1815,7 +1815,7 @@  nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
         struct flower_key_to_pedit *m = &flower_pedit_map[i];
         struct tc_pedit_key *pedit_key = NULL;
         struct tc_pedit_key_ex *pedit_key_ex = NULL;
-        uint32_t *mask, *data, first_word_mask, last_word_mask;
+        ovs_be32 *mask, *data, first_word_mask, last_word_mask;
         int cnt = 0, cur_offset = 0;
 
         if (!m->size) {
@@ -1826,7 +1826,7 @@  nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
                      &first_word_mask, &mask, &data);
 
         for (j = 0; j < cnt; j++,  mask++, data++, cur_offset += 4) {
-            uint32_t mask_word = *mask;
+            ovs_be32 mask_word = *mask;
 
             if (j == 0) {
                 mask_word &= first_word_mask;