There are multiple logical and coding issues in this functionality.
They are semi-related, so it's easier to fix them all at once instead
of trying to untangle the changes into separate commits.
The issues are:
- The implementation of a non-masked variant of the action is putting
the base flow values into the action instead of the updated key, so
the packet never gets actually updated.
- But this is OK, because the non-masked variant is mostly a dead
code, since 'fully_masked' is always false, as mdtype and np are always
zeroed out before calling commit_nsh(). So, the only way to hit this
code is to manually turn off support for masked set.
- The put_nsh_key() function can be called for a mask, but it checks
the mdtype value as if it was a key regardless. And since mdtype is
always zeroed out in the mask, the match on the context is never
propagated to the datapath flow, even if added during the action
commit.
- The key and the base are compared for being the same twice, once
in the commit_set_nsh_action() and again in commit_nsh().
- commit_set_nsh_action() is using an assumption that mdtype is
changing in the mask whenever the mask is changing. This is fine in
the current code, but isn't how it supposed to work in general.
- The masked variant of the action is generating a set with an empty
mask in case the context didn't change. Same for the base header.
This works in userspace, but kernel generally fails the validation on
set() with an empty mask. The attribute should just not be provided
in this case.
- For some reason the printing function initializes the mask to an
exact match, printing out missing attributes in full as all-zero,
while they should not be printed if not present.
- The masked variant duplicates the part of commit_masked_set_action()
twice, while this code can be re-used without much trouble.
Fixing all the issues above and making the code look more like it
looks for other fields and header types. Completely removing the
get/put_nsh_key() functions as we should not be clearing the context
anyway. If we somehow end up with the MD1 context on an MD2 packet,
it means that we have a bug somewhere else that should be fixed.
The tests updated to cover the problematic functionality.
There are still more issues here. The main one is that we need to
actually probe the datapath before using this action, as not all the
kernel versions support NSH and support for set(nsh) was recently
removed entirely from the Linux kernel due to being completely wrong.
This is the reason for not adding any system tests here, as they
would fail or crash the kernel.
Will work on probing and the re-introduction of the support in the
kernel separately.
Fixes: 3d2fbd70bda5 ("userspace: Add support for NSH MD1 match fields")
Fixes: f59cb331c481 ("nsh: rework NSH netlink keys and actions")
Fixes: 81fdabb94dd7 ("nsh: fix nested mask for OVS_KEY_ATTR_NSH")
Signed-off-by: Ilya Maximets <[email protected]>
---
lib/odp-util.c | 185 ++++++++++++++++++-------------------------------
tests/nsh.at | 61 +++++++++++++---
2 files changed, 117 insertions(+), 129 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index ee1868202..fbbfa426f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1050,7 +1050,7 @@ format_odp_set_nsh(struct ds *ds, const struct nlattr
*attr)
struct ovs_key_nsh nsh_mask;
memset(&nsh, 0, sizeof nsh);
- memset(&nsh_mask, 0xff, sizeof nsh_mask);
+ memset(&nsh_mask, 0, sizeof nsh_mask);
NL_NESTED_FOR_EACH (a, left, attr) {
enum ovs_nsh_key_attr type = nl_attr_type(a);
@@ -6333,10 +6333,6 @@ static void get_arp_key(const struct flow *, struct
ovs_key_arp *);
static void put_arp_key(const struct ovs_key_arp *, struct flow *);
static void get_nd_key(const struct flow *, struct ovs_key_nd *);
static void put_nd_key(const struct ovs_key_nd *, struct flow *);
-static void get_nsh_key(const struct flow *flow, struct ovs_key_nsh *nsh,
- bool is_mask);
-static void put_nsh_key(const struct ovs_key_nsh *nsh, struct flow *flow,
- bool is_mask);
/* These share the same layout. */
union ovs_key_tp {
@@ -7936,16 +7932,13 @@ commit_set_action(struct ofpbuf *odp_actions, enum
ovs_key_attr key_type,
nl_msg_end_nested(odp_actions, offset);
}
-/* Masked set actions have a mask following the data within the netlink
- * attribute. The unmasked bits in the data will be cleared as the data
- * is copied to the action. */
-void
-commit_masked_set_action(struct ofpbuf *odp_actions,
- enum ovs_key_attr key_type,
- const void *key_, const void *mask_, size_t key_size)
+/* Commit one attribute for a masked set action. Masked set actions have
+ * a mask following the data within the netlink attribute. The unmasked bits
+ * in the data will be cleared as the data is copied to the attribute. */
+static void
+commit_masked_attribute(struct ofpbuf *odp_actions, int key_type,
+ const void *key_, const void *mask_, size_t key_size)
{
- size_t offset = nl_msg_start_nested(odp_actions,
- OVS_ACTION_ATTR_SET_MASKED);
char *data = nl_msg_put_unspec_uninit(odp_actions, key_type, key_size * 2);
const char *key = key_, *mask = mask_;
@@ -7954,6 +7947,17 @@ commit_masked_set_action(struct ofpbuf *odp_actions,
while (key_size--) {
*data++ = *key++ & *mask++;
}
+}
+
+/* A helper to commit a masked set action for a single attribute. */
+void
+commit_masked_set_action(struct ofpbuf *odp_actions,
+ enum ovs_key_attr key_type,
+ const void *key_, const void *mask_, size_t key_size)
+{
+ size_t offset = nl_msg_start_nested(odp_actions,
+ OVS_ACTION_ATTR_SET_MASKED);
+ commit_masked_attribute(odp_actions, key_type, key_, mask_, key_size);
nl_msg_end_nested(odp_actions, offset);
}
@@ -8541,116 +8545,65 @@ commit_set_nw_action(const struct flow *flow, struct
flow *base,
return 0;
}
-static inline void
-get_nsh_key(const struct flow *flow, struct ovs_key_nsh *nsh, bool is_mask)
-{
- *nsh = flow->nsh;
- if (!is_mask) {
- if (nsh->mdtype != NSH_M_TYPE1) {
- memset(nsh->context, 0, sizeof(nsh->context));
- }
- }
-}
-
-static inline void
-put_nsh_key(const struct ovs_key_nsh *nsh, struct flow *flow,
- bool is_mask OVS_UNUSED)
-{
- flow->nsh = *nsh;
- if (flow->nsh.mdtype != NSH_M_TYPE1) {
- memset(flow->nsh.context, 0, sizeof(flow->nsh.context));
- }
-}
-
static bool
-commit_nsh(const struct ovs_key_nsh * flow_nsh, bool use_masked_set,
- const struct ovs_key_nsh *key, struct ovs_key_nsh *base,
- struct ovs_key_nsh *mask, size_t size,
+commit_nsh(bool use_masked_set, const struct ovs_key_nsh *key,
+ struct ovs_key_nsh *base, struct ovs_key_nsh *mask, size_t size,
struct ofpbuf *odp_actions)
{
- enum ovs_key_attr attr = OVS_KEY_ATTR_NSH;
-
- if (memcmp(key, base, size) == 0) {
+ if (!memcmp(key, base, size)) {
/* Mask bits are set when we have either read or set the corresponding
* values. Masked bits will be exact-matched, no need to set them
* if the value did not actually change. */
return false;
}
- bool fully_masked = odp_mask_is_exact(attr, mask, size);
+ bool fully_masked = odp_mask_is_exact(OVS_KEY_ATTR_NSH, mask, size);
+ size_t set_ofs;
if (use_masked_set && !fully_masked) {
- size_t nsh_key_ofs;
- struct ovs_nsh_key_base nsh_base;
- struct ovs_nsh_key_base nsh_base_mask;
- struct ovs_nsh_key_md1 md1;
- struct ovs_nsh_key_md1 md1_mask;
- size_t offset = nl_msg_start_nested(odp_actions,
- OVS_ACTION_ATTR_SET_MASKED);
-
- nsh_base.flags = key->flags;
- nsh_base.ttl = key->ttl;
- nsh_base.mdtype = key->mdtype;
- nsh_base.np = key->np;
- nsh_base.path_hdr = key->path_hdr;
-
- nsh_base_mask.flags = mask->flags;
- nsh_base_mask.ttl = mask->ttl;
- nsh_base_mask.mdtype = mask->mdtype;
- nsh_base_mask.np = mask->np;
- nsh_base_mask.path_hdr = mask->path_hdr;
-
- /* OVS_KEY_ATTR_NSH keys */
- nsh_key_ofs = nl_msg_start_nested(odp_actions, OVS_KEY_ATTR_NSH);
-
- /* put value and mask for OVS_NSH_KEY_ATTR_BASE */
- char *data = nl_msg_put_unspec_uninit(odp_actions,
- OVS_NSH_KEY_ATTR_BASE,
- 2 * sizeof(nsh_base));
- const char *lkey = (char *)&nsh_base, *lmask = (char *)&nsh_base_mask;
- size_t lkey_size = sizeof(nsh_base);
-
- while (lkey_size--) {
- *data++ = *lkey++ & *lmask++;
- }
- lmask = (char *)&nsh_base_mask;
- memcpy(data, lmask, sizeof(nsh_base_mask));
-
- switch (key->mdtype) {
- case NSH_M_TYPE1:
- memcpy(md1.context, key->context, sizeof key->context);
- memcpy(md1_mask.context, mask->context, sizeof mask->context);
-
- /* put value and mask for OVS_NSH_KEY_ATTR_MD1 */
- data = nl_msg_put_unspec_uninit(odp_actions,
- OVS_NSH_KEY_ATTR_MD1,
- 2 * sizeof(md1));
- lkey = (char *)&md1;
- lmask = (char *)&md1_mask;
- lkey_size = sizeof(md1);
-
- while (lkey_size--) {
- *data++ = *lkey++ & *lmask++;
- }
- lmask = (char *)&md1_mask;
- memcpy(data, lmask, sizeof(md1_mask));
- break;
- case NSH_M_TYPE2:
- default:
- /* No match support for other MD formats yet. */
- break;
+ struct ovs_nsh_key_base nsh_base = {
+ .flags = key->flags,
+ .ttl = key->ttl,
+ /* 'mdtype' and 'np' are not writable. */
+ .path_hdr = key->path_hdr,
+ };
+ struct ovs_nsh_key_base nsh_base_mask = {
+ .flags = mask->flags,
+ .ttl = mask->ttl,
+ /* 'mdtype' and 'np' are not writable. */
+ .path_hdr = mask->path_hdr,
+ };
+ size_t nsh_ofs;
+
+ set_ofs = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SET_MASKED);
+ nsh_ofs = nl_msg_start_nested(odp_actions, OVS_KEY_ATTR_NSH);
+
+ if (!is_all_zeros(&nsh_base_mask, sizeof nsh_base_mask)) {
+ commit_masked_attribute(odp_actions, OVS_NSH_KEY_ATTR_BASE,
+ &nsh_base, &nsh_base_mask,
+ sizeof nsh_base);
}
- nl_msg_end_nested(odp_actions, nsh_key_ofs);
+ /* No match support for other MD formats yet. */
+ if (key->mdtype == NSH_M_TYPE1
+ && !is_all_zeros(mask->context, sizeof *mask->context)) {
+ commit_masked_attribute(odp_actions, OVS_NSH_KEY_ATTR_MD1,
+ key->context, mask->context,
+ sizeof key->context);
+ }
- nl_msg_end_nested(odp_actions, offset);
+ nl_msg_end_nested(odp_actions, nsh_ofs);
+ nl_msg_end_nested(odp_actions, set_ofs);
} else {
+ /* Overwriting the whole header. Make sure that fields that wasn't
+ * in the mask are indeed the same as in the committed action. */
if (!fully_masked) {
memset(mask, 0xff, size);
}
- size_t offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SET);
- nsh_key_to_attr(odp_actions, flow_nsh, NULL, 0, false);
- nl_msg_end_nested(odp_actions, offset);
+
+ set_ofs = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SET);
+ nsh_key_to_attr(odp_actions, key, NULL, 0, false);
+ nl_msg_end_nested(odp_actions, set_ofs);
}
memcpy(base, key, size);
return true;
@@ -8664,8 +8617,7 @@ commit_set_nsh_action(const struct flow *flow, struct
flow *base_flow,
{
struct ovs_key_nsh key, mask, base;
- if (flow->dl_type != htons(ETH_TYPE_NSH) ||
- !memcmp(&base_flow->nsh, &flow->nsh, sizeof base_flow->nsh)) {
+ if (flow->dl_type != htons(ETH_TYPE_NSH)) {
return;
}
@@ -8673,18 +8625,13 @@ commit_set_nsh_action(const struct flow *flow, struct
flow *base_flow,
ovs_assert(flow->nsh.mdtype == base_flow->nsh.mdtype &&
flow->nsh.np == base_flow->nsh.np);
- get_nsh_key(flow, &key, false);
- get_nsh_key(base_flow, &base, false);
- get_nsh_key(&wc->masks, &mask, true);
- mask.mdtype = 0; /* Not writable. */
- mask.np = 0; /* Not writable. */
+ key = flow->nsh;
+ base = base_flow->nsh;
+ mask = wc->masks.nsh;
- if (commit_nsh(&base_flow->nsh, use_masked, &key, &base, &mask,
- sizeof key, odp_actions)) {
- put_nsh_key(&base, base_flow, false);
- if (mask.mdtype != 0) { /* Mask was changed by commit(). */
- put_nsh_key(&mask, &wc->masks, true);
- }
+ if (commit_nsh(use_masked, &key, &base, &mask, sizeof key, odp_actions)) {
+ base_flow->nsh = base;
+ or_bytes(&wc->masks.nsh, &mask, sizeof wc->masks.nsh);
}
}
diff --git a/tests/nsh.at b/tests/nsh.at
index 022540dd6..d9f80fc02 100644
--- a/tests/nsh.at
+++ b/tests/nsh.at
@@ -4,7 +4,7 @@ AT_BANNER([network service header (NSH)])
### Simple NSH matching test case
### -----------------------------------------------------------------
-AT_SETUP([nsh - matching])
+AT_SETUP([nsh - match and set])
OVS_VSWITCHD_START([dnl
set bridge br0 datapath_type=dummy \
@@ -13,7 +13,8 @@ OVS_VSWITCHD_START([dnl
add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
AT_DATA([flows.txt], [dnl
-
table=0,in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344,actions=set_field:0x2->nsh_flags,set_field:254->nsh_si,set_field:0x44332211->nsh_c1,2
+table=0,in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344,dnl
+actions=set_field:0x2->nsh_flags,set_field:254->nsh_si,set_field:0x44332211->nsh_c1,load:0x77->nsh_c3[[8..15]],2
])
AT_CHECK([
@@ -21,13 +22,48 @@ AT_CHECK([
ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep actions
], [0], [dnl
-
in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344
actions=set_field:2->nsh_flags,set_field:254->nsh_si,set_field:0x44332211->nsh_c1,output:2
+[
in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344]dnl
+[
actions=set_field:2->nsh_flags,set_field:254->nsh_si,set_field:0x44332211->nsh_c1,set_field:0x7700/0xff00->nsh_c3,output:2]
])
-AT_CHECK([
- ovs-appctl ofproto/trace br0
'in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344,nsh_c2=0x55667788,nsh_c3=0x99aabbcc,nsh_c4=0xddeeff00'
-], [0], [dnl
-Flow:
in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344,nsh_c2=0x55667788,nsh_c3=0x99aabbcc,nsh_c4=0xddeeff00,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
+m4_define([NSH_HEADER], [m4_join([,],
+ [nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255],
+ [nsh_c1=0x11223344,nsh_c2=0x55667788,nsh_c3=0x99aabbcc,nsh_c4=0xddeeff00])])
+
+m4_define([NSH_HEADER2], [m4_join([,],
+ [nsh_flags=2,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=254],
+ [nsh_c1=0x44332211,nsh_c2=0x55667788,nsh_c3=0x99aa77cc,nsh_c4=0xddeeff00])])
+
+AT_CHECK([ovs-appctl ofproto/trace br0
'in_port=1,eth_type=0x894f,NSH_HEADER'], [0], [dnl
+Flow: in_port=1,vlan_tci=0x0000,dnl
+dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,NSH_HEADER,dnl
+nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
+
+bridge("br0")
+-------------
+ 0.
in_port=1,dl_type=0x894f,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344,
priority 32768
+ set_field:2->nsh_flags
+ set_field:254->nsh_si
+ set_field:0x44332211->nsh_c1
+ set_field:0x7700/0xff00->nsh_c3
+ output:2
+
+Final flow: in_port=1,vlan_tci=0x0000,dnl
+dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,NSH_HEADER2,dnl
+nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
+Megaflow: recirc_id=0,eth,in_port=1,dl_type=0x894f,dnl
+nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,dnl
+nsh_c1=0x11223344,nsh_c3=0xbb00/0xff00
+Datapath actions:
set(nsh(flags=2,ttl=63,spi=0x123456,si=254,c1=0x44332211,c3=0x7700/0x0000ff00)),2
+])
+
+dnl Check that non-masked action variant is also correct.
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 masked_set_action false])
+
+AT_CHECK([ovs-appctl ofproto/trace br0
'in_port=1,eth_type=0x894f,NSH_HEADER'], [0], [dnl
+Flow: in_port=1,vlan_tci=0x0000,dnl
+dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,NSH_HEADER,dnl
+nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
bridge("br0")
-------------
@@ -35,11 +71,16 @@ bridge("br0")
set_field:2->nsh_flags
set_field:254->nsh_si
set_field:0x44332211->nsh_c1
+ set_field:0x7700/0xff00->nsh_c3
output:2
-Final flow:
in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,nsh_flags=2,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=254,nsh_c1=0x44332211,nsh_c2=0x55667788,nsh_c3=0x99aabbcc,nsh_c4=0xddeeff00,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
-Megaflow:
recirc_id=0,eth,in_port=1,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,nsh_c1=0x11223344
-Datapath actions: set(nsh(flags=2,ttl=63,spi=0x123456,si=254,c1=0x44332211)),2
+Final flow: in_port=1,vlan_tci=0x0000,dnl
+dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,dl_type=0x894f,NSH_HEADER2,dnl
+nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0,nw_frag=no
+Megaflow: recirc_id=0,eth,in_port=1,dl_type=0x894f,dnl
+nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x123456,nsh_si=255,dnl
+nsh_c1=0x11223344,nsh_c2=0x55667788,nsh_c3=0x99aabbcc,nsh_c4=0xddeeff00
+Datapath actions:
set(nsh(flags=2,ttl=63,mdtype=1,np=3,spi=0x123456,si=254,c1=0x44332211,c2=0x55667788,c3=0x99aa77cc,c4=0xddeeff00)),2
])
OVS_VSWITCHD_STOP
--
2.52.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
