netdev - Re: [PATCH net-next v7] openvswitch: enable NSH support

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:   Tue, 5 Sep 2017 11:46:45 +0200
From:   Jiri Benc <jbenc@...hat.com>
To:     "Yang, Yi" <yi.y.yang@...el.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "e@...g.me" <e@...g.me>, "blp@....org" <blp@....org>,
        "jan.scheurich@...csson.com" <jan.scheurich@...csson.com>
Subject: Re: [PATCH net-next v7] openvswitch: enable NSH support

On Tue, 5 Sep 2017 13:51:45 +0800, Yang, Yi wrote:
> But if we follow your way, how does nla_for_each_nested handle such
> pattern?
> 
> attribute1
> attribute1_mask
> attribute2
> attribute2_mask
> attribute3
> attribute3_mask

Uh? That will just work. Note that nla len contains the size of the
whole attribute, i.e. includes both fields.

> I don't think this can increase performance and readability.

Do you realize you're stating that not copying data around in hot path
can't increase performance? ;-)

> if we use one function to handle both attributes and masks, I can't
> see any substantial difference between two ways as far as the
> performance is concerned.

Except that what you did is so unexpected to netlink that you had to go
out of your way to parse it. Those two memcpys speak for themselves.

> If we consider OVS_KEY_ATTR_NSH as a big attribute, current way is
> precisely following current design pattern

Do you have an idea how nested netlink attributes work? It's very well
defined. What you're doing is breaking the definition. You can't do
that.

The (non-nested) attributes contain header followed by data. The data
is a single value or it is a struct. In ovs for masked actions,
attributes contain a struct of two fields (value and mask). The struct
access is open coded but it's still a struct.

Now, nested attributes are very well defined: it's a nla header
followed by a stream of attributes. There's no requirement on the order
of the attributes. Do you see how you're breaking this assumption? You
expect that each attribute is present exactly twice, once among first
half of attributes, once among second half of attributes. You don't
check that this weird assumption is adhered to. You don't even check
that the point where you split the data is between attributes! You may
very well be splitting an attribute in half and interpreting its data
as nla headers.

Consider your proposal NACKed from my side.

 Jiri