netdev - Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170927125603.GH1944@nanopsycho.orion>
Date:   Wed, 27 Sep 2017 14:56:03 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Simon Horman <simon.horman@...ronome.com>
Cc:     David Miller <davem@...emloft.net>, Jiri Pirko <jiri@...lanox.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel
 options

Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@...ronome.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.horman@...ronome.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.horman@...ronome.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.



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