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]

Message-ID: <20180605142700.6033a7a0@cakuba.netronome.com>
Date:   Tue, 5 Jun 2018 14:27:00 -0700
From:   Jakub Kicinski <kubakici@...pl>
To:     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 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.