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: <20170927140011.GJ1944@nanopsycho.orion>
Date:   Wed, 27 Sep 2017 16:00:11 +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, amir@...ai.me
Subject: Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel
 options

Wed, Sep 27, 2017 at 03:50:44PM CEST, simon.horman@...ronome.com wrote:
>On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@...ronome.com wrote:
>> >On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
>> >> 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.
>> >
>> >Would an better approach be to move the fl_classify() below into, say,
>> >skb_flow_dissect_tunnel_info() and call that from fl_classify().
>> 
>> No. There is one flow dissection function and you just set it up in a
>> way you need it. Makes no sense to me to split it up in any way.
>> 
>> 
>> >
>> >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.
>> 
>> __skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
>> only in case it is needed.
>
>Ok, do you think it is sufficient for __skb_flow_dissect to look at the
>tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
>break flower as it look at the tunnel info unconditionally.

yeah. When flower needs that, it will get that from the flow dissector.
I don't see why it would break anything. Again, existing code is wrong:
commit bc3103f1ed405de587fa43d8b0671e615505a700
Author: Amir Vadai <amir@...ai.me>
Date:   Thu Sep 8 16:23:47 2016 +0300

    net/sched: cls_flower: Classify packet in ip tunnels

The dissection has to be moved to flow dissector.