[ovs-dev] [PATCH v3 ovn] Enforce datapath and port key constraints in vxlan mode
Numan Siddique
numans at ovn.org
Mon Oct 4 19:36:05 UTC 2021
On Thu, Sep 30, 2021 at 6:05 PM Ihar Hrachyshka <ihrachys at redhat.com> wrote:
>
> With vxlan enabled for in-cluster communication, the ranges available
> for port and datapath keys are limited to 12 bits (including multigroup
> keys). (The default range is 16 bit long.)
>
> This means that OVN should avoid allocating, or allowing to request,
> tunnel keys for datapaths and ports that are equal or higher than
> 2 << 11. This was not enforced before, and this patch adds the missing
> enforcement rules.
>
> Fixes: b07f1bc3d068 ("Add VXLAN support for non-VTEP datapath bindings")
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
Thanks for the fix. I applied this patch to the main branch and branch-21.09.
It doesn't apply cleanly to other branches. Please submit branch
specific backport
patches if you think it is required.
Numan
>
> --
>
> v1: initial commit
> v2: fix build (added missing OVN_VXLAN_MIN_MULTICAST macro)
> v3: rebase
> ---
> lib/mcast-group-index.h | 2 ++
> northd/northd.c | 53 ++++++++++++++++++++++++++++-------------
> northd/ovn_northd.dl | 31 +++++++++++++++++-------
> tests/ovn.at | 49 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 110 insertions(+), 25 deletions(-)
>
> diff --git a/lib/mcast-group-index.h b/lib/mcast-group-index.h
> index 72af117a4..5bc725451 100644
> --- a/lib/mcast-group-index.h
> +++ b/lib/mcast-group-index.h
> @@ -23,6 +23,8 @@ struct sbrec_datapath_binding;
> #define OVN_MIN_MULTICAST 32768
> #define OVN_MAX_MULTICAST 65535
>
> +#define OVN_VXLAN_MIN_MULTICAST 2048
> +
> enum ovn_mcast_tunnel_keys {
>
> OVN_MCAST_FLOOD_TUNNEL_KEY = OVN_MIN_MULTICAST,
> diff --git a/northd/northd.c b/northd/northd.c
> index cf2467fe1..21b34a70b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1305,7 +1305,8 @@ ovn_datapath_allocate_key(struct northd_context *ctx,
> }
>
> static void
> -ovn_datapath_assign_requested_tnl_id(struct hmap *dp_tnlids,
> +ovn_datapath_assign_requested_tnl_id(struct northd_context *ctx,
> + struct hmap *dp_tnlids,
> struct ovn_datapath *od)
> {
> const struct smap *other_config = (od->nbs
> @@ -1313,6 +1314,13 @@ ovn_datapath_assign_requested_tnl_id(struct hmap *dp_tnlids,
> : &od->nbr->options);
> uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0);
> if (tunnel_key) {
> + if (is_vxlan_mode(ctx->ovnsb_idl) && tunnel_key >= 1 << 12) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> + VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is "
> + "incompatible with VXLAN", tunnel_key,
> + od->nbs ? od->nbs->name : od->nbr->name);
> + return;
> + }
> if (ovn_add_tnlid(dp_tnlids, tunnel_key)) {
> od->tunnel_key = tunnel_key;
> } else {
> @@ -1342,10 +1350,10 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths,
> struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids);
> struct ovn_datapath *od, *next;
> LIST_FOR_EACH (od, list, &both) {
> - ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od);
> + ovn_datapath_assign_requested_tnl_id(ctx, &dp_tnlids, od);
> }
> LIST_FOR_EACH (od, list, &nb_only) {
> - ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od);
> + ovn_datapath_assign_requested_tnl_id(ctx, &dp_tnlids, od);
> }
>
> /* Keep nonconflicting tunnel IDs that are already assigned. */
> @@ -3750,27 +3758,40 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key)
> }
>
> static void
> -ovn_port_assign_requested_tnl_id(struct ovn_port *op)
> +ovn_port_assign_requested_tnl_id(struct northd_context *ctx,
> + struct ovn_port *op)
> {
> const struct smap *options = (op->nbsp
> ? &op->nbsp->options
> : &op->nbrp->options);
> uint32_t tunnel_key = smap_get_int(options, "requested-tnl-key", 0);
> - if (tunnel_key && !ovn_port_add_tnlid(op, tunnel_key)) {
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> - VLOG_WARN_RL(&rl, "Logical %s port %s requests same tunnel key "
> - "%"PRIu32" as another LSP or LRP",
> - op->nbsp ? "switch" : "router",
> - op_get_name(op), tunnel_key);
> + if (tunnel_key) {
> + if (is_vxlan_mode(ctx->ovnsb_idl) &&
> + tunnel_key >= OVN_VXLAN_MIN_MULTICAST) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> + VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s "
> + "is incompatible with VXLAN",
> + tunnel_key, op_get_name(op));
> + return;
> + }
> + if (!ovn_port_add_tnlid(op, tunnel_key)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> + VLOG_WARN_RL(&rl, "Logical %s port %s requests same tunnel key "
> + "%"PRIu32" as another LSP or LRP",
> + op->nbsp ? "switch" : "router",
> + op_get_name(op), tunnel_key);
> + }
> }
> }
>
> static void
> -ovn_port_allocate_key(struct hmap *ports, struct ovn_port *op)
> +ovn_port_allocate_key(struct northd_context *ctx, struct hmap *ports,
> + struct ovn_port *op)
> {
> if (!op->tunnel_key) {
> + uint8_t key_bits = is_vxlan_mode(ctx->ovnsb_idl)? 12 : 16;
> op->tunnel_key = ovn_allocate_tnlid(&op->od->port_tnlids, "port",
> - 1, (1u << 15) - 1,
> + 1, (1u << (key_bits - 1)) - 1,
> &op->od->port_key_hint);
> if (!op->tunnel_key) {
> if (op->sb) {
> @@ -3810,10 +3831,10 @@ build_ports(struct northd_context *ctx,
> /* Assign explicitly requested tunnel ids first. */
> struct ovn_port *op, *next;
> LIST_FOR_EACH (op, list, &both) {
> - ovn_port_assign_requested_tnl_id(op);
> + ovn_port_assign_requested_tnl_id(ctx, op);
> }
> LIST_FOR_EACH (op, list, &nb_only) {
> - ovn_port_assign_requested_tnl_id(op);
> + ovn_port_assign_requested_tnl_id(ctx, op);
> }
>
> /* Keep nonconflicting tunnel IDs that are already assigned. */
> @@ -3825,10 +3846,10 @@ build_ports(struct northd_context *ctx,
>
> /* Assign new tunnel ids where needed. */
> LIST_FOR_EACH_SAFE (op, next, list, &both) {
> - ovn_port_allocate_key(ports, op);
> + ovn_port_allocate_key(ctx, ports, op);
> }
> LIST_FOR_EACH_SAFE (op, next, list, &nb_only) {
> - ovn_port_allocate_key(ports, op);
> + ovn_port_allocate_key(ctx, ports, op);
> }
>
> /* For logical ports that are in both databases, update the southbound
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 22913f05a..ed21c0da0 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -8363,10 +8363,18 @@ relation OvnMaxDpKeyLocal[integer]
> OvnMaxDpKeyLocal[oVN_MAX_DP_VXLAN_KEY()] :- IsVxlanMode[true].
> OvnMaxDpKeyLocal[oVN_MAX_DP_KEY() - oVN_MAX_DP_GLOBAL_NUM()] :- IsVxlanMode[false].
>
> -function get_dp_tunkey(map: Map<istring,istring>, key: istring): Option<integer> {
> +relation OvnPortKeyBits[bit<32>]
> +OvnPortKeyBits[12] :- IsVxlanMode[true].
> +OvnPortKeyBits[16] :- IsVxlanMode[false].
> +
> +relation OvnDpKeyBits[bit<32>]
> +OvnDpKeyBits[12] :- IsVxlanMode[true].
> +OvnDpKeyBits[24] :- IsVxlanMode[false].
> +
> +function get_dp_tunkey(map: Map<istring,istring>, key: istring, bits: bit<32>): Option<integer> {
> map.get(key)
> .and_then(parse_dec_u64)
> - .and_then(|x| if (x > 0 and x < (2<<24)) {
> + .and_then(|x| if (x > 0 and x < (1<<bits)) {
> Some{x}
> } else {
> None
> @@ -8376,11 +8384,13 @@ function get_dp_tunkey(map: Map<istring,istring>, key: istring): Option<integer>
> // Tunnel keys requested by datapaths.
> relation RequestedTunKey(datapath: uuid, tunkey: integer)
> RequestedTunKey(uuid, tunkey) :-
> + OvnDpKeyBits[bits],
> ls in &nb::Logical_Switch(._uuid = uuid),
> - Some{var tunkey} = get_dp_tunkey(ls.other_config, i"requested-tnl-key").
> + Some{var tunkey} = get_dp_tunkey(ls.other_config, i"requested-tnl-key", bits).
> RequestedTunKey(uuid, tunkey) :-
> + OvnDpKeyBits[bits],
> lr in nb::Logical_Router(._uuid = uuid),
> - Some{var tunkey} = get_dp_tunkey(lr.options, i"requested-tnl-key").
> + Some{var tunkey} = get_dp_tunkey(lr.options, i"requested-tnl-key", bits).
> Warning[message] :-
> RequestedTunKey(datapath, tunkey),
> var count = datapath.group_by((tunkey)).size(),
> @@ -8442,13 +8452,14 @@ TunKeyAllocation(datapath, tunkey) :-
> /*
> * Port id allocation:
> *
> - * Port IDs in a per-datapath space in the range 1...2**15-1
> + * Port IDs in a per-datapath space in the range 1...2**(bits-1)-1, where
> + * bits is the number of bits available for port keys (default: 16, vxlan: 12)
> */
>
> -function get_port_tunkey(map: Map<istring,istring>, key: istring): Option<integer> {
> +function get_port_tunkey(map: Map<istring,istring>, key: istring, bits: bit<32>): Option<integer> {
> map.get(key)
> .and_then(parse_dec_u64)
> - .and_then(|x| if (x > 0 and x < (2<<15)) {
> + .and_then(|x| if (x > 0 and x < (1<<bits)) {
> Some{x}
> } else {
> None
> @@ -8458,15 +8469,17 @@ function get_port_tunkey(map: Map<istring,istring>, key: istring): Option<intege
> // Tunnel keys requested by port bindings.
> relation RequestedPortTunKey(datapath: uuid, port: uuid, tunkey: integer)
> RequestedPortTunKey(datapath, port, tunkey) :-
> + OvnPortKeyBits[bits],
> sp in &SwitchPort(),
> var datapath = sp.sw._uuid,
> var port = sp.lsp._uuid,
> - Some{var tunkey} = get_port_tunkey(sp.lsp.options, i"requested-tnl-key").
> + Some{var tunkey} = get_port_tunkey(sp.lsp.options, i"requested-tnl-key", bits).
> RequestedPortTunKey(datapath, port, tunkey) :-
> + OvnPortKeyBits[bits],
> rp in &RouterPort(),
> var datapath = rp.router._uuid,
> var port = rp.lrp._uuid,
> - Some{var tunkey} = get_port_tunkey(rp.lrp.options, i"requested-tnl-key").
> + Some{var tunkey} = get_port_tunkey(rp.lrp.options, i"requested-tnl-key", bits).
> Warning[message] :-
> RequestedPortTunKey(datapath, port, tunkey),
> var count = port.group_by((datapath, tunkey)).size(),
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 172b5c713..d04a32d28 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -3690,6 +3690,55 @@ done
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([VXLAN check port/datapath key space limits])
> +
> +ovn_start
> +net_add net
> +check ovs-vsctl add-br br-phys
> +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +ovn_attach net br-phys 192.168.0.1 24 vxlan
> +check ovn-nbctl --wait=sb sync
> +
> +check ovn-nbctl ls-add ls-bad -- \
> + set Logical_Switch ls-bad other_config:requested-tnl-key=5000
> +check ovn-nbctl lsp-add ls-bad lsp-bad -- \
> + set logical_switch_port lsp-bad options:requested-tnl-key=5000
> +check ovn-nbctl --wait=sb sync
> +
> +check ovs-vsctl add-port br-int vif-bad -- \
> + set Interface vif-bad external-ids:iface-id=lsp-bad
> +OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp-bad` = xup])
> +
> +# 5000 is higher than 1 << 12, so OVN should ignore the key requests
> +AT_CHECK([ovn-sbctl get Datapath_Binding ls-bad tunnel_key], [0], [dnl
> +1
> +])
> +AT_CHECK([ovn-sbctl get Port_Binding lsp-bad tunnel_key], [0], [dnl
> +1
> +])
> +
> +check ovn-nbctl ls-add ls-good -- \
> + set Logical_Switch ls-good other_config:requested-tnl-key=1000
> +check ovn-nbctl lsp-add ls-good lsp-good -- \
> + set logical_switch_port lsp-good options:requested-tnl-key=1000
> +check ovn-nbctl --wait=sb sync
> +
> +check ovs-vsctl add-port br-int vif-good -- \
> + set Interface vif-good external-ids:iface-id=lsp-good
> +OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp-good` = xup])
> +
> +# 1000 is lower than 1 << 12, so OVN should honor the key requests
> +AT_CHECK([ovn-sbctl get Datapath_Binding ls-good tunnel_key], [0], [dnl
> +1000
> +])
> +AT_CHECK([ovn-sbctl get Port_Binding lsp-good tunnel_key], [0], [dnl
> +1000
> +])
> +
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([2 HVs, 1 LS, no switching between multiple localnet ports with different tags])
> ovn_start
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list