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

[Marcelo Ricardo Leitner <mleitner@redhat.com>]

On Fri, Jan 25, 2019 at 02:37:13PM +0100, Simon Horman wrote:
> 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@redhat.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. 

Right. ct_state must be handled differently. For conntrack it is a
linear enum and as we want to be able to OR match, we will have to
convert the states in a bitfield as you were saying or so.

I don't think the representation above wouldn't change, though: we have
8 bits wrapped under a u8. What would change is how we deal with it.

If iproute tc is able to parse the cmdline and set a corresponding bit
for each state, the flower-side of flow dissector here should be
mostly fine (need to consider the invalid bits as you mentioned, as
part of sanity checking).
Then just need to change on how flow dissector is reading ct_state
from the packet.


Is your comment only related to ct_state or other fields too? I'm
thinking only ct_state.

> 
> > +	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,
> >  };
> 
> ...
>