netdev - Re: [ovs-dev] [PATCH] net: openvswitch: Support masked set actions.

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: <14D3CD27-3B5C-4CC4-9EE7-5C79A2AAD303@nicira.com>
Date:	Thu, 5 Feb 2015 10:20:56 -0800
From:	Jarno Rajahalme <jrajahalme@...ira.com>
To:	Jesse Gross <jesse@...ira.com>
Cc:	netdev <netdev@...r.kernel.org>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [ovs-dev] [PATCH] net: openvswitch: Support masked set actions.

Sorry for not remembering about these…

On Dec 10, 2014, at 6:03 PM, Jesse Gross <jesse@...ira.com> wrote:

> On Tue, Dec 9, 2014 at 4:10 PM, Jarno Rajahalme <jrajahalme@...ira.com> wrote:
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index df3c7f2..276bb60 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -2007,6 +2117,18 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
>>                                return err;
>>                        break;
>> 
>> +               case OVS_ACTION_ATTR_SET_MASKED:
>> +                       err = masked_set_action_to_attr(a, skb);
>> +                       if (err)
>> +                               return err;
>> +                       break;
> 
> I don't think this is necessary - the default case will handle things
> that don't need any special processing.
> 

Right, the masked_set_action_to_attr() function is actually the same as the default processing, so it can be removed. Thanks for pointing this out!

> I think you can also remove the port checks in validate_tp_port()
> since the reasoning behind them is the same as the IP proto check.
> 

The reasoning for removing the the IP proto check was that it may be wildcarded, and that we are checking for the presence of the IP header anyway when executing the check.

Same applies to the ports, in principle the flow could wildcard them and a set action could set one or both of them regardless (even though the current OVS userspace still exact matches all the fields it sets, but this may change in the future).

For transport protocol we validate the flow for the right IP proto field value, and in execution we use sib_make_writeable() to make sure the packet actually has the space for the header, so we can safely drop the flow key transport port checks from validate_tp_port(). All that remains there are the ethertype checks, which we need to keep as the ip.proto field is also used for the ARP opcode, which could collide with a valid IPPROTO_ value. However, it seems strange to have a function named validate_tp_ports() to only check that the ethertype is either IPv4 or IPv6, so I will inline the checks to validate_set().

> Otherwise, I'm generally happy with this though.

I’ll send a v3 later today.

Thanks,

  Jarno--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html