linux-kernel - Re: [ovs-dev] [PATCH] openvswitch: reduce padding in struct sw_flow

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: <CAEh+42gush2ouc286W=O_76ZNLO-BYSW_uiaPrbkJz22bJRt_g@mail.gmail.com>
Date:	Fri, 18 Mar 2016 15:22:50 -0700
From:	Jesse Gross <jesse@...nel.org>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Pravin Shelar <pshelar@...ira.com>,
	"David S. Miller" <davem@...emloft.net>,
	ovs dev <dev@...nvswitch.org>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Jiri Benc <jbenc@...hat.com>, linux-kernel@...r.kernel.org,
	Joe Stringer <joestringer@...ira.com>,
	Justin Pettit <jpettit@...ira.com>
Subject: Re: [ovs-dev] [PATCH] openvswitch: reduce padding in struct sw_flow_key

On Fri, Mar 18, 2016 at 6:34 AM, Arnd Bergmann <arnd@...db.de> wrote:
> This means it's still too large really, we just don't warn about it any more,
> and will get the warning again once another member is added. My patch is a
> band-aid at best, but more work is needed here. One problem is that
> ovs_flow_cmd_new and ovs_flow_cmd_set have two copies of struct sw_flow_key on
> the stack, one of them nested within sw_flow_mask. If we could reduce that to
> a single instance, the stack usage would be cut in half here.

I think it should be possible to reduce those two functions to only
use a single instance of the structure.

For new flows, we're already allocating the key structure on the heap,
it seems like we could just translate the key into that rather than to
intermediate location on the stack. And when setting flows, I'm not
sure that we really need the mask at all - we don't do anything with
it other than check it against the actions but we really should be
using the original mask for that rather than the new one.

It seems like it would be preferable to go down that route rather than
this patch, since as you say, this patch is really only suppressing
the warning and doesn't have a significant impact on the actual stack
consumption. Plus, the ordering of the flow layout affects how much
data needs to be compared during packet lookup, so this change will
likely be bad for forwarding performance.