netdev - Re: [PATCH net-next v3 1/2] cxgb4: add TC-MATCHALL classifier egress offload

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: <20191115105112.17c14b2b@cakuba.netronome.com>
Date:   Fri, 15 Nov 2019 10:51:12 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Rahul Lakkireddy <rahul.lakkireddy@...lsio.com>,
        netdev@...r.kernel.org, davem@...emloft.net, nirranjan@...lsio.com,
        vishal@...lsio.com, dt@...lsio.com
Subject: Re: [PATCH net-next v3 1/2] cxgb4: add TC-MATCHALL classifier
 egress offload

On Fri, 15 Nov 2019 16:32:47 +0100, Jiri Pirko wrote:
> Fri, Nov 15, 2019 at 04:08:30PM CET, rahul.lakkireddy@...lsio.com wrote:
> >On Friday, November 11/15/19, 2019 at 14:58:45 +0100, Jiri Pirko wrote:  
> >> Fri, Nov 15, 2019 at 01:14:20PM CET, rahul.lakkireddy@...lsio.com wrote:
> >> >+static int cxgb4_matchall_egress_validate(struct net_device *dev,
> >> >+					  struct tc_cls_matchall_offload *cls)
> >> >+{
> >> >+	struct netlink_ext_ack *extack = cls->common.extack;
> >> >+	struct flow_action *actions = &cls->rule->action;
> >> >+	struct port_info *pi = netdev2pinfo(dev);
> >> >+	struct flow_action_entry *entry;
> >> >+	u64 max_link_rate;
> >> >+	u32 i, speed;
> >> >+	int ret;
> >> >+
> >> >+	if (cls->common.prio != 1) {
> >> >+		NL_SET_ERR_MSG_MOD(extack,
> >> >+				   "Egress MATCHALL offload must have prio 1");  
> >> 
> >> I don't understand why you need it to be prio 1.  
> >
> >This is to maintain rule ordering with the kernel. Jakub has suggested
> >this in my earlier series [1][2]. I see similar checks in various
> >drivers (mlx5 and nfp), while offloading matchall with policer.  
> 
> I don't think that is correct. If matchall is the only filter there, it
> does not matter which prio is it. It matters only in case there are
> other filters.

Yup, the ingress side is the one that matters.

> The code should just check for other filters and forbid to insert the
> rule if other filters have higher prio (lower number).

Ack as well, that'd work even better. 

I've capitulated to the prio == 1 condition as "good enough" when
netronome was adding the policer offload for OvS.