Skip to content

Commit

Permalink
ovn: Fix IPv6 DAD failure for child ports
Browse files Browse the repository at this point in the history 
When a child vlan interface is created inside a VM, the below kernel message
is seen and IPv6 doesn't work on that interface.

[  138.000753] IPv6: vlan4: IPv6 duplicate address <IPv6 LLA> detected!

When a child port sends a broadcast packet, OVN delivers the same
packet back to the child port (and hence the DAD check fails).
This is because 'MLF_ALLOW_LOOPBACK_BIT' is set in REG10 in table 0 when
the packet is received from any child port and table 'OFTABLE_CHECK_LOOPBACK'
doesn't drop the packet.

This patch fixes the issue by using a new register bit (MLF_NESTED_CONTAINER_BIT)
instead of 'MLF_ALLOW_LOOPBACK_BIT' and sets it in REG10 for the packets received
from child ports.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Gurucharan Shetty <guru@ovn.org>
  • Loading branch information
@numansiddique @shettyg
numansiddique authored and shettyg committed Oct 11, 2018
1 parent d4395b8 commit eda56ed
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 15 deletions.
29 changes: 20 additions & 9 deletions ovn/controller/ofctrl.c
View file
Expand Up @@ -630,9 +630,11 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
*
* The caller should initialize its own hmap to hold the flows. */
void
ofctrl_add_flow(struct hmap *desired_flows,
uint8_t table_id, uint16_t priority, uint64_t cookie,
const struct match *match, const struct ofpbuf *actions)
ofctrl_check_and_add_flow(struct hmap *desired_flows,
uint8_t table_id, uint16_t priority, uint64_t cookie,
const struct match *match,
const struct ofpbuf *actions,
bool log_duplicate_flow)
{
struct ovn_flow *f = xmalloc(sizeof *f);
f->table_id = table_id;
Expand All @@ -644,20 +646,29 @@ ofctrl_add_flow(struct hmap *desired_flows,
f->cookie = cookie;

if (ovn_flow_lookup(desired_flows, f)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
if (!VLOG_DROP_DBG(&rl)) {
char *s = ovn_flow_to_string(f);
VLOG_DBG("dropping duplicate flow: %s", s);
free(s);
if (log_duplicate_flow) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
if (!VLOG_DROP_DBG(&rl)) {
char *s = ovn_flow_to_string(f);
VLOG_DBG("dropping duplicate flow: %s", s);
free(s);
}
}

ovn_flow_destroy(f);
return;
}

hmap_insert(desired_flows, &f->hmap_node, f->hmap_node.hash);
}

void
ofctrl_add_flow(struct hmap *desired_flows,
uint8_t table_id, uint16_t priority, uint64_t cookie,
const struct match *match, const struct ofpbuf *actions)
{
ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
match, actions, true);
}

/* ovn_flow. */

Expand Down
5 changes: 5 additions & 0 deletions ovn/controller/ofctrl.h
View file
Expand Up @@ -55,4 +55,9 @@ void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id,
uint16_t priority, uint64_t cookie,
const struct match *, const struct ofpbuf *ofpacts);

void ofctrl_check_and_add_flow(struct hmap *desired_flows, uint8_t table_id,
uint16_t priority, uint64_t cookie,
const struct match *,
const struct ofpbuf *ofpacts,
bool log_duplicate_flow);
#endif /* ovn/ofctrl.h */
38 changes: 32 additions & 6 deletions ovn/controller/physical.c
View file
Expand Up @@ -189,7 +189,8 @@ get_zone_ids(const struct sbrec_port_binding *binding,

static void
put_local_common_flows(uint32_t dp_key, uint32_t port_key,
bool nested_container, const struct zone_ids *zone_ids,
uint32_t parent_port_key,
const struct zone_ids *zone_ids,
struct ofpbuf *ofpacts_p, struct hmap *flow_table)
{
struct match match;
Expand Down Expand Up @@ -253,7 +254,7 @@ put_local_common_flows(uint32_t dp_key, uint32_t port_key,
ofpbuf_clear(ofpacts_p);
match_set_metadata(&match, htonll(dp_key));
match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
if (!nested_container) {
if (!parent_port_key) {
match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
MLF_ALLOW_LOOPBACK, MLF_ALLOW_LOOPBACK);
}
Expand All @@ -264,6 +265,25 @@ put_local_common_flows(uint32_t dp_key, uint32_t port_key,
put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, 0,
&match, ofpacts_p);

if (parent_port_key) {
match_init_catchall(&match);
ofpbuf_clear(ofpacts_p);
match_set_metadata(&match, htonll(dp_key));
match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, parent_port_key);
match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
MLF_NESTED_CONTAINER, MLF_NESTED_CONTAINER);

put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p);
put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
/* When a parent port has multiple child ports, we don't want to
* log the duplicate flow message.
*/
ofctrl_check_and_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, 0,
&match, ofpacts_p, false);
}
}

