netdev - Re: [RFC PATCH 2/6] net/sched: flower: add support for matching on ConnTrack

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: <20190125133711.f3caew4d7osr5czg@netronome.com>
Date:   Fri, 25 Jan 2019 14:37:13 +0100
From:   Simon Horman <simon.horman@...ronome.com>
To:     Marcelo Ricardo Leitner <mleitner@...hat.com>
Cc:     Guy Shattah <sguy@...lanox.com>, Aaron Conole <aconole@...hat.com>,
        John Hurley <john.hurley@...ronome.com>,
        Justin Pettit <jpettit@....org>,
        Gregory Rose <gvrose8192@...il.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Flavio Leitner <fbl@...hat.com>,
        Florian Westphal <fwestpha@...hat.com>,
        Jiri Pirko <jiri@...nulli.us>, Rashid Khan <rkhan@...hat.com>,
        Sushil Kulkarni <sukulkar@...hat.com>,
        Andy Gospodarek <andrew.gospodarek@...adcom.com>,
        Roi Dayan <roid@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Rony Efraim <ronye@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/6] net/sched: flower: add support for matching on
 ConnTrack

Hi Marcelo,

On Fri, Jan 25, 2019 at 12:32:31AM -0200, Marcelo Ricardo Leitner wrote:
> Hook on flow dissector's new interface on ConnTrack from previous patch.
> 
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@...hat.com>
> ---
>  include/uapi/linux/pkt_cls.h |  9 +++++++++
>  net/sched/cls_flower.c       | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 95d0db2a8350dffb1dd20816591f3b179913fb2e..ba1f3bc01b2fdfd810e37a2b3853a1da1f838acf 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -490,6 +490,15 @@ enum {
>  	TCA_FLOWER_KEY_PORT_DST_MIN,	/* be16 */
>  	TCA_FLOWER_KEY_PORT_DST_MAX,	/* be16 */
>  
> +	TCA_FLOWER_KEY_CT_ZONE,		/* u16 */
> +	TCA_FLOWER_KEY_CT_ZONE_MASK,	/* u16 */
> +	TCA_FLOWER_KEY_CT_STATE,	/* u8 */
> +	TCA_FLOWER_KEY_CT_STATE_MASK,	/* u8 */

With the corresponding flow dissector patch this API is
exposing the contents of an instance of enum ip_conntrack_info
as an ABI for conntrack state.

I believe (after getting similar review for my geneve options macthing
patches for flower) that this exposes implementation details as an ABI
to a degree that is not desirable.

My suggested would be to define, say in the form of named bits,
an ABI, that describes the state information that is exposed.
These bits may not correspond directly to the implementation of
ip_conntrack_info.

I think there should also be some consideration of if a mask makes
sense for the state as, f.e. in the implementation of enum
ip_conntrack_info not all bit combinations are valid. 

> +	TCA_FLOWER_KEY_CT_MARK,		/* u32 */
> +	TCA_FLOWER_KEY_CT_MARK_MASK,	/* u32 */
> +	TCA_FLOWER_KEY_CT_LABEL,	/* 128 bits */
> +	TCA_FLOWER_KEY_CT_LABEL_MASK,	/* 128 bits */
> +
>  	__TCA_FLOWER_MAX,
>  };

...