On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote: > Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.hor...@netronome.com wrote: > >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote: > >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.hor...@netronome.com wrote: > >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote: > >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.hor...@netronome.com wrote: > > > >... > > > >> >> > enum flow_dissector_key_id { > >> >> > FLOW_DISSECTOR_KEY_CONTROL, /* struct > >> >> > flow_dissector_key_control */ > >> >> > FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */ > >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id { > >> >> > FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */ > >> >> > FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */ > >> >> > FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */ > >> >> >+ FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct > >> >> >flow_dissector_key_enc_opts */ > >> >> > >> >> I don't see the actual dissection implementation. Where is it? > >> >> Did you test the patchset? > >> > > >> >Yes, I did test it. But it is also possible something went astray along > >> >the > >> >way and I will retest. > >> > > >> >I think that the code you are looking for is in > >> >fl_classify() in this patch. > >> > >> The dissection should be done in the flow_dissector. That's the whole > >> point in having it generic. You should move it there. > > > >Coming back to this after lunch, I believe what I have done in this patch > >is consistent with handling of other enc fields, which are set in > >fl_classify() rather than the dissector. In particular the ip_tunnel_info, > >which is used by this patch, is already used in fl_classify(). > > That means the current code is wrong. The dissection should be done in > flow_dissector, not in fl_classify.
Would an better approach be to move the fl_classify() below into, say, skb_flow_dissect_tunnel_info() and call that from fl_classify(). The reason I suggest this rather than moving the code into __skb_flow_dissect() is that currently flower assumes that tunnel_info is used if present. While I assume other users of () assume tunnel_info is not used even if present. > >Without this patch I see: > > > > > >static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp, > > struct tcf_result *res) > >{ > > ... > > struct ip_tunnel_info *info; > > > > ... > > > > info = skb_tunnel_info(skb); > > if (info) { > > struct ip_tunnel_key *key = &info->key; > > > > switch (ip_tunnel_info_af(info)) { > > case AF_INET: > > skb_key.enc_control.addr_type = > > FLOW_DISSECTOR_KEY_IPV4_ADDRS; > > skb_key.enc_ipv4.src = key->u.ipv4.src; > > skb_key.enc_ipv4.dst = key->u.ipv4.dst; > > break; > > case AF_INET6: > > skb_key.enc_control.addr_type = > > FLOW_DISSECTOR_KEY_IPV6_ADDRS; > > skb_key.enc_ipv6.src = key->u.ipv6.src; > > skb_key.enc_ipv6.dst = key->u.ipv6.dst; > > break; > > } > > > > skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id); > > skb_key.enc_tp.src = key->tp_src; > > skb_key.enc_tp.dst = key->tp_dst; > > } > > > > ... > >} > > > >This patch adds the following inside the if() clause above: > > > > if (info->options_len) { > > skb_key.enc_opts.len = info->options_len; > > ip_tunnel_info_opts_get(skb_key.enc_opts.data, info); > > } > > > >