static void
Expand Down Expand Up @@ -328,7 +348,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,
}

struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
put_local_common_flows(dp_key, port_key, false, &binding_zones,
put_local_common_flows(dp_key, port_key, 0, &binding_zones,
ofpacts_p, flow_table);

match_init_catchall(&match);
Expand Down Expand Up @@ -452,6 +472,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,

int tag = 0;
bool nested_container = false;
const struct sbrec_port_binding *parent_port = NULL;
ofp_port_t ofport;
bool is_remote = false;
if (binding->parent_port && *binding->parent_port) {
Expand All @@ -463,6 +484,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,
if (ofport) {
tag = *binding->tag;
nested_container = true;
parent_port = lport_lookup_by_name(
sbrec_port_binding_by_name, binding->parent_port);
}
} else {
ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
Expand Down Expand Up @@ -523,7 +546,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,
*/

struct zone_ids zone_ids = get_zone_ids(binding, ct_zones);
put_local_common_flows(dp_key, port_key, nested_container, &zone_ids,
uint32_t parent_port_key = parent_port ? parent_port->tunnel_key : 0;
put_local_common_flows(dp_key, port_key, parent_port_key, &zone_ids,
ofpacts_p, flow_table);

/* Table 0, Priority 150 and 100.
Expand Down Expand Up @@ -553,8 +577,10 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,
if (nested_container) {
/* When a packet comes from a container sitting behind a
* parent_port, we should let it loopback to other containers
* or the parent_port itself. */
put_load(MLF_ALLOW_LOOPBACK, MFF_LOG_FLAGS, 0, 1, ofpacts_p);
* or the parent_port itself. Indicate this by setting the
* MLF_NESTED_CONTAINER_BIT in MFF_LOG_FLAGS.*/
put_load(1, MFF_LOG_FLAGS, MLF_NESTED_CONTAINER_BIT, 1,
ofpacts_p);
}
ofpact_put_STRIP_VLAN(ofpacts_p);
}
Expand Down
4 changes: 4 additions & 0 deletions ovn/lib/logical-fields.h
View file
Expand Up @@ -50,6 +50,7 @@ enum mff_log_flags_bits {
MLF_FORCE_SNAT_FOR_DNAT_BIT = 2,
MLF_FORCE_SNAT_FOR_LB_BIT = 3,
MLF_LOCAL_ONLY_BIT = 4,
MLF_NESTED_CONTAINER_BIT = 5,
};

/* MFF_LOG_FLAGS_REG flag assignments */
Expand All @@ -75,6 +76,9 @@ enum mff_log_flags {
* hypervisors should instead only be output to local targets
*/
MLF_LOCAL_ONLY = (1 << MLF_LOCAL_ONLY_BIT),

/* Indicate that a packet has received from a nested container. */
MLF_NESTED_CONTAINER = (1 << MLF_NESTED_CONTAINER_BIT),
};

#endif /* ovn/lib/logical-fields.h */
11 changes: 11 additions & 0 deletions tests/ovn.at
View file
Expand Up @@ -7044,6 +7044,17 @@ packet=${dst_mac}${src_mac}8100000208004500001c000000003f110100${src_ip}${dst_ip
echo $packet >> expected1
OVN_CHECK_PACKETS([hv1/vm1-tx.pcap], [expected1])

# Send broadcast packet from foo1. foo1 should not receive the same packet.
src_mac="f00000010205"
dst_mac="ffffffffffff"
src_ip=`ip_to_hex 192 168 1 2`
dst_ip=`ip_to_hex 255 255 255 255`
packet=${dst_mac}${src_mac}8100000108004500001c0000000040110000${src_ip}${dst_ip}0035111100080000
as hv1 ovs-appctl netdev-dummy/receive vm1 $packet

# expected packet at VM1
OVN_CHECK_PACKETS([hv1/vm1-tx.pcap], [expected1])

OVN_CLEANUP([hv1],[hv2])

AT_CLEANUP
Expand Down

0 comments on commit eda56ed

Please sign in to comment.