mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-09-01 06:45:17 +00:00
Code Issues Releases Wiki Activity

ofp-actions: Ensure aligned accesses to masked fields.

Browse Source 
For example is parsing the OVN "eth.dst[40] = 1;" action, which
triggered the following warning from UndefinedBehaviorSanitizer:

  lib/meta-flow.c:3210:9:
  runtime error: member access within misaligned address 0x000000de4e36
  for type 'const union mf_value', which requires 8 byte alignment
  0x000000de4e36: note: pointer points here
   00 00 00 00 01 00  00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00
               ^
   10 51 de 00 00 00 00 00  c0 4f

      0 0x5818bc in mf_format lib/meta-flow.c:3210
      1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342
      2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213
      3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237
      4 0x410922 in test_parse_actions tests/test-ovn.c:1360

To avoid this we now change the internal representation of the set_field
actions, struct ofpact_set_field, such that the mask is always stored
at a correctly aligned address, multiple of OFPACT_ALIGNTO.

We also need to adapt the "ovs-ofctl show-flows - Oversized flow" test
because now the ofpact representation of the set_field action uses more
bytes in memory (for the extra alignment).  Change the test to use
dec_ttl instead.

Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
This commit is contained in:
Dumitru Ceara
2022-04-11 13:38:13 +02:00
committed by Ilya Maximets
parent 471babb811
commit 933aaf9444
3 changed files with 20 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
include/openvswitch/ofp-actions.h
View File

@@ -549,7 +549,8 @@ struct ofpact_set_field {
const struct mf_field *field; const struct mf_field *field;
); );
union mf_value value[]; /* Significant value bytes followed by union mf_value value[]; /* Significant value bytes followed by
* significant mask bytes. */ * significant mask bytes aligned at
* OFPACT_ALIGNTO bytes. */
}; };
BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value) BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value)
% OFPACT_ALIGNTO == 0); % OFPACT_ALIGNTO == 0);
@@ -557,9 +558,10 @@ BUILD_ASSERT_DECL(offsetof(struct ofpact_set_field, value)
== sizeof(struct ofpact_set_field)); == sizeof(struct ofpact_set_field));
/* Use macro to not have to deal with constness. */ /* Use macro to not have to deal with constness. */
#define ofpact_set_field_mask(SF) \ #define ofpact_set_field_mask(SF) \
ALIGNED_CAST(union mf_value *, \ ALIGNED_CAST(union mf_value *, \
(uint8_t *)(SF)->value + (SF)->field->n_bytes) (uint8_t *)(SF)->value + \
ROUND_UP((SF)->field->n_bytes, OFPACT_ALIGNTO))
/* OFPACT_PUSH_VLAN/MPLS/PBB /* OFPACT_PUSH_VLAN/MPLS/PBB
* *

19
lib/ofp-actions.c
View File

@@ -3381,21 +3381,28 @@ check_SET_FIELD(struct ofpact_set_field *a,
return 0; return 0;
} }
/* Appends an OFPACT_SET_FIELD ofpact with enough space for the value and mask /* Appends an OFPACT_SET_FIELD ofpact with enough space for the value and a
* for the 'field' to 'ofpacts' and returns it. Fills in the value from * properly aligned mask for the 'field' to 'ofpacts' and returns it. Fills
* 'value', if non-NULL, and mask from 'mask' if non-NULL. If 'value' is * in the value from 'value', if non-NULL, and mask from 'mask' if non-NULL.
* non-NULL and 'mask' is NULL, an all-ones mask will be filled in. */ * If 'value' is non-NULL and 'mask' is NULL, an all-ones mask will be
* filled in. */
struct ofpact_set_field * struct ofpact_set_field *
ofpact_put_set_field(struct ofpbuf *ofpacts, const struct mf_field *field, ofpact_put_set_field(struct ofpbuf *ofpacts, const struct mf_field *field,
const void *value, const void *mask) const void *value, const void *mask)
{ {
/* Ensure there's enough space for:
* - value (8-byte aligned)
* - mask (8-byte aligned)
* - padding (to make the whole ofpact_set_field 8-byte aligned)
*/
size_t total_size = 2 * ROUND_UP(field->n_bytes, OFPACT_ALIGNTO);
struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
sf->field = field; sf->field = field;
/* Fill in the value and mask if given, otherwise put zeroes so that the /* Fill in the value and mask if given, otherwise put zeroes so that the
* caller may fill in the value and mask itself. */ * caller may fill in the value and mask itself. */
if (value) { if (value) {
ofpbuf_put_uninit(ofpacts, 2 * field->n_bytes); ofpbuf_put_uninit(ofpacts, total_size);
sf = ofpacts->header; sf = ofpacts->header;
memcpy(sf->value, value, field->n_bytes); memcpy(sf->value, value, field->n_bytes);
if (mask) { if (mask) {
@@ -3404,7 +3411,7 @@ ofpact_put_set_field(struct ofpbuf *ofpacts, const struct mf_field *field,
memset(ofpact_set_field_mask(sf), 0xff, field->n_bytes); memset(ofpact_set_field_mask(sf), 0xff, field->n_bytes);
} }
} else { } else {
ofpbuf_put_zeros(ofpacts, 2 * field->n_bytes); ofpbuf_put_zeros(ofpacts, total_size);
sf = ofpacts->header; sf = ofpacts->header;
} }
/* Update length. */ /* Update length. */

2
tests/ovs-ofctl.at
View File

@@ -3258,7 +3258,7 @@ AT_SETUP([ovs-ofctl show-flows - Oversized flow])
OVS_VSWITCHD_START OVS_VSWITCHD_START
printf " priority=90,icmp,reg15=0x8005,metadata=0x1,nw_dst=11.0.0.1,icmp_type=8,icmp_code=0 actions=" > flow.txt printf " priority=90,icmp,reg15=0x8005,metadata=0x1,nw_dst=11.0.0.1,icmp_type=8,icmp_code=0 actions=" > flow.txt
for i in `seq 1 1022`; do printf "set_field:0x399->reg13,set_field:0x$i->reg15,resubmit(,39),"; done >> flow.txt for i in `seq 1 2045`; do printf "dec_ttl,resubmit(,39),"; done >> flow.txt
printf "resubmit(,39)\n" >> flow.txt printf "resubmit(,39)\n" >> flow.txt
AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flow.txt]) AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flow.txt])