On 7 March 2017 at 09:39, Yi-Hung Wei <[email protected]> wrote:
> Currently, a controller may potentially trigger a segmentation fault if it
> accidentally removes a TLV mapping that is still used by an active flow.
> To resolve this issue, in this patch, we maintain reference counting for each
> dynamically allocated variable length mf_fields, so that vswitchd can use this
> information to properly remove a TLV mapping, and to return an error if the
> controller tries to remove a TLV mapping that is still used by any active
> flow.
>
> To keep track of the usage of tun_metadata for each flow, two 'uint64_t'
> bitmaps are introduce for the flow match and flow action respectively. We use
> 'uint64_t' as a bitmap since the 64 geneve TLV tunnel metadata are the only
> available variable length mf_fields for now. We shall adopt general bitmap
> when
> more variable length mf_fields are introduced. The bitmaps are configured
> during the flow decoding process, and vswitchd use these bitmaps to increase
> or
> decrease the ref counting when the flow is created or deleted.
>
> VMWare-BZ: #1768370
> Fixes: 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs.")
> Suggested-by: Jarno Rajahalme <[email protected]>
> Suggested-by: Joe Stringer <[email protected]>
> Signed-off-by: Yi-Hung Wei <[email protected]>
> ---
Thanks for the patch, feedback below.
> build-aux/extract-ofp-actions | 9 +-
> include/openvswitch/ofp-actions.h | 3 +-
> include/openvswitch/ofp-errors.h | 4 +
> include/openvswitch/ofp-util.h | 1 +
> lib/learn.c | 6 ++
> lib/meta-flow.c | 196
> +++++++++++++++++++++++++++++++-------
> lib/ofp-actions.c | 156 ++++++++++++++++++------------
> lib/ofp-util.c | 21 ++--
> lib/vl-mff-map.h | 10 +-
> ofproto/ofproto-provider.h | 4 +
> ofproto/ofproto.c | 39 ++++++--
> ovn/controller/pinctrl.c | 6 +-
> tests/tunnel.at | 76 ++++++++++++++-
> utilities/ovs-ofctl.c | 2 +-
> 14 files changed, 404 insertions(+), 129 deletions(-)
>
> diff --git a/build-aux/extract-ofp-actions b/build-aux/extract-ofp-actions
> index 184447b99422..0062ab881dd5 100755
> --- a/build-aux/extract-ofp-actions
> +++ b/build-aux/extract-ofp-actions
> @@ -322,7 +322,8 @@ def extract_ofp_actions(fn, definitions):
> static enum ofperr
> ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type
> raw,
> enum ofp_version version, uint64_t arg,
> - const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> + const struct vl_mff_map *vl_mff_map,
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> switch (raw) {\
> """
> @@ -343,7 +344,7 @@ ofpact_decode(const struct ofp_action_header *a, enum
> ofp_raw_action_type raw,
> else:
> arg = "arg"
> if arg_vl_mff_map:
> - print " return decode_%s(%s, version, vl_mff_map,
> out);" % (enum, arg)
> + print " return decode_%s(%s, version, vl_mff_map,
> tlv_bitmap, out);" % (enum, arg)
> else:
> print " return decode_%s(%s, version, out);" %
> (enum, arg)
> print
> @@ -365,7 +366,7 @@ ofpact_decode(const struct ofp_action_header *a, enum
> ofp_raw_action_type raw,
> else:
> prototype += "%s, enum ofp_version, " % base_argtype
> if arg_vl_mff_map:
> - prototype += 'const struct vl_mff_map *, '
> + prototype += 'const struct vl_mff_map *, uint64_t *, '
> prototype += "struct ofpbuf *);"
> print prototype
>
> @@ -374,7 +375,7 @@ static enum ofperr ofpact_decode(const struct
> ofp_action_header *,
> enum ofp_raw_action_type raw,
> enum ofp_version version,
> uint64_t arg, const struct vl_mff_map
> *vl_mff_map,
> - struct ofpbuf *out);
> + uint64_t *tlv_bitmap, struct ofpbuf *out);
> """
>
> if __name__ == '__main__':
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 88f573dcd74e..808715e9b1a4 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -946,13 +946,14 @@ enum ofperr ofpacts_pull_openflow_actions(struct ofpbuf
> *openflow,
> unsigned int actions_len,
> enum ofp_version version,
> const struct vl_mff_map *,
> + uint64_t *,
Please name this variable, this is the public library. Particularly
since the field is a generic type (uint64_t), there's no way to get
the context of what it's used for in the header here. Also, please
update the comment for this function, specifying whether it is a
required field and what the effect of specifying this is.
> struct ofpbuf *ofpacts);
> enum ofperr
> ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
> unsigned int instructions_len,
> enum ofp_version version,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts);
> + uint64_t *, struct ofpbuf *ofpacts);
Same here.
> enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
> struct flow *, ofp_port_t max_ports,
> uint8_t table_id, uint8_t n_tables,
> diff --git a/include/openvswitch/ofp-errors.h
> b/include/openvswitch/ofp-errors.h
> index 81825817e843..a5bba8619bcb 100644
> --- a/include/openvswitch/ofp-errors.h
> +++ b/include/openvswitch/ofp-errors.h
> @@ -772,6 +772,10 @@ enum ofperr {
> * to be mapped is the same as one assigned to a different field. */
> OFPERR_NXTTMFC_DUP_ENTRY,
>
> + /* NX1.0-1.1(1,537), NX1.2+(38). Attempted to delete a TLV mapping that
> + * is used by any active flow. */
> + OFPERR_NXTTMFC_INVALID_TLV_DEL,
> +
> /* ## ---------- ## */
> /* ## NXT_RESUME ## */
> /* ## ---------- ## */
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
> index e73a942a3e15..7cb9e7fd32bd 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -326,6 +326,7 @@ struct ofputil_flow_mod {
> uint16_t importance; /* Eviction precedence. */
> struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
> size_t ofpacts_len; /* Length of ofpacts, in bytes. */
> + uint64_t ofpacts_tlv_bitmap; /* 1-bit for each present TLV in 'ofpacts'.
> */
> };
>
> enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
> diff --git a/lib/learn.c b/lib/learn.c
> index ce52c35f297a..0e86f1629d74 100644
> --- a/lib/learn.c
> +++ b/lib/learn.c
> @@ -29,6 +29,7 @@
> #include "openvswitch/ofp-errors.h"
> #include "openvswitch/ofp-util.h"
> #include "openvswitch/ofpbuf.h"
> +#include "vl-mff-map.h"
> #include "unaligned.h"
>
>
> @@ -107,6 +108,8 @@ learn_execute(const struct ofpact_learn *learn, const
> struct flow *flow,
> fm->importance = 0;
> fm->buffer_id = UINT32_MAX;
> fm->out_port = OFPP_NONE;
> + fm->match.flow.tunnel.metadata.present.map = 0;
This line should already be covered by the call to match_init_catchall() above.
> + fm->ofpacts_tlv_bitmap = 0;
> fm->flags = 0;
> if (learn->flags & NX_LEARN_F_SEND_FLOW_REM) {
> fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
> @@ -136,6 +139,8 @@ learn_execute(const struct ofpact_learn *learn, const
> struct flow *flow,
> switch (spec->dst_type) {
> case NX_LEARN_DST_MATCH:
> mf_write_subfield(&spec->dst, &value, &fm->match);
> + mf_vl_mff_set_tlv_bitmap(
> + spec->dst.field,
> &fm->match.flow.tunnel.metadata.present.map);
> break;
>
> case NX_LEARN_DST_LOAD:
> @@ -145,6 +150,7 @@ learn_execute(const struct ofpact_learn *learn, const
> struct flow *flow,
> spec->n_bits);
> bitwise_one(ofpact_set_field_mask(sf), spec->dst.field->n_bytes,
> spec->dst.ofs, spec->n_bits);
> + mf_vl_mff_set_tlv_bitmap(spec->dst.field,
> &fm->ofpacts_tlv_bitmap);
> break;
>
> case NX_LEARN_DST_OUTPUT:
I think we should also set the tlv bitmap in this LEARN_DST_OUTPUT
case? IIUC this uses an OXM field to determine the output port. I
believe it's possible for someone to use a TLV to specify an output
action using LEARN - so it would set the bitmap in the same way that
the NX_LEARN_DST_LOAD case does.
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 40704e628aaa..7807b3055f5b 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -27,6 +27,8 @@
> #include "openvswitch/dynamic-string.h"
> #include "nx-match.h"
> #include "openvswitch/ofp-util.h"
> +#include "ofproto/ofproto-provider.h"
> +#include "ovs-atomic.h"
> #include "ovs-rcu.h"
> #include "ovs-thread.h"
> #include "packets.h"
> @@ -2646,6 +2648,7 @@ field_array_set(enum mf_field_id id, const union
> mf_value *value,
> * struct vl_mff_map.*/
> struct vl_mf_field {
> struct mf_field mf;
> + struct ovs_refcount ref_cnt;
> struct cmap_node cmap_node; /* In ofproto->vl_mff_map->cmap. */
> };
>
> @@ -2655,17 +2658,25 @@ mf_field_hash(uint32_t key)
> return hash_int(key, 0);
> }
>
> -void
> -mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> +enum ofperr
> +mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool clear)
> OVS_REQUIRES(vl_mff_map->mutex)
> {
> struct vl_mf_field *vmf;
>
> CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
> - cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> - mf_field_hash(vmf->mf.id));
> - ovsrcu_postpone(free, vmf);
> + if (clear) {
> + cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> + mf_field_hash(vmf->mf.id));
> + ovsrcu_postpone(free, vmf);
> + } else {
> + if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> + return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> + }
> + }
> }
> +
> + return 0;
> }
I found this a bit confusing. mf_vl_mff_map_clear() has a boolean,
also named 'clear' that determines whether it will actually clear or
not. If you call a function like "clear(clear=false)", what would it
intuitively do?
I see two calling conventions - first, for TLV clear command from controller:
error = mf_vl_mff_map_clear(vl_mff_map, false);
if (error)
return error;
error = mf_vl_mff_map_clear(vl_mff_map, true);
Second, from ofproto cleanup code:
mf_vl_mff_map_clear(vl_mff_map, true);
I suggest that this would be easier to understand if the function were
structured with the the bool to be 'force':
enum ofperr
mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool force)
OVS_REQUIRES(vl_mff_map->mutex)
{
struct vl_mf_field *vmf;
/* First, check whether any flows still refer to the current TLV
map. */
if (!force) {
CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
return OFPERR_NXTTMFC_INVALID_TLV_DEL;
}
}
}
CMAP_FOR_EACH (vmf, cmap_node, &vl_mff_map->cmap) {
cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
mf_field_hash(vmf->mf.id));
ovsrcu_postpone(free, vmf);
}
return 0;
}
You may also consider refactoring the ovsrcu_postpone(free, vmf) into
a separate, vmf_delete() function which looks something like this:
static void vmf_delete(struct vl_mf_field *vmf)
{
if (ovs_refcount_unref(&vmf->ref_cnt) == 1) {
/* Postpone as this function is typically called immediately
after removing from cmap. */
ovsrcu_postpone(free, vmf);
} else {
VLOG_WARN_RL(&rl, "Attempted to delete VMF %s but refcount is
nonzero!",
vmf->mf.name);
}
}
This could be reused from the other delete locations as well.
> static struct vl_mf_field *
> @@ -2697,53 +2708,100 @@ mf_get_vl_mff(const struct mf_field *mff,
> return NULL;
> }
>
> -/* Updates the tun_metadata mf_field in 'vl_mff_map' according to 'ttm'.
> - * This function is supposed to be invoked after tun_metadata_table_mod(). */
> -enum ofperr
> -mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
> - const struct ofputil_tlv_table_mod *ttm)
> +static enum ofperr
> +mf_vl_mff_map_del(struct vl_mff_map *vl_mff_map,
> + const struct ofputil_tlv_table_mod *ttm, bool del)
Similarly, I find it confusing to reason about a function that can be
called like "delete(delete=false)".
> OVS_REQUIRES(vl_mff_map->mutex)
> {
> struct ofputil_tlv_map *tlv_map;
> -
> - if (ttm->command == NXTTMC_CLEAR) {
> - mf_vl_mff_map_clear(vl_mff_map);
> - return 0;
> - }
> + struct vl_mf_field *vmf;
> + unsigned int idx;
>
> LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> - unsigned int idx = MFF_TUN_METADATA0 + tlv_map->index;
> - struct vl_mf_field *vmf;
> -
> + idx = MFF_TUN_METADATA0 + tlv_map->index;
> if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
> return OFPERR_NXTTMFC_BAD_FIELD_IDX;
> }
>
> - switch (ttm->command) {
> - case NXTTMC_ADD:
> - vmf = xmalloc(sizeof *vmf);
> - vmf->mf = mf_fields[idx];
> - vmf->mf.n_bytes = tlv_map->option_len;
> - vmf->mf.n_bits = tlv_map->option_len * 8;
> - vmf->mf.mapped = true;
> -
> - cmap_insert(&vl_mff_map->cmap, &vmf->cmap_node,
> - mf_field_hash(idx));
> - break;
> -
> - case NXTTMC_DELETE:
> - vmf = mf_get_vl_mff__(idx, vl_mff_map);
> - if (vmf) {
> + vmf = mf_get_vl_mff__(idx, vl_mff_map);
> + if (vmf) {
> + if (del == true) {
> cmap_remove(&vl_mff_map->cmap, &vmf->cmap_node,
> mf_field_hash(idx));
> ovsrcu_postpone(free, vmf);
> + } else {
> + if (ovs_refcount_read(&vmf->ref_cnt) != 1) {
> + return OFPERR_NXTTMFC_INVALID_TLV_DEL;
> + }
> }
> - break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static enum ofperr
> +mf_vl_mff_map_add(struct vl_mff_map *vl_mff_map,
> + const struct ofputil_tlv_table_mod *ttm)
> + OVS_REQUIRES(vl_mff_map->mutex)
> +{
> + struct ofputil_tlv_map *tlv_map;
> + struct vl_mf_field *vmf;
> + unsigned int idx;
> +
> + LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
> + idx = MFF_TUN_METADATA0 + tlv_map->index;
> + if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
> + return OFPERR_NXTTMFC_BAD_FIELD_IDX;
> + }
> +
> + vmf = xmalloc(sizeof *vmf);
> + vmf->mf = mf_fields[idx];
> + vmf->mf.n_bytes = tlv_map->option_len;
> + vmf->mf.n_bits = tlv_map->option_len * 8;
> + vmf->mf.mapped = true;
> + ovs_refcount_init(&vmf->ref_cnt);
> +
> + cmap_insert(&vl_mff_map->cmap, &vmf->cmap_node,
> + mf_field_hash(idx));
> + }
> +
> + return 0;
> +}
>
> - case NXTTMC_CLEAR:
> - default:
> - OVS_NOT_REACHED();
> +/* Updates the tun_metadata mf_field in 'vl_mff_map' according to 'ttm'.
> + * This function is supposed to be invoked after tun_metadata_table_mod().
I realise the above sentence is from the existing code, but it's
clearer if this comment is prescriptive, ie:
"This function must be invoked after tun_metadata_table_mod()."
> + * Returns OFPERR_NXTTMFC_BAD_FIELD_IDX, if the index for the vl_mf_field is
> + * invalid.
> + * Returns OFPERR_NXTTMFC_INVALID_TLV_DEL, if 'ttm' tries to delete an
> + * vl_mf_field that is still used by any active flow.*/
> +enum ofperr
> +mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
> + const struct ofputil_tlv_table_mod *ttm)
> + OVS_REQUIRES(vl_mff_map->mutex)
> +{
> + enum ofperr error;
> +
> + switch (ttm->command) {
> + case NXTTMC_ADD:
> + return mf_vl_mff_map_add(vl_mff_map, ttm);
> +
> + case NXTTMC_DELETE:
> + error = mf_vl_mff_map_del(vl_mff_map, ttm, false);
> + if (error) {
> + return error;
> }
> + return mf_vl_mff_map_del(vl_mff_map, ttm, true);
> +
> + case NXTTMC_CLEAR:
> + error = mf_vl_mff_map_clear(vl_mff_map, false);
> + if (error) {
> + return error;
> + }
> + return mf_vl_mff_map_clear(vl_mff_map, true);
> +
> + default:
> + OVS_NOT_REACHED();
> }
>
> return 0;
> @@ -2756,3 +2814,67 @@ mf_vl_mff_invalid(const struct mf_field *mff, const
> struct vl_mff_map *map)
> {
> return map && mff && mff->variable_len && !mff->mapped;
> }
> +
> +void
> +mf_vl_mff_set_tlv_bitmap(const struct mf_field *mff, uint64_t *tlv_bitmap)
> +{
> + if (mff && mff->mapped) {
> + ULLONG_SET1(*tlv_bitmap, mff->id - MFF_TUN_METADATA0);
I wonder if we should add some runtime check here to ovs_assert() that
we're not trying to set a bit beyond the length of the field. I think
that if the second parameter of the ULLONG_SET1() is >64 then some
tests will fail, but it's not as obvious as an assert right here where
you're attempting to set the bit.
> + }
> +}
> +
> +/* If the 'mff' is a valid variable length mf_field, udpates the
> 'tlv_bitmap'.
> + * Returns OFPERR_NXFMFC_INVALID_TLV_FIELD if the variable length mf_field
> + * is invalid. */
> +enum ofperr
> +mf_check_vl_mff(const struct mf_field *mff, const struct vl_mff_map *map,
> + uint64_t *tlv_bitmap)
Could we rename this to mf_set_vl_mff_bitmap()?
> +{
> + if (mf_vl_mff_invalid(mff, map)) {
> + return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> + }
> + mf_vl_mff_set_tlv_bitmap(mff, tlv_bitmap);
> +
> + return 0;
> +}
> +
> +static void
> +mf_vl_mff_ref_cnt_mod(const struct vl_mff_map *map, uint64_t tlv_bitmap,
> + bool ref)
> +{
> + struct vl_mf_field *vmf;
> + int i;
> +
> + if (map) {
> + ULLONG_FOR_EACH_1 (i, tlv_bitmap) {
> + vmf = mf_get_vl_mff__(i + MFF_TUN_METADATA0, map);
> + if (vmf) {
> + if (ref) {
> + ovs_refcount_ref(&vmf->ref_cnt);
> + } else {
> + ovs_refcount_unref(&vmf->ref_cnt);
We should warn if we're trying to unref and this is the final ref,
because then we're not freeing the object.
> + }
> + } else {
> + VLOG_WARN("Invalid TLV index %d.", i);
> + }
> + }
> + }
> +}
> +
> +void
> +mf_vl_mff_add_ref_cnt_from_rule(const struct rule *rule)
> +{
> + mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->match_tlv_bitmap,
> + true);
> + mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->ofpacts_tlv_bitmap,
> + true);
> +}
> +
> +void
> +mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *rule)
> +{
> + mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map, rule->match_tlv_bitmap,
> + false);
> + mf_vl_mff_ref_cnt_mod(&rule->ofproto->vl_mff_map,
> rule->ofpacts_tlv_bitmap,
> + false);
> +}
These should probably just provide the vl_mff_map and bitmap rather
than introducing a dependency on ofproto/ofproto-provider.h and struct
rule.
Usually functions in ovs use the format foo_ref() and foo_unref() to
add/remove refcounts. Could you rename these to keep with that style?
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index ce80f57e8df3..584e5cd4e847 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -405,7 +405,7 @@ static enum ofperr ofpacts_pull_openflow_actions__(
> struct ofpbuf *openflow, unsigned int actions_len,
> enum ofp_version version, uint32_t allowed_ovsinsts,
> struct ofpbuf *ofpacts, enum ofpact_type outer_action,
> - const struct vl_mff_map *vl_mff_map);
> + const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap);
> static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
> const char *s_, struct ofpbuf *ofpacts,
> enum ofputil_protocol *usable_protocols,
> @@ -1117,13 +1117,20 @@ struct nx_action_output_reg2 {
> };
> OFP_ASSERT(sizeof(struct nx_action_output_reg2) == 24);
>
> +#define CHECK_VL_MFF(FIELD, VL_MFF_MAP, BITMAP, ERR) \
> +ERR = mf_check_vl_mff(FIELD, VL_MFF_MAP, BITMAP); \
> +if (ERR) { \
> + return ERR; \
> +}
> +
I realise there's a few spots in OVS code that use a style like this,
and it's (a little) easier to write.. but for reading, it's easy to
miss the fact that this macro may cause functions to return. For
instance, we used to do some MPLS checking like this and we ended up
removing it to improve readability. I think we should just expand this
out in the 5 or 6 places that this is used in this file.
> static enum ofperr
> decode_NXAST_RAW_OUTPUT_REG(const struct nx_action_output_reg *naor,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> struct ofpact_output_reg *output_reg;
> + enum ofperr error;
>
> if (!is_all_zeros(naor->zero, sizeof naor->zero)) {
> return OFPERR_OFPBAC_BAD_ARGUMENT;
> @@ -1136,9 +1143,7 @@ decode_NXAST_RAW_OUTPUT_REG(const struct
> nx_action_output_reg *naor,
> output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
> output_reg->max_len = ntohs(naor->max_len);
>
> - if (mf_vl_mff_invalid(output_reg->src.field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(output_reg->src.field, vl_mff_map, tlv_bitmap, error);
>
> return mf_check_src(&output_reg->src, NULL);
> }
> @@ -1147,7 +1152,7 @@ static enum ofperr
> decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> struct ofpact_output_reg *output_reg;
> output_reg = ofpact_put_OUTPUT_REG(out);
> @@ -1164,6 +1169,8 @@ decode_NXAST_RAW_OUTPUT_REG2(const struct
> nx_action_output_reg2 *naor,
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(output_reg->src.field, tlv_bitmap);
> +
I wonder if it's cleaner to fold this into nx_pull_header().
It's not so obvious why the ofpacts decode functions in this file
inconsistently need to call the mf_check_vl_mff() and/or
mf_vl_mff_set_tlv_bitmap(), which makes it a little more difficult to
verify (and easier to mess up if new actions are added). It'd be nice
to make this more consistent.
> if (!is_all_zeros(b.data, b.size)) {
> return OFPERR_NXBRC_MUST_BE_ZERO;
> }
> @@ -1286,7 +1293,8 @@ OFP_ASSERT(sizeof(struct nx_action_bundle) == 32);
>
> static enum ofperr
> decode_bundle(bool load, const struct nx_action_bundle *nab,
> - const struct vl_mff_map *vl_mff_map, struct ofpbuf *ofpacts)
> + const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap,
> + struct ofpbuf *ofpacts)
> {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> struct ofpact_bundle *bundle;
> @@ -1326,9 +1334,7 @@ decode_bundle(bool load, const struct nx_action_bundle
> *nab,
> bundle->dst.field = mf_from_nxm_header(ntohl(nab->dst), vl_mff_map);
> bundle->dst.ofs = nxm_decode_ofs(nab->ofs_nbits);
> bundle->dst.n_bits = nxm_decode_n_bits(nab->ofs_nbits);
> - if (mf_vl_mff_invalid(bundle->dst.field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(bundle->dst.field, vl_mff_map, tlv_bitmap, error);
>
> if (bundle->dst.n_bits < 16) {
> VLOG_WARN_RL(&rl, "bundle_load action requires at least 16 bit "
> @@ -1369,16 +1375,16 @@ decode_NXAST_RAW_BUNDLE(const struct nx_action_bundle
> *nab,
> enum ofp_version ofp_version OVS_UNUSED,
> struct ofpbuf *out)
> {
> - return decode_bundle(false, nab, NULL, out);
> + return decode_bundle(false, nab, NULL, NULL, out);
> }
>
> static enum ofperr
> decode_NXAST_RAW_BUNDLE_LOAD(const struct nx_action_bundle *nab,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> - return decode_bundle(true, nab, vl_mff_map, out);
> + return decode_bundle(true, nab, vl_mff_map, tlv_bitmap, out);
> }
>
> static void
> @@ -2282,7 +2288,7 @@ static enum ofperr
> decode_copy_field__(ovs_be16 src_offset, ovs_be16 dst_offset, ovs_be16
> n_bits,
> const void *action, ovs_be16 action_len, size_t
> oxm_offset,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
> move->ofpact.raw = ONFACT_RAW13_COPY_FIELD;
> @@ -2298,10 +2304,13 @@ decode_copy_field__(ovs_be16 src_offset, ovs_be16
> dst_offset, ovs_be16 n_bits,
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> +
> error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
>
> if (!is_all_zeros(b.data, b.size)) {
> return OFPERR_NXBRC_MUST_BE_ZERO;
> @@ -2314,31 +2323,31 @@ static enum ofperr
> decode_OFPAT_RAW15_COPY_FIELD(const struct ofp15_action_copy_field *oacf,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
> oacf->n_bits, oacf, oacf->len,
> OBJECT_OFFSETOF(oacf, pad2), vl_mff_map,
> - ofpacts);
> + tlv_bitmap, ofpacts);
> }
>
> static enum ofperr
> decode_ONFACT_RAW13_COPY_FIELD(const struct onf_action_copy_field *oacf,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> return decode_copy_field__(oacf->src_offset, oacf->dst_offset,
> oacf->n_bits, oacf, oacf->len,
> OBJECT_OFFSETOF(oacf, pad3), vl_mff_map,
> - ofpacts);
> + tlv_bitmap, ofpacts);
> }
>
> static enum ofperr
> decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
> move->ofpact.raw = NXAST_RAW_REG_MOVE;
> @@ -2354,10 +2363,13 @@ decode_NXAST_RAW_REG_MOVE(const struct
> nx_action_reg_move *narm,
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(move->src.field, tlv_bitmap);
> +
> error = nx_pull_header(&b, vl_mff_map, &move->dst.field, NULL);
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(move->dst.field, tlv_bitmap);
>
> if (!is_all_zeros(b.data, b.size)) {
> return OFPERR_NXBRC_MUST_BE_ZERO;
> @@ -2481,7 +2493,7 @@ OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
> static enum ofperr
> decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
> bool may_mask, const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpbuf b = ofpbuf_const_initializer(oasf, ntohs(oasf->len));
> ofpbuf_pull(&b, OBJECT_OFFSETOF(oasf, pad));
> @@ -2495,6 +2507,8 @@ decode_ofpat_set_field(const struct
> ofp12_action_set_field *oasf,
> ? OFPERR_OFPBAC_BAD_SET_MASK
> : error);
> }
> + mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> +
> if (!may_mask) {
> memset(&mask, 0xff, field->n_bytes);
> }
> @@ -2540,25 +2554,26 @@ static enum ofperr
> decode_OFPAT_RAW12_SET_FIELD(const struct ofp12_action_set_field *oasf,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> - return decode_ofpat_set_field(oasf, false, vl_mff_map, ofpacts);
> + return decode_ofpat_set_field(oasf, false, vl_mff_map, tlv_bitmap,
> + ofpacts);
> }
>
> static enum ofperr
> decode_OFPAT_RAW15_SET_FIELD(const struct ofp12_action_set_field *oasf,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> - return decode_ofpat_set_field(oasf, true, vl_mff_map, ofpacts);
> + return decode_ofpat_set_field(oasf, true, vl_mff_map, tlv_bitmap,
> ofpacts);
> }
>
> static enum ofperr
> decode_NXAST_RAW_REG_LOAD(const struct nx_action_reg_load *narl,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> struct mf_subfield dst;
> enum ofperr error;
> @@ -2566,9 +2581,7 @@ decode_NXAST_RAW_REG_LOAD(const struct
> nx_action_reg_load *narl,
> dst.field = mf_from_nxm_header(ntohl(narl->dst), vl_mff_map);
> dst.ofs = nxm_decode_ofs(narl->ofs_nbits);
> dst.n_bits = nxm_decode_n_bits(narl->ofs_nbits);
> - if (mf_vl_mff_invalid(dst.field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(dst.field, vl_mff_map, tlv_bitmap, error);
>
> error = mf_check_dst(&dst, NULL);
> if (error) {
> @@ -2596,7 +2609,7 @@ static enum ofperr
> decode_NXAST_RAW_REG_LOAD2(const struct ext_action_header *eah,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> struct ofpbuf b = ofpbuf_const_initializer(eah, ntohs(eah->len));
> ofpbuf_pull(&b, OBJECT_OFFSETOF(eah, pad));
> @@ -2607,6 +2620,8 @@ decode_NXAST_RAW_REG_LOAD2(const struct
> ext_action_header *eah,
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(field, tlv_bitmap);
> +
> if (!is_all_zeros(b.data, b.size)) {
> return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> }
> @@ -3109,7 +3124,7 @@ OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
>
> static enum ofperr
> decode_stack_action(const struct nx_action_stack *nasp,
> - const struct vl_mff_map *vl_mff_map,
> + const struct vl_mff_map *vl_mff_map, uint64_t
> *tlv_bitmap,
> struct ofpact_stack *stack_action)
> {
> stack_action->subfield.ofs = ntohs(nasp->offset);
> @@ -3121,6 +3136,8 @@ decode_stack_action(const struct nx_action_stack *nasp,
> if (error) {
> return error;
> }
> + mf_vl_mff_set_tlv_bitmap(stack_action->subfield.field, tlv_bitmap);
> +
> stack_action->subfield.n_bits = ntohs(*(const ovs_be16 *) b.data);
> ofpbuf_pull(&b, 2);
> if (!is_all_zeros(b.data, b.size)) {
> @@ -3134,10 +3151,11 @@ static enum ofperr
> decode_NXAST_RAW_STACK_PUSH(const struct nx_action_stack *nasp,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpact_stack *push = ofpact_put_STACK_PUSH(ofpacts);
> - enum ofperr error = decode_stack_action(nasp, vl_mff_map, push);
> + enum ofperr error = decode_stack_action(nasp, vl_mff_map, tlv_bitmap,
> + push);
> return error ? error : nxm_stack_push_check(push, NULL);
> }
>
> @@ -3145,10 +3163,11 @@ static enum ofperr
> decode_NXAST_RAW_STACK_POP(const struct nx_action_stack *nasp,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpact_stack *pop = ofpact_put_STACK_POP(ofpacts);
> - enum ofperr error = decode_stack_action(nasp, vl_mff_map, pop);
> + enum ofperr error = decode_stack_action(nasp, vl_mff_map, tlv_bitmap,
> + pop);
> return error ? error : nxm_stack_pop_check(pop, NULL);
> }
>
> @@ -4279,14 +4298,14 @@ get_be32(const void **pp)
>
> static enum ofperr
> get_subfield(int n_bits, const void **p, struct mf_subfield *sf,
> - const struct vl_mff_map *vl_mff_map)
> + const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap)
> {
> + enum ofperr error;
> +
> sf->field = mf_from_nxm_header(ntohl(get_be32(p)), vl_mff_map);
> sf->ofs = ntohs(get_be16(p));
> sf->n_bits = n_bits;
> - if (mf_vl_mff_invalid(sf->field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(sf->field, vl_mff_map, tlv_bitmap, error);
> return 0;
> }
>
> @@ -4319,7 +4338,7 @@ static enum ofperr
> decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *ofpacts)
> + uint64_t *tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpact_learn *learn;
> const void *p, *end;
> @@ -4384,7 +4403,8 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn
> *nal,
> unsigned int imm_bytes = 0;
> enum ofperr error;
> if (spec->src_type == NX_LEARN_SRC_FIELD) {
> - error = get_subfield(spec->n_bits, &p, &spec->src, vl_mff_map);
> + error = get_subfield(spec->n_bits, &p, &spec->src, vl_mff_map,
> + tlv_bitmap);
> if (error) {
> return error;
> }
> @@ -4399,7 +4419,8 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn
> *nal,
> /* Get the destination. */
> if (spec->dst_type == NX_LEARN_DST_MATCH ||
> spec->dst_type == NX_LEARN_DST_LOAD) {
> - error = get_subfield(spec->n_bits, &p, &spec->dst, vl_mff_map);
> + error = get_subfield(spec->n_bits, &p, &spec->dst, vl_mff_map,
> + tlv_bitmap);
> if (error) {
> return error;
> }
> @@ -4649,11 +4670,12 @@ static enum ofperr
> decode_NXAST_RAW_MULTIPATH(const struct nx_action_multipath *nam,
> enum ofp_version ofp_version OVS_UNUSED,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> uint32_t n_links = ntohs(nam->max_link) + 1;
> size_t min_n_bits = log_2_ceil(n_links);
> struct ofpact_multipath *mp;
> + enum ofperr error;
>
> mp = ofpact_put_MULTIPATH(out);
> mp->fields = ntohs(nam->fields);
> @@ -4665,9 +4687,7 @@ decode_NXAST_RAW_MULTIPATH(const struct
> nx_action_multipath *nam,
> mp->dst.ofs = nxm_decode_ofs(nam->ofs_nbits);
> mp->dst.n_bits = nxm_decode_n_bits(nam->ofs_nbits);
>
> - if (mf_vl_mff_invalid(mp->dst.field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(mp->dst.field, vl_mff_map, tlv_bitmap, error);
>
> if (!flow_hash_fields_valid(mp->fields)) {
> VLOG_WARN_RL(&rl, "unsupported fields %d", (int) mp->fields);
> @@ -4855,7 +4875,7 @@ static enum ofperr
> decode_NXAST_RAW_CLONE(const struct ext_action_header *eah,
> enum ofp_version ofp_version,
> const struct vl_mff_map *vl_mff_map,
> - struct ofpbuf *out)
> + uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> int error;
> struct ofpbuf openflow;
> @@ -4869,7 +4889,7 @@ decode_NXAST_RAW_CLONE(const struct ext_action_header
> *eah,
> error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
> ofp_version,
> 1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> - out, 0, vl_mff_map);
> + out, 0, vl_mff_map, tlv_bitmap);
> clone = ofpbuf_push_uninit(out, sizeof *clone);
> out->header = &clone->ofpact;
> ofpact_finish_CLONE(out, &clone);
> @@ -5316,7 +5336,7 @@ OFP_ASSERT(sizeof(struct nx_action_conntrack) == 24);
> static enum ofperr
> decode_ct_zone(const struct nx_action_conntrack *nac,
> struct ofpact_conntrack *out,
> - const struct vl_mff_map *vl_mff_map)
> + const struct vl_mff_map *vl_mff_map, uint64_t *tlv_bitmap)
> {
> if (nac->zone_src) {
> enum ofperr error;
> @@ -5325,9 +5345,7 @@ decode_ct_zone(const struct nx_action_conntrack *nac,
> vl_mff_map);
> out->zone_src.ofs = nxm_decode_ofs(nac->zone_ofs_nbits);
> out->zone_src.n_bits = nxm_decode_n_bits(nac->zone_ofs_nbits);
> - if (mf_vl_mff_invalid(out->zone_src.field, vl_mff_map)) {
> - return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> - }
> + CHECK_VL_MFF(out->zone_src.field, vl_mff_map, tlv_bitmap, error);
>
> error = mf_check_src(&out->zone_src, NULL);
> if (error) {
> @@ -5350,13 +5368,14 @@ decode_ct_zone(const struct nx_action_conntrack *nac,
> static enum ofperr
> decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
> enum ofp_version ofp_version,
> - const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> + const struct vl_mff_map *vl_mff_map, uint64_t
> *tlv_bitmap,
> + struct ofpbuf *out)
> {
> const size_t ct_offset = ofpacts_pull(out);
> struct ofpact_conntrack *conntrack = ofpact_put_CT(out);
> conntrack->flags = ntohs(nac->flags);
>
> - int error = decode_ct_zone(nac, conntrack, vl_mff_map);
> + int error = decode_ct_zone(nac, conntrack, vl_mff_map, tlv_bitmap);
> if (error) {
> goto out;
> }
> @@ -5370,7 +5389,8 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack
> *nac,
> error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
> ofp_version,
> 1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> - out, OFPACT_CT, vl_mff_map);
> + out, OFPACT_CT, vl_mff_map,
> + tlv_bitmap);
> if (error) {
> goto out;
> }
> @@ -6256,7 +6276,8 @@ log_bad_action(const struct ofp_action_header *actions,
> size_t actions_len,
> static enum ofperr
> ofpacts_decode(const void *actions, size_t actions_len,
> enum ofp_version ofp_version,
> - const struct vl_mff_map *vl_mff_map, struct ofpbuf *ofpacts)
> + const struct vl_mff_map *vl_mff_map,
> + uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts)
> {
> struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len);
> while (openflow.size) {
> @@ -6268,7 +6289,7 @@ ofpacts_decode(const void *actions, size_t actions_len,
> error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg);
> if (!error) {
> error = ofpact_decode(action, raw, ofp_version, arg, vl_mff_map,
> - ofpacts);
> + ofpacts_tlv_bitmap, ofpacts);
> }
>
> if (error) {
> @@ -6286,7 +6307,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
> uint32_t allowed_ovsinsts,
> struct ofpbuf *ofpacts,
> enum ofpact_type outer_action,
> - const struct vl_mff_map *vl_mff_map)
> + const struct vl_mff_map *vl_mff_map,
> + uint64_t *ofpacts_tlv_bitmap)
> {
> const struct ofp_action_header *actions;
> size_t orig_size = ofpacts->size;
> @@ -6306,7 +6328,8 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
> return OFPERR_OFPBRC_BAD_LEN;
> }
>
> - error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> ofpacts);
> + error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> + ofpacts_tlv_bitmap, ofpacts);
> if (error) {
> ofpacts->size = orig_size;
> return error;
> @@ -6342,11 +6365,13 @@ ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
> unsigned int actions_len,
> enum ofp_version version,
> const struct vl_mff_map *vl_mff_map,
> + uint64_t *ofpacts_tlv_bitmap,
> struct ofpbuf *ofpacts)
> {
> return ofpacts_pull_openflow_actions__(openflow, actions_len, version,
> 1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
> - ofpacts, 0, vl_mff_map);
> + ofpacts, 0, vl_mff_map,
> + ofpacts_tlv_bitmap);
> }
>
> /* OpenFlow 1.1 actions. */
> @@ -6586,13 +6611,15 @@ static enum ofperr
> ofpacts_decode_for_action_set(const struct ofp_action_header *in,
> size_t n_in, enum ofp_version version,
> const struct vl_mff_map *vl_mff_map,
> + uint64_t *ofpacts_tlv_bitmap,
> struct ofpbuf *out)
> {
> enum ofperr error;
> struct ofpact *a;
> size_t start = out->size;
>
> - error = ofpacts_decode(in, n_in, version, vl_mff_map, out);
> + error = ofpacts_decode(in, n_in, version, vl_mff_map, ofpacts_tlv_bitmap,
> + out);
>
> if (error) {
> return error;
> @@ -6891,6 +6918,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> unsigned int instructions_len,
> enum ofp_version version,
> const struct vl_mff_map *vl_mff_map,
> + uint64_t *ofpacts_tlv_bitmap,
> struct ofpbuf *ofpacts)
> {
> const struct ofp11_instruction *instructions;
> @@ -6902,7 +6930,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> return ofpacts_pull_openflow_actions__(openflow, instructions_len,
> version,
> (1u << N_OVS_INSTRUCTIONS) -
> 1,
> - ofpacts, 0, vl_mff_map);
> + ofpacts, 0, vl_mff_map,
> + ofpacts_tlv_bitmap);
> }
>
> if (instructions_len % OFP11_INSTRUCTION_ALIGN != 0) {
> @@ -6946,7 +6975,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
> &actions, &actions_len);
> error = ofpacts_decode(actions, actions_len, version, vl_mff_map,
> - ofpacts);
> + ofpacts_tlv_bitmap, ofpacts);
> if (error) {
> goto exit;
> }
> @@ -6966,7 +6995,8 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
> get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
> &actions, &actions_len);
> error = ofpacts_decode_for_action_set(actions, actions_len,
> - version, vl_mff_map, ofpacts);
> + version, vl_mff_map,
> + ofpacts_tlv_bitmap, ofpacts);
> if (error) {
> goto exit;
> }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index f7da90b276f9..96b4f8d22333 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1726,8 +1726,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
> return OFPERR_OFPFMFC_BAD_COMMAND;
> }
>
> + fm->ofpacts_tlv_bitmap = 0;
> error = ofpacts_pull_openflow_instructions(&b, b.size, oh->version,
> - vl_mff_map, ofpacts);
> + vl_mff_map,
> + &fm->ofpacts_tlv_bitmap,
> + ofpacts);
> if (error) {
> return error;
> }
> @@ -3009,7 +3012,7 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
> }
>
> if (ofpacts_pull_openflow_instructions(msg, instructions_len,
> oh->version,
> - NULL, ofpacts)) {
> + NULL, NULL, ofpacts)) {
> VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
> return EINVAL;
> }
> @@ -4034,7 +4037,7 @@ parse_actions_property(struct ofpbuf *property, enum
> ofp_version version,
> }
>
> return ofpacts_pull_openflow_actions(property, property->size,
> - version, NULL, ofpacts);
> + version, NULL, NULL, ofpacts);
> }
>
> /* This is like ofputil_decode_packet_in(), except that it decodes the
> @@ -4189,7 +4192,8 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
> }
>
> error = ofpacts_pull_openflow_actions(&b, ntohs(opo->actions_len),
> - oh->version, NULL, ofpacts);
> + oh->version, NULL, NULL,
> + ofpacts);
> if (error) {
> return error;
> }
> @@ -4201,7 +4205,8 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
> po->in_port = u16_to_ofp(ntohs(opo->in_port));
>
> error = ofpacts_pull_openflow_actions(&b, ntohs(opo->actions_len),
> - oh->version, NULL, ofpacts);
> + oh->version, NULL, NULL,
> + ofpacts);
> if (error) {
> return error;
> }
> @@ -6743,7 +6748,7 @@ ofputil_decode_flow_update(struct ofputil_flow_update
> *update,
>
> actions_len = length - sizeof *nfuf - ROUND_UP(match_len, 8);
> error = ofpacts_pull_openflow_actions(msg, actions_len, oh->version,
> - NULL, ofpacts);
> + NULL, NULL, ofpacts);
> if (error) {
> return error;
> }
> @@ -8727,7 +8732,7 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t
> buckets_length,
>
> ofpbuf_init(&ofpacts, 0);
> error = ofpacts_pull_openflow_actions(msg, ob_len - sizeof *ob,
> - version, NULL, &ofpacts);
> + version, NULL, NULL, &ofpacts);
> if (error) {
> ofpbuf_uninit(&ofpacts);
> ofputil_bucket_list_destroy(buckets);
> @@ -8801,7 +8806,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t
> buckets_length,
> buckets_length -= ob_len;
>
> err = ofpacts_pull_openflow_actions(msg, actions_len, version,
> - NULL, &ofpacts);
> + NULL, NULL, &ofpacts);
> if (err) {
> goto err;
> }
> diff --git a/lib/vl-mff-map.h b/lib/vl-mff-map.h
> index 1c29385782ec..a47b8aa8a6f9 100644
> --- a/lib/vl-mff-map.h
> +++ b/lib/vl-mff-map.h
> @@ -20,6 +20,8 @@
> #include "cmap.h"
> #include "openvswitch/thread.h"
>
> +struct rule;
> +
> /* Variable length mf_fields mapping map. This is a single writer,
> * multiple-reader hash table that a writer must hold the following mutex
> * to access this map. */
> @@ -29,7 +31,7 @@ struct vl_mff_map {
> };
>
> /* Variable length fields. */
> -void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
> +enum ofperr mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map, bool)
> OVS_REQUIRES(vl_mff_map->mutex);
> enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
> struct vl_mff_map *vl_mff_map, const struct ofputil_tlv_table_mod *)
> @@ -37,5 +39,9 @@ enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
> const struct mf_field * mf_get_vl_mff(const struct mf_field *,
> const struct vl_mff_map *);
> bool mf_vl_mff_invalid(const struct mf_field *, const struct vl_mff_map *);
> -
> +void mf_vl_mff_set_tlv_bitmap(const struct mf_field *, uint64_t *);
> +enum ofperr mf_check_vl_mff(const struct mf_field *, const struct vl_mff_map
> *,
> + uint64_t *);
> +void mf_vl_mff_add_ref_cnt_from_rule(const struct rule *);
> +void mf_vl_mff_remove_ref_cnt_from_rule(const struct rule *);
> #endif /* vl-mff-map.h */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7d3e929f21e3..dab74e3a195f 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -423,6 +423,10 @@ struct rule {
>
> /* Must hold 'mutex' for both read/write, 'ofproto_mutex' not needed. */
> long long int modified OVS_GUARDED; /* Time of last modification. */
> +
> + /* 1-bit for each present TLV in flow match / action. */
> + uint64_t match_tlv_bitmap;
> + uint64_t ofpacts_tlv_bitmap;
> };
>
> void ofproto_rule_ref(struct rule *);
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 699c37cd5c3e..cf4db291e12a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -228,6 +228,8 @@ static enum ofperr ofproto_rule_create(struct ofproto *,
> struct cls_rule *,
> uint16_t importance,
> const struct ofpact *ofpacts,
> size_t ofpacts_len,
> + uint64_t match_tlv_bitmap,
> + uint64_t ofpacts_tlv_bitmap,
> struct rule **new_rule)
> OVS_NO_THREAD_SAFETY_ANALYSIS;
>
> @@ -1580,12 +1582,6 @@ ofproto_destroy__(struct ofproto *ofproto)
> tun_metadata_free(ovsrcu_get_protected(struct tun_table *,
> &ofproto->metadata_tab));
>
> - ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> - mf_vl_mff_map_clear(&ofproto->vl_mff_map);
> - ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> - cmap_destroy(&ofproto->vl_mff_map.cmap);
> - ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> -
> free(ofproto->name);
> free(ofproto->type);
> free(ofproto->mfr_desc);
> @@ -1603,6 +1599,12 @@ ofproto_destroy__(struct ofproto *ofproto)
> }
> free(ofproto->tables);
>
> + ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> + mf_vl_mff_map_clear(&ofproto->vl_mff_map, true);
> + ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> + cmap_destroy(&ofproto->vl_mff_map.cmap);
> + ovs_mutex_destroy(&ofproto->vl_mff_map.mutex);
> +
> ovs_assert(hindex_is_empty(&ofproto->cookies));
> hindex_destroy(&ofproto->cookies);
>
Is there a reason for the above two hunks to shift this logic later?
Should this be in a separate patch to explain why, or could it be
dropped?
> @@ -2829,6 +2831,7 @@ rule_destroy_cb(struct rule *rule)
> ofproto_rule_send_removed(rule);
> }
> rule->ofproto->ofproto_class->rule_destruct(rule);
> + mf_vl_mff_remove_ref_cnt_from_rule(rule);
> ofproto_rule_destroy__(rule);
> }
>
> @@ -4741,7 +4744,9 @@ add_flow_init(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> fm->new_cookie, fm->idle_timeout,
> fm->hard_timeout, fm->flags,
> fm->importance, fm->ofpacts,
> - fm->ofpacts_len, &ofm->temp_rule);
> + fm->ofpacts_len,
> +
> fm->match.flow.tunnel.metadata.present.map,
> + fm->ofpacts_tlv_bitmap, &ofm->temp_rule);
> if (error) {
> return error;
> }
> @@ -4856,6 +4861,7 @@ ofproto_rule_create(struct ofproto *ofproto, struct
> cls_rule *cr,
> uint16_t idle_timeout, uint16_t hard_timeout,
> enum ofputil_flow_mod_flags flags, uint16_t importance,
> const struct ofpact *ofpacts, size_t ofpacts_len,
> + uint64_t match_tlv_bitmap, uint64_t ofpacts_tlv_bitmap,
> struct rule **new_rule)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> @@ -4906,6 +4912,9 @@ ofproto_rule_create(struct ofproto *ofproto, struct
> cls_rule *cr,
> }
>
> rule->state = RULE_INITIALIZED;
> + rule->match_tlv_bitmap = match_tlv_bitmap;
> + rule->ofpacts_tlv_bitmap = ofpacts_tlv_bitmap;
> + mf_vl_mff_add_ref_cnt_from_rule(rule);
>
> *new_rule = rule;
> return 0;
> @@ -5003,6 +5012,8 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod
> *ofm)
> rule->importance,
> rule->actions->ofpacts,
> rule->actions->ofpacts_len,
> + rule->match_tlv_bitmap,
> + rule->ofpacts_tlv_bitmap,
> &ofm->temp_rule);
> ovs_mutex_unlock(&rule->mutex);
> if (!error) {
> @@ -5263,6 +5274,11 @@ modify_flows_start__(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> cls_rule_destroy(CONST_CAST(struct cls_rule *, &temp->cr));
> cls_rule_clone(CONST_CAST(struct cls_rule *, &temp->cr),
> &old_rule->cr);
> + if (temp->match_tlv_bitmap != old_rule->match_tlv_bitmap) {
> + mf_vl_mff_remove_ref_cnt_from_rule(temp);
> + temp->match_tlv_bitmap = old_rule->match_tlv_bitmap;
> + mf_vl_mff_add_ref_cnt_from_rule(temp);
> + }
> *CONST_CAST(uint8_t *, &temp->table_id) = old_rule->table_id;
> rule_collection_add(new_rules, temp);
> first = false;
> @@ -5276,6 +5292,8 @@ modify_flows_start__(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> temp->importance,
> temp->actions->ofpacts,
> temp->actions->ofpacts_len,
> + old_rule->match_tlv_bitmap,
> + temp->ofpacts_tlv_bitmap,
> &new_rule);
> if (!error) {
> rule_collection_add(new_rules, new_rule);
> @@ -7784,14 +7802,15 @@ handle_tlv_table_mod(struct ofconn *ofconn, const
> struct ofp_header *oh)
> old_tab = ovsrcu_get_protected(struct tun_table *,
> &ofproto->metadata_tab);
> error = tun_metadata_table_mod(&ttm, old_tab, &new_tab);
> if (!error) {
> - ovsrcu_set(&ofproto->metadata_tab, new_tab);
> - tun_metadata_postpone_free(old_tab);
> -
> ovs_mutex_lock(&ofproto->vl_mff_map.mutex);
> error = mf_vl_mff_map_mod_from_tun_metadata(&ofproto->vl_mff_map,
> &ttm);
> ovs_mutex_unlock(&ofproto->vl_mff_map.mutex);
> }
> + if (!error) {
> + ovsrcu_set(&ofproto->metadata_tab, new_tab);
> + tun_metadata_postpone_free(old_tab);
> + }
Maybe I didn't quite dig deep enough, but what's the implications of
moving this? Could it fix another bug, should it be a separate patch?
>
> ofputil_uninit_tlv_table(&ttm.mappings);
> return error;
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 0380c8481ecf..890b47fe0d4f 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -168,7 +168,8 @@ pinctrl_handle_arp(const struct flow *ip_flow, const
> struct match *md,
>
> reload_metadata(&ofpacts, md);
> enum ofperr error = ofpacts_pull_openflow_actions(userdata,
> userdata->size,
> - version, NULL,
> &ofpacts);
> + version, NULL, NULL,
> + &ofpacts);
> if (error) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_WARN_RL(&rl, "failed to parse arp actions (%s)",
> @@ -1398,7 +1399,8 @@ pinctrl_handle_nd_na(const struct flow *ip_flow, const
> struct match *md,
> reload_metadata(&ofpacts, md);
>
> enum ofperr error = ofpacts_pull_openflow_actions(userdata,
> userdata->size,
> - version, NULL,
> &ofpacts);
> + version, NULL, NULL,
> + &ofpacts);
> if (error) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_WARN_RL(&rl, "failed to parse actions for 'na' (%s)",
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index 1ba209dd4ce7..b9e9e21bfb3f 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -535,7 +535,81 @@ AT_CHECK([tail -2 stdout], [0],
> Datapath actions: 2
> ])
>
> -AT_CHECK([ovs-ofctl del-tlv-map br0])
> +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip],
> +[0], [dnl
> +NXST_FLOW reply:
> + tun_metadata3=0x1234567890abcdef actions=output:2
> +])
> +
> +dnl A TLV mapping should not be removed if any active flow uses the mapping.
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + CLEAR
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0], [0])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> +
> +dnl Flow modification
> +AT_CHECK([ovs-ofctl add-tlv-map br0
> "{class=0xffff,type=1,len=4}->tun_metadata0"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0
> "{class=0xffff,type=2,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0
> "{class=0xffff,type=3,len=4}->tun_metadata2"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1
> actions=multipath(eth_src,50,modulo_n,1,0,tun_metadata0[[0..31]])"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2
> actions=push:tun_metadata1[[0..31]],clone(move:tun_metadata2[[0..31]]->reg0[[0..31]])"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 actions=output:4"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=3,len=4}->tun_metadata0"])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2
> actions=push:tun_metadata2[[0..31]]"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=2,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=3,len=4}->tun_metadata2"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type length match field
> + ----- ---- ------ -----------
> + 0xffff 0x3 4 tun_metadata2
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0], [0])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
> +
> +dnl Learn action
> +AT_CHECK([ovs-ofctl add-tlv-map br0
> "{class=0xffff,type=1,len=4}->tun_metadata1"])
> +AT_CHECK([ovs-ofctl add-tlv-map br0
> "{class=0xffff,type=2,len=4}->tun_metadata2"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:01
> actions=learn(tun_metadata1[[0..31]]=reg1, output:NXM_OF_IN_PORT[[]])"])
> +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2, eth_src=00:00:00:00:00:02
> actions=learn(reg1[[0..31]]=0xFF,
> load:reg1[[0..31]]->tun_metadata2[[0..31]])"])
> +flow1="in_port(2),eth(src=00:00:00:00:00:01,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0800)"
> +flow2="in_port(2),eth(src=00:00:00:00:00:02,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0800)"
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow1" -generate], [0],
> [stdout])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow2" -generate], [0],
> [stdout])
> +
> +dnl Delete flows with learn action
> +AT_CHECK([ovs-ofctl del-flows br0 "in_port=2"])
> +
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=1,len=4}->tun_metadata1"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type length match field
> + ----- ---- ------ -----------
> + 0xffff 0x1 4 tun_metadata1
> +])
> +AT_CHECK([ovs-ofctl del-flows br0 "tun_metadata1"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=1,len=4}->tun_metadata1"])
> +
> +AT_CHECK([ovs-ofctl del-tlv-map br0
> "{class=0xffff,type=2,len=4}->tun_metadata2"], [1], [], [dnl
> +OFPT_ERROR (xid=0x4): NXTTMFC_INVALID_TLV_DEL
> +NXT_TLV_TABLE_MOD (xid=0x4):
> + DEL mapping table:
> + class type length match field
> + ----- ---- ------ -----------
> + 0xffff 0x2 4 tun_metadata2
> +])
> +AT_CHECK([ovs-ofctl del-flows br0 "reg1=0xFF"])
> +AT_CHECK([ovs-ofctl del-tlv-map br0], [0])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 7b214ebbaacd..6f684d28c9d1 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3876,7 +3876,7 @@ ofctl_parse_actions__(const char *version_s, bool
> instructions)
> error = (instructions
> ? ofpacts_pull_openflow_instructions
> : ofpacts_pull_openflow_actions)(
> - &of_in, of_in.size, version, NULL, &ofpacts);
> + &of_in, of_in.size, version, NULL, NULL, &ofpacts);
> if (!error && instructions) {
> /* Verify actions, enforce consistency. */
> enum ofputil_protocol protocol;
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
