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: <20191115190056.GA14695@chelsio.com>
Date:   Sat, 16 Nov 2019 00:30:57 +0530
From:   Rahul Lakkireddy <rahul.lakkireddy@...lsio.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, 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 Friday, November 11/15/19, 2019 at 10:51:12 -0800, Jakub Kicinski wrote:
> 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.

I see. I thought there was some sort of mutual agreement, that to
offload police, then prio must be 1, when I saw several drivers do
it. I don't have a police offload on ingress side yet. So, I'm
guessing this check for prio is not needed at all for my series?
Please confirm again so that I'm on the same page. :)

Thanks,
Rahul