netdev - Re: [PATCH net] net: sched: cls: Fix offloading when ingress dev is vxlan

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]

Date:   Wed, 6 Jun 2018 10:59:30 +0300
From:   Paul Blakey <paulb@...lanox.com>
To:     Jakub Kicinski <kubakici@...pl>, David Miller <davem@...emloft.net>
Cc:     paulb@...lanox.com, jiri@...lanox.com, xiyou.wangcong@...il.com,
        jhs@...atatu.com, netdev@...r.kernel.org, kliteyn@...lanox.com,
        roid@...lanox.com, shahark@...lanox.com, markb@...lanox.com,
        ogerlitz@...lanox.com
Subject: Re: [PATCH net] net: sched: cls: Fix offloading when ingress dev is
 vxlan



On 06/06/2018 00:27, Jakub Kicinski wrote:
> On Tue, 05 Jun 2018 15:06:40 -0400 (EDT), David Miller wrote:
>> From: Jakub Kicinski <kubakici@...pl>
>> Date: Tue, 5 Jun 2018 11:57:47 -0700
>>
>>> Do we still care about correctness and not breaking backward
>>> compatibility?
>>
>> Jakub let me know if you want me to revert this change.
> 
> Yes, I think this patch introduces a regression when block is shared
> between offload capable and in-capable device, therefore it should be
> reverted.  Shared blocks went through a number of review cycles to
> ensure such cases are handled correctly.
> 
> 
> Longer story about the actual issue which is never explained in the
> commit message is this: in kernels 4.10 - 4.14 skip_sw flag was
> supported on tunnels in cls_flower only:
> 
> static int fl_hw_replace_filter(struct tcf_proto *tp,
> [...]
> 	if (!tc_can_offload(dev)) {
> 		if (tcf_exts_get_dev(dev, &f->exts, &f->hw_dev) ||
> 		    (f->hw_dev && !tc_can_offload(f->hw_dev))) {
> 			f->hw_dev = dev;
> 			return tc_skip_sw(f->flags) ? -EINVAL : 0;
> 		}
> 		dev = f->hw_dev;
> 		cls_flower.egress_dev = true;
> 	} else {
> 		f->hw_dev = dev;
> 	}
> 
> 
> In 4.15 - 4.17 with addition of shared blocks egdev mechanism was
> promoted to a generic TC thing supported on all filters but it no
> longer works with skip_sw.
> 
> I'd argue skip_sw is incorrect for tunnels, because the rule will only
> apply to traffic ingressing on ASIC ports, not all traffic which hits
> the tunnel netdev.  Therefore we should keep the 4.15 - 4.17 behaviour.
> 
> But that's a side note, I don't think we should be breaking offload on
> shared blocks whether we decide to support skip_sw on tunnels or not.
> 

Maybe we can apply my patch logic of still trying the egress dev if the 
block has a single device, and not shared. Is that ok with you?

You're patch seems good as an add on, but the egress hw device (sw1np0) 
would go over the tc actions and see if it can offload such rule (output 
to software lo device) and would fail anyway.