* [PATCH net] openvswitch: Fix mask generation for nested attributes.
@ 2015-09-11 1:36 Jesse Gross
2015-09-11 16:04 ` Pravin Shelar
0 siblings, 1 reply; 3+ messages in thread
From: Jesse Gross @ 2015-09-11 1:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Masks were added to OVS flows in a way that was backwards compatible
with userspace programs that did not generate masks. As a result, it is
possible that we may receive flows that do not have a mask and we need
to synthesize one.
Generating a mask requires iterating over attributes and descending into
nested attributes. For each level we need to know the size to generate the
correct mask. We do this with a linked table of attribute types.
Although the logic to handle these nested attributes was there in concept,
there are a number of bugs in practice. Examples include incomplete links
between tables, variable length attributes being treated as nested and
missing sanity checks.
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
net/openvswitch/flow_netlink.c | 71 +++++++++++++++++++++++++++++-------------
1 file changed, 50 insertions(+), 21 deletions(-)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c92d6a2..83a6847 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -57,6 +57,7 @@ struct ovs_len_tbl {
};
#define OVS_ATTR_NESTED -1
+#define OVS_ATTR_VARIABLE -2
static void update_range(struct sw_flow_match *match,
size_t offset, size_t size, bool is_mask)
@@ -304,6 +305,10 @@ size_t ovs_key_attr_size(void)
+ nla_total_size(28); /* OVS_KEY_ATTR_ND */
}
+static const struct ovs_len_tbl ovs_vxlan_ext_key_lens[OVS_VXLAN_EXT_MAX + 1] = {
+ [OVS_VXLAN_EXT_GBP] = { .len = sizeof(u32) },
+};
+
static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
[OVS_TUNNEL_KEY_ATTR_ID] = { .len = sizeof(u64) },
[OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = { .len = sizeof(u32) },
@@ -315,8 +320,9 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]
[OVS_TUNNEL_KEY_ATTR_TP_SRC] = { .len = sizeof(u16) },
[OVS_TUNNEL_KEY_ATTR_TP_DST] = { .len = sizeof(u16) },
[OVS_TUNNEL_KEY_ATTR_OAM] = { .len = 0 },
- [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = { .len = OVS_ATTR_NESTED },
- [OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS] = { .len = OVS_ATTR_NESTED },
+ [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = { .len = OVS_ATTR_VARIABLE },
+ [OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS] = { .len = OVS_ATTR_NESTED,
+ .next = ovs_vxlan_ext_key_lens },
};
/* The size of the argument for each %OVS_KEY_ATTR_* Netlink attribute. */
@@ -388,7 +394,9 @@ static int __parse_flow_nlattrs(const struct nlattr *attr,
}
expected_len = ovs_key_lens[type].len;
- if (nla_len(nla) != expected_len && expected_len != OVS_ATTR_NESTED) {
+ if (nla_len(nla) != expected_len &&
+ !(expected_len == OVS_ATTR_NESTED ||
+ expected_len == OVS_ATTR_VARIABLE)) {
OVS_NLERR(log, "Key %d has unexpected len %d expected %d",
type, nla_len(nla), expected_len);
return -EINVAL;
@@ -473,29 +481,45 @@ static int genev_tun_opt_from_nlattr(const struct nlattr *a,
return 0;
}
-static const struct nla_policy vxlan_opt_policy[OVS_VXLAN_EXT_MAX + 1] = {
- [OVS_VXLAN_EXT_GBP] = { .type = NLA_U32 },
-};
-
-static int vxlan_tun_opt_from_nlattr(const struct nlattr *a,
+static int vxlan_tun_opt_from_nlattr(const struct nlattr *attr,
struct sw_flow_match *match, bool is_mask,
bool log)
{
- struct nlattr *tb[OVS_VXLAN_EXT_MAX+1];
+ struct nlattr *a;
+ int rem;
unsigned long opt_key_offset;
struct vxlan_metadata opts;
- int err;
BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
- err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
- if (err < 0)
- return err;
-
memset(&opts, 0, sizeof(opts));
+ nla_for_each_nested(a, attr, rem) {
+ int type = nla_type(a);
- if (tb[OVS_VXLAN_EXT_GBP])
- opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_GBP]);
+ if (type > OVS_VXLAN_EXT_MAX) {
+ OVS_NLERR(log, "VXLAN extension %d out of range max %d",
+ type, OVS_VXLAN_EXT_MAX);
+ return -EINVAL;
+ }
+
+ if (ovs_vxlan_ext_key_lens[type].len != nla_len(a) &&
+ !(ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_NESTED ||
+ ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_VARIABLE)) {
+ OVS_NLERR(log, "VXLAN extension %d has unexpected len %d expected %d",
+ type, nla_len(a), ovs_vxlan_ext_key_lens[type].len);
+ return -EINVAL;
+ }
+
+ switch (type) {
+ case OVS_VXLAN_EXT_GBP:
+ opts.gbp = nla_get_u32(a);
+ break;
+ default:
+ OVS_NLERR(log, "Unknown VXLAN extension attribute %d",
+ type);
+ return -EINVAL;
+ }
+ }
if (!is_mask)
SW_FLOW_KEY_PUT(match, tun_opts_len, sizeof(opts), false);
@@ -529,7 +553,8 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
}
if (ovs_tunnel_key_lens[type].len != nla_len(a) &&
- ovs_tunnel_key_lens[type].len != OVS_ATTR_NESTED) {
+ !(ovs_tunnel_key_lens[type].len == OVS_ATTR_NESTED ||
+ ovs_tunnel_key_lens[type].len == OVS_ATTR_VARIABLE)) {
OVS_NLERR(log, "Tunnel attr %d has unexpected len %d expected %d",
type, nla_len(a), ovs_tunnel_key_lens[type].len);
return -EINVAL;
@@ -1052,10 +1077,13 @@ static void nlattr_set(struct nlattr *attr, u8 val,
/* The nlattr stream should already have been validated */
nla_for_each_nested(nla, attr, rem) {
- if (tbl && tbl[nla_type(nla)].len == OVS_ATTR_NESTED)
- nlattr_set(nla, val, tbl[nla_type(nla)].next);
- else
+ if (tbl[nla_type(nla)].len == OVS_ATTR_NESTED) {
+ if (tbl[nla_type(nla)].next)
+ tbl = tbl[nla_type(nla)].next;
+ nlattr_set(nla, val, tbl);
+ } else {
memset(nla_data(nla), val, nla_len(nla));
+ }
}
}
@@ -1923,7 +1951,8 @@ static int validate_set(const struct nlattr *a,
if (key_type > OVS_KEY_ATTR_MAX ||
(ovs_key_lens[key_type].len != key_len &&
- ovs_key_lens[key_type].len != OVS_ATTR_NESTED))
+ !(ovs_key_lens[key_type].len == OVS_ATTR_NESTED ||
+ ovs_key_lens[key_type].len == OVS_ATTR_VARIABLE)))
return -EINVAL;
if (masked && !validate_masked(nla_data(ovs_key), key_len))
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] openvswitch: Fix mask generation for nested attributes.
2015-09-11 1:36 [PATCH net] openvswitch: Fix mask generation for nested attributes Jesse Gross
@ 2015-09-11 16:04 ` Pravin Shelar
2015-09-12 1:37 ` Jesse Gross
0 siblings, 1 reply; 3+ messages in thread
From: Pravin Shelar @ 2015-09-11 16:04 UTC (permalink / raw)
To: Jesse Gross; +Cc: David Miller, netdev
On Thu, Sep 10, 2015 at 6:36 PM, Jesse Gross <jesse@nicira.com> wrote:
> Masks were added to OVS flows in a way that was backwards compatible
> with userspace programs that did not generate masks. As a result, it is
> possible that we may receive flows that do not have a mask and we need
> to synthesize one.
>
> Generating a mask requires iterating over attributes and descending into
> nested attributes. For each level we need to know the size to generate the
> correct mask. We do this with a linked table of attribute types.
>
> Although the logic to handle these nested attributes was there in concept,
> there are a number of bugs in practice. Examples include incomplete links
> between tables, variable length attributes being treated as nested and
> missing sanity checks.
>
Missing Fixes tag.
> Signed-off-by: Jesse Gross <jesse@nicira.com>
This patch adds white space space errors.
./scripts/checkpatch.pl
0001-openvswitch-Fix-mask-generation-for-nested-attribute.patch
....
....
total: 15 errors, 19 warnings, 0 checks, 130 lines checked
> ---
> net/openvswitch/flow_netlink.c | 71 +++++++++++++++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index c92d6a2..83a6847 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -57,6 +57,7 @@ struct ovs_len_tbl {
> };
>
...
> unsigned long opt_key_offset;
> struct vxlan_metadata opts;
> - int err;
>
> BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
>
> - err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
> - if (err < 0)
> - return err;
> -
> memset(&opts, 0, sizeof(opts));
> + nla_for_each_nested(a, attr, rem) {
> + int type = nla_type(a);
>
> - if (tb[OVS_VXLAN_EXT_GBP])
> - opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_GBP]);
> + if (type > OVS_VXLAN_EXT_MAX) {
> + OVS_NLERR(log, "VXLAN extension %d out of range max %d",
> + type, OVS_VXLAN_EXT_MAX);
> + return -EINVAL;
> + }
> +
> + if (ovs_vxlan_ext_key_lens[type].len != nla_len(a) &&
> + !(ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_NESTED ||
> + ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_VARIABLE)) {
ovs_vxlan_ext_key_lens types is never set to nested or variable len.
so this would not true.
> + OVS_NLERR(log, "VXLAN extension %d has unexpected len %d expected %d",
> + type, nla_len(a), ovs_vxlan_ext_key_lens[type].len);
> + return -EINVAL;
> + }
> +
> + switch (type) {
> + case OVS_VXLAN_EXT_GBP:
> + opts.gbp = nla_get_u32(a);
> + break;
> + default:
> + OVS_NLERR(log, "Unknown VXLAN extension attribute %d",
> + type);
> + return -EINVAL;
> + }
> + }
Need to check for remaining bytes in this attribute.
>
> if (!is_mask)
> SW_FLOW_KEY_PUT(match, tun_opts_len, sizeof(opts), false);
> @@ -529,7 +553,8 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
> }
>
> if (ovs_tunnel_key_lens[type].len != nla_len(a) &&
> - ovs_tunnel_key_lens[type].len != OVS_ATTR_NESTED) {
> + !(ovs_tunnel_key_lens[type].len == OVS_ATTR_NESTED ||
> + ovs_tunnel_key_lens[type].len == OVS_ATTR_VARIABLE)) {
> OVS_NLERR(log, "Tunnel attr %d has unexpected len %d expected %d",
> type, nla_len(a), ovs_tunnel_key_lens[type].len);
> return -EINVAL;
> @@ -1052,10 +1077,13 @@ static void nlattr_set(struct nlattr *attr, u8 val,
>
> /* The nlattr stream should already have been validated */
> nla_for_each_nested(nla, attr, rem) {
> - if (tbl && tbl[nla_type(nla)].len == OVS_ATTR_NESTED)
> - nlattr_set(nla, val, tbl[nla_type(nla)].next);
> - else
> + if (tbl[nla_type(nla)].len == OVS_ATTR_NESTED) {
> + if (tbl[nla_type(nla)].next)
> + tbl = tbl[nla_type(nla)].next;
> + nlattr_set(nla, val, tbl);
> + } else {
> memset(nla_data(nla), val, nla_len(nla));
> + }
> }
> }
>
> @@ -1923,7 +1951,8 @@ static int validate_set(const struct nlattr *a,
>
> if (key_type > OVS_KEY_ATTR_MAX ||
> (ovs_key_lens[key_type].len != key_len &&
> - ovs_key_lens[key_type].len != OVS_ATTR_NESTED))
> + !(ovs_key_lens[key_type].len == OVS_ATTR_NESTED ||
> + ovs_key_lens[key_type].len == OVS_ATTR_VARIABLE)))
Same check are done multiple times, It would be nice if we add helper
function for this.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] openvswitch: Fix mask generation for nested attributes.
2015-09-11 16:04 ` Pravin Shelar
@ 2015-09-12 1:37 ` Jesse Gross
0 siblings, 0 replies; 3+ messages in thread
From: Jesse Gross @ 2015-09-12 1:37 UTC (permalink / raw)
To: Pravin Shelar; +Cc: David Miller, netdev
On Fri, Sep 11, 2015 at 9:04 AM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Thu, Sep 10, 2015 at 6:36 PM, Jesse Gross <jesse@nicira.com> wrote:
>> Masks were added to OVS flows in a way that was backwards compatible
>> with userspace programs that did not generate masks. As a result, it is
>> possible that we may receive flows that do not have a mask and we need
>> to synthesize one.
>>
>> Generating a mask requires iterating over attributes and descending into
>> nested attributes. For each level we need to know the size to generate the
>> correct mask. We do this with a linked table of attribute types.
>>
>> Although the logic to handle these nested attributes was there in concept,
>> there are a number of bugs in practice. Examples include incomplete links
>> between tables, variable length attributes being treated as nested and
>> missing sanity checks.
>>
> Missing Fixes tag.
This isn't really fixing one commit, it's more something that has
degraded over time. I'm also not looking for this to go to stable
since I haven't actually heard reports of this causing a problem, it's
just something I saw by looking at the code.
> This patch adds white space space errors.
>
> ./scripts/checkpatch.pl
> 0001-openvswitch-Fix-mask-generation-for-nested-attribute.patch
>
> total: 15 errors, 19 warnings, 0 checks, 130 lines checked
Fixed, thanks.
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index c92d6a2..83a6847 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -57,6 +57,7 @@ struct ovs_len_tbl {
>> };
>>
>
> ...
>> unsigned long opt_key_offset;
>> struct vxlan_metadata opts;
>> - int err;
>>
>> BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
>>
>> - err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
>> - if (err < 0)
>> - return err;
>> -
>> memset(&opts, 0, sizeof(opts));
>> + nla_for_each_nested(a, attr, rem) {
>> + int type = nla_type(a);
>>
>> - if (tb[OVS_VXLAN_EXT_GBP])
>> - opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_GBP]);
>> + if (type > OVS_VXLAN_EXT_MAX) {
>> + OVS_NLERR(log, "VXLAN extension %d out of range max %d",
>> + type, OVS_VXLAN_EXT_MAX);
>> + return -EINVAL;
>> + }
>> +
>> + if (ovs_vxlan_ext_key_lens[type].len != nla_len(a) &&
>> + !(ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_NESTED ||
>> + ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_VARIABLE)) {
>
> ovs_vxlan_ext_key_lens types is never set to nested or variable len.
> so this would not true.
At the moment, it's true that it can't happen. However, I'd like to
avoid making assumptions like that in the code since it could make it
more likely that this type of problem is reintroduced in the future.
It's somewhat moot anyways after the introduction of a common helper
function.
>> + OVS_NLERR(log, "VXLAN extension %d has unexpected len %d expected %d",
>> + type, nla_len(a), ovs_vxlan_ext_key_lens[type].len);
>> + return -EINVAL;
>> + }
>> +
>> + switch (type) {
>> + case OVS_VXLAN_EXT_GBP:
>> + opts.gbp = nla_get_u32(a);
>> + break;
>> + default:
>> + OVS_NLERR(log, "Unknown VXLAN extension attribute %d",
>> + type);
>> + return -EINVAL;
>> + }
>> + }
>
> Need to check for remaining bytes in this attribute.
Added.
>> @@ -1923,7 +1951,8 @@ static int validate_set(const struct nlattr *a,
>>
>> if (key_type > OVS_KEY_ATTR_MAX ||
>> (ovs_key_lens[key_type].len != key_len &&
>> - ovs_key_lens[key_type].len != OVS_ATTR_NESTED))
>> + !(ovs_key_lens[key_type].len == OVS_ATTR_NESTED ||
>> + ovs_key_lens[key_type].len == OVS_ATTR_VARIABLE)))
>
> Same check are done multiple times, It would be nice if we add helper
> function for this.
I agree it's better. I factored these out.
I'll send out a v2 shortly.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-12 1:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-11 1:36 [PATCH net] openvswitch: Fix mask generation for nested attributes Jesse Gross
2015-09-11 16:04 ` Pravin Shelar
2015-09-12 1:37 ` Jesse Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).