netdev - Re: [RFC PATCH net-next 1/6 v2] net/sched: Introduce act

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] [day] [month] [year] [list]

Message-ID: <20190204124804.thbjjwypnoyb2xbx@netronome.com>
Date:   Mon, 4 Feb 2019 13:48:05 +0100
From:   Simon Horman <simon.horman@...ronome.com>
To:     Paul Blakey <paulb@...lanox.com>
Cc:     Marcelo Leitner <mleitner@...hat.com>,
        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" <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 1/6 v2] net/sched: Introduce act_ct

[Repost without HTML; sorry about that]

On Sun, Feb 03, 2019 at 08:26:23AM +0000, Paul Blakey wrote:
> 
> 
> On 01/02/2019 15:23, Marcelo Leitner wrote:
> > On Tue, Jan 29, 2019 at 10:02:01AM +0200, Paul Blakey wrote:
> > ...
> >> diff --git a/include/uapi/linux/tc_act/tc_ct.h b/include/uapi/linux/tc_act/tc_ct.h
> >> new file mode 100644
> >> index 0000000..6dbd771
> >> --- /dev/null
> >> +++ b/include/uapi/linux/tc_act/tc_ct.h
> >> @@ -0,0 +1,29 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +#ifndef __UAPI_TC_CT_H
> >> +#define __UAPI_TC_CT_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/pkt_cls.h>
> >> +
> >> +#define TCA_ACT_CT 18
> >> +
> >> +struct tc_ct {
> >> +	tc_gen;
> >> +	__u16 zone;
> >> +	__u32 labels[4];
> >> +	__u32 labels_mask[4];
> >> +	__u32 mark;
> >> +	__u32 mark_mask;
> >> +	bool commit;
> > 
> > This is one of the points that our implementations differs. You used a
> > struct and wrapped it into TCA_CT_PARMS attribute, while I broke it up
> > into several attributes.
> > 
> > cls_flower and act_bpf, for example, doesn't use structs, but others
> > do.
> > 
> > Both have pros and cons and I imagine this topic probably was already
> > discussed but I'm not aware of a recommendation. Do we have one?
> 
> I guess flower uses a netlink attribute per key attribute because
> a lot of time, most of them won't be used, and you would send less.
> we can have ct, ct + snat, ct + dnat, zone and mark.... a lot of this
> won't be used sometimes.
> 
> Also you can't add nested attributes to the struct easily.
> 
> Also netlink attributes can be tested for existence, while a struct
> would need a special non valid value, or another field to specify which
> fields are used.
> 
> both are hard to test if a requested attribute was ignored, besides
> checking the netlink echo or dumping the action back. if for example a
> older kernel module and newer userspace uses a attribute above
> enum TCA_CT_MAX (struct attributes also don't have max len, in nla_parse).
> 
> 
> All in all, I think mostly netlink attributes would be better.

+1

I believe that Flower uses more attributes because its regarded as being
more flexible and that benefit outweighs the extra cost - f.e. the netlink
messages would tend to be a bit larger if a struct was used.

> 
> > 
> >> +};
> >> +
> >> +enum {
> >> +	TCA_CT_UNSPEC,
> >> +	TCA_CT_PARMS,
> >> +	TCA_CT_TM,
> >> +	TCA_CT_PAD,
> >> +	__TCA_CT_MAX
> >> +};
> >> +#define TCA_CT_MAX (__TCA_CT_MAX - 1)
> >> +
> >> +#endif /* __UAPI_TC_CT_H */
> > ...
> >