Skip to content

Commit

Permalink
ofproto-dpif-upcall: Mirror packets that are modified.
Browse files Browse the repository at this point in the history 
Currently OVS keeps track of which mirrors that each packet has been
sent to for the purpose of deduplication. However, this doesn't consider
that openflow rules can make significant changes to packets after
ingress.

For example, OVN can create OpenFlow rules that turn an echo request
into an echo response by flipping source/destination addresses and
setting the ICMP type to Reply. When a mirror is configured, only the
request gets mirrored even though a response is received.

This can cause a false impression of the actual traffic on wire if
someone inspects the mirror and doesn't see an echo reply even though
one has been sent.

This patch resets the mirrors every time a packet is modified, so
mirrors will receive every copy of a packet that is sent for output.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
Acked-by: Eelco Chaudron <[email protected]>
Acked-by: Aaron Conole <[email protected]>
Signed-off-by: Mike Pattrick <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
@mkp-rh @igsilya
mkp-rh authored and igsilya committed Jul 21, 2023
1 parent 5d97653 commit a1ca9e5
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 3 deletions.
103 changes: 103 additions & 0 deletions ofproto/ofproto-dpif-xlate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6960,6 +6960,107 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
"cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
}

/* Reset the mirror context if we modify the packet and would like to mirror
* the new copy. */
static void
reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow,
const struct ofpact *a)
{
switch (a->type) {
case OFPACT_STRIP_VLAN:
case OFPACT_PUSH_VLAN:
case OFPACT_SET_ETH_SRC:
case OFPACT_SET_ETH_DST:
case OFPACT_PUSH_MPLS:
case OFPACT_POP_MPLS:
case OFPACT_SET_MPLS_LABEL:
case OFPACT_SET_MPLS_TC:
case OFPACT_SET_MPLS_TTL:
case OFPACT_DEC_MPLS_TTL:
case OFPACT_DEC_NSH_TTL:
case OFPACT_DEC_TTL:
case OFPACT_SET_VLAN_VID:
case OFPACT_SET_VLAN_PCP:
case OFPACT_ENCAP:
case OFPACT_DECAP:
case OFPACT_NAT:
ctx->mirrors = 0;
return;

case OFPACT_SET_FIELD: {
const struct ofpact_set_field *set_field;
const struct mf_field *mf;

set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
if (mf_are_prereqs_ok(mf, flow, NULL)) {
ctx->mirrors = 0;
}
return;
}

case OFPACT_SET_IPV4_SRC:
case OFPACT_SET_IPV4_DST:
if (flow->dl_type == htons(ETH_TYPE_IP)) {
ctx->mirrors = 0;
}
return;

case OFPACT_SET_IP_DSCP:
case OFPACT_SET_IP_ECN:
case OFPACT_SET_IP_TTL:
if (is_ip_any(flow)) {
ctx->mirrors = 0;
}
return;

case OFPACT_SET_L4_SRC_PORT:
case OFPACT_SET_L4_DST_PORT:
if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
ctx->mirrors = 0;
}
return;

case OFPACT_OUTPUT_REG:
case OFPACT_OUTPUT_TRUNC:
case OFPACT_GROUP:
case OFPACT_OUTPUT:
case OFPACT_CONTROLLER:
case OFPACT_RESUBMIT:
case OFPACT_GOTO_TABLE:
case OFPACT_WRITE_METADATA:
case OFPACT_SET_TUNNEL:
case OFPACT_REG_MOVE:
case OFPACT_STACK_PUSH:
case OFPACT_STACK_POP:
case OFPACT_LEARN:
case OFPACT_ENQUEUE:
case OFPACT_SET_QUEUE:
case OFPACT_POP_QUEUE:
case OFPACT_MULTIPATH:
case OFPACT_BUNDLE:
case OFPACT_EXIT:
case OFPACT_UNROLL_XLATE:
case OFPACT_FIN_TIMEOUT:
case OFPACT_CLEAR_ACTIONS:
case OFPACT_WRITE_ACTIONS:
case OFPACT_METER:
case OFPACT_SAMPLE:
case OFPACT_CLONE:
case OFPACT_DEBUG_RECIRC:
case OFPACT_DEBUG_SLOW:
case OFPACT_CT:
case OFPACT_CT_CLEAR:
case OFPACT_CHECK_PKT_LARGER:
case OFPACT_DELETE_FIELD:
case OFPACT_NOTE:
case OFPACT_CONJUNCTION:
return;
}

OVS_NOT_REACHED();
}

static void
do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
struct xlate_ctx *ctx, bool is_last_action,
Expand Down Expand Up @@ -7001,6 +7102,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
break;
}

reset_mirror_ctx(ctx, flow, a);

if (OVS_UNLIKELY(ctx->xin->trace)) {
struct ds s = DS_EMPTY_INITIALIZER;
struct ofpact_format_params fp = { .s = &s };
Expand Down
6 changes: 3 additions & 3 deletions tests/ofproto-dpif.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5220,7 +5220,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
AT_CHECK_UNQUOTED([tail -1 stdout], [0],
[Datapath actions: 3,push_vlan(vid=17,pcp=0),2
[Datapath actions: 3,push_vlan(vid=17,pcp=0),2,3
])

flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
Expand Down Expand Up @@ -5259,7 +5259,7 @@ flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x080
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
actual=`tail -1 stdout | sed 's/Datapath actions: //'`

expected="push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),1,2,100"
expected="push_vlan(vid=12,pcp=0),100,2,1,pop_vlan,push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),100,2,1"
AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
mv stdout expout
AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
Expand Down Expand Up @@ -5527,7 +5527,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
AT_CHECK_UNQUOTED([tail -1 stdout], [0],
[Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2
[Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2,trunc(100),3
])

flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
Expand Down

0 comments on commit a1ca9e5

Please sign in to comment.