diff mbox series

[ovs-dev] Add dl_type to flow metadata for correct interpretation of conntrack metadata

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

Commit Message

Daniel Alvarez Oct. 25, 2017, 9:35 a.m. UTC
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>
---
 lib/flow.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Ben Pfaff Oct. 25, 2017, 5:10 p.m. UTC | #1
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 mbox series

Patch

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);