| Message ID | 20171025093552.14502-1-dalvarez@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] Add dl_type to flow metadata for correct interpretation of conntrack metadata | expand |
On Wed, Oct 25, 2017 at 11:35:52AM +0200, Daniel Alvarez wrote: > When a packet is sent to the controller, dl_type is not stored in the > 'ofputil_packet_in_private'. When the packet is resumed, the flow's > dl_type is 0 and this leads to invalid value in ct_orig_tuple in the > pkt_metadata. > > This patch adds the dl_type to the metadata so that conntrack > information can be interpreted correctly when packets are resumed. > > Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Thanks a lot for the patch. I believe it fixes an important bug. I might add another paragraph to the commit message to really drive home what's going, perhaps something like this: This is a change from the ordinary practice, since flow_get_metadata() is only supposed to deal with metadata and dl_type is not metadata. It is necessary when ct_state is involved, though, because ct_state only applies in the case of particular Ethertypes (IPv4 and IPv6 currently), so we need to add it as a kind of prerequisite. (This isn't ideal; maybe we didn't think through the ct_state mechanism carefully enough.) However, this breaks several tests (1211 1213 1215 1216 1217 1218 1219 1220 1221 1223 1226). Would you mind checking that this makes sense for those tests and updating them as necessary to pass? Thanks, Ben.
diff --git a/lib/flow.c b/lib/flow.c index b2b10aa48..4d2b7747a 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -1073,6 +1073,9 @@ flow_get_metadata(const struct flow *flow, struct match *flow_metadata) if (flow->ct_state != 0) { match_set_ct_state(flow_metadata, flow->ct_state); + /* Match dl_type since it is required for the later interpretation of + * the conntrack metadata. */ + match_set_dl_type(flow_metadata, flow->dl_type); if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0) { if (flow->dl_type == htons(ETH_TYPE_IP)) { match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);