| Message ID | CFF8EF42F1132E4CBE2BF0AB6C21C58D6E541CD7@ESESSMB107.ericsson.se |
|---|---|
| State | Not Applicable |
| Headers | show |
Here are some tests of the below proposal: ovs-ofctl -Oopenflow13 add-flow br0 "in_port=1,ip,actions=decap(),set_field:1.1.1.1->nw_dst" ovs-ofctl -Oopenflow13 add-flow br0 "in_port=1,packet_type=(1,0x800),actions=decap(),set_field:1.1.1.1->nw_dst" 2017-07-13T18:49:34Z|00001|ofp_actions|WARN|set_field ip_dst lacks correct prerequisites ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT) /Jan > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Jan Scheurich > Sent: Thursday, 13 July, 2017 20:17 > To: Ben Pfaff <blp@ovn.org>; Zoltán Balogh <zoltan.balogh@ericsson.com> > Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> > Subject: Re: [ovs-dev] [PATCH 1/4] Add OF actions for generic encap and decap > > Hi Ben, > > > -----Original Message----- > > From: Ben Pfaff [mailto:blp@ovn.org] > > Sent: Tuesday, 11 July, 2017 23:46 > > > > What is your plan for handling ofpact_check__() with decap actions? > > > > In general a decap() action reveals an inner packet that requires reparsing before any subsequent actions can be applied. The only known > exception is decap() on an Ethernet packet, which just changes the packet type to (1, ether_type). So for an Ethernet packet we could just > change the flow's packet type accordingly. > > For all other packet types, we should not accept any subsequent actions in an action list after decap(). > We can't leave the packet_type unchanged, as that might allow false positives. Instead we should perhaps change the packet_type to an > "undefined" value: > > So here's my proposal: > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index de7c612..e0a6c7b 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -7814,7 +7814,7 @@ inconsistent_match(enum ofputil_protocol *usable_protocols) > *usable_protocols &= OFPUTIL_P_OF10_ANY; > } > > -/* May modify flow_packet_type, flow->dl_type, flow->nw_proto and > +/* May modify flow->packet_type, flow->dl_type, flow->nw_proto and > * flow->vlan_tci, caller must restore them. > * > * Modifies some actions, filling in fields that could not be properly set > @@ -8127,8 +8127,16 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, > return 0; > > case OFPACT_DECAP: > - /* FIXME: The resulting packet_type is not known at flow deployment > - * time. How can we allow actions with pre-requisites after decap? */ > + if (flow->packet_type == htonl(PT_ETH)) { > + /* Adjust the packet_type to allow subsequent actions. */ > + flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE, > + ntohs(flow->dl_type)); > + } else { > + /* The actual packet_type is only known after decapsulation. > + * Do not allow subsequent actions that depend on packet headers. */ > + flow->packet_type = htonl(PT_UNKNOWN); > + flow->dl_type = OVS_BE16_MAX; > + } > return 0; > > I haven't tested that yet, but I believe it should do the trick. > > Regards, Jan > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index de7c612..e0a6c7b 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7814,7 +7814,7 @@ inconsistent_match(enum ofputil_protocol *usable_protocols) *usable_protocols &= OFPUTIL_P_OF10_ANY; } -/* May modify flow_packet_type, flow->dl_type, flow->nw_proto and +/* May modify flow->packet_type, flow->dl_type, flow->nw_proto and * flow->vlan_tci, caller must restore them. * * Modifies some actions, filling in fields that could not be properly set @@ -8127,8 +8127,16 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, return 0; case OFPACT_DECAP: - /* FIXME: The resulting packet_type is not known at flow deployment - * time. How can we allow actions with pre-requisites after decap? */ + if (flow->packet_type == htonl(PT_ETH)) { + /* Adjust the packet_type to allow subsequent actions. */ + flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE, + ntohs(flow->dl_type)); + } else { + /* The actual packet_type is only known after decapsulation. + * Do not allow subsequent actions that depend on packet headers. */ + flow->packet_type = htonl(PT_UNKNOWN); + flow->dl_type = OVS_BE16_MAX; + } return 0; I haven't tested that yet, but I believe it should do the trick.