[ovs-dev] [PATCH 2/2] datapath: Use nla_parse_strict() for netlink parsing.

Pravin Shelar pshelar at nicira.com
Fri Aug 29 03:57:50 UTC 2014


On Tue, Aug 26, 2014 at 4:50 PM, Joe Stringer <joestringer at nicira.com> wrote:
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
>  datapath/flow_netlink.c |  219 +++++++++++++++++++----------------------------
>  1 file changed, 86 insertions(+), 133 deletions(-)
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 69d1919..aa2cece 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -287,161 +287,121 @@ size_t ovs_key_attr_size(void)
>  }
>
>  /* The size of the argument for each %OVS_KEY_ATTR_* Netlink attribute.  */
> -static const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> -       [OVS_KEY_ATTR_ENCAP] = -1,
> -       [OVS_KEY_ATTR_PRIORITY] = sizeof(u32),
> -       [OVS_KEY_ATTR_IN_PORT] = sizeof(u32),
> -       [OVS_KEY_ATTR_SKB_MARK] = sizeof(u32),
> -       [OVS_KEY_ATTR_ETHERNET] = sizeof(struct ovs_key_ethernet),
> -       [OVS_KEY_ATTR_VLAN] = sizeof(__be16),
> -       [OVS_KEY_ATTR_ETHERTYPE] = sizeof(__be16),
> -       [OVS_KEY_ATTR_IPV4] = sizeof(struct ovs_key_ipv4),
> -       [OVS_KEY_ATTR_IPV6] = sizeof(struct ovs_key_ipv6),
> -       [OVS_KEY_ATTR_TCP] = sizeof(struct ovs_key_tcp),
> -       [OVS_KEY_ATTR_TCP_FLAGS] = sizeof(__be16),
> -       [OVS_KEY_ATTR_UDP] = sizeof(struct ovs_key_udp),
> -       [OVS_KEY_ATTR_SCTP] = sizeof(struct ovs_key_sctp),
> -       [OVS_KEY_ATTR_ICMP] = sizeof(struct ovs_key_icmp),
> -       [OVS_KEY_ATTR_ICMPV6] = sizeof(struct ovs_key_icmpv6),
> -       [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
> -       [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
> -       [OVS_KEY_ATTR_DP_HASH] = sizeof(u32),
> -       [OVS_KEY_ATTR_RECIRC_ID] = sizeof(u32),
> -       [OVS_KEY_ATTR_TUNNEL] = -1,
> -       [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
> +static const struct nla_policy ovs_key_policy[OVS_KEY_ATTR_MAX + 1] = {
> +       [OVS_KEY_ATTR_ENCAP] = { .type = NLA_NESTED },
> +       [OVS_KEY_ATTR_PRIORITY] = { .len = sizeof(u32) },
> +       [OVS_KEY_ATTR_IN_PORT] = { .len = sizeof(u32) },
> +       [OVS_KEY_ATTR_SKB_MARK] = { .len = sizeof(u32) },
> +       [OVS_KEY_ATTR_ETHERNET] = { .len = sizeof(struct ovs_key_ethernet) },
> +       [OVS_KEY_ATTR_VLAN] = { .len = sizeof(__be16) },
> +       [OVS_KEY_ATTR_ETHERTYPE] = { .len = sizeof(__be16) },
> +       [OVS_KEY_ATTR_IPV4] = { .len = sizeof(struct ovs_key_ipv4) },
> +       [OVS_KEY_ATTR_IPV6] = { .len = sizeof(struct ovs_key_ipv6) },
> +       [OVS_KEY_ATTR_TCP] = { .len = sizeof(struct ovs_key_tcp) },
> +       [OVS_KEY_ATTR_TCP_FLAGS] = { .len = sizeof(__be16) },
> +       [OVS_KEY_ATTR_UDP] = { .len = sizeof(struct ovs_key_udp) },
> +       [OVS_KEY_ATTR_SCTP] = { .len = sizeof(struct ovs_key_sctp) },
> +       [OVS_KEY_ATTR_ICMP] = { .len = sizeof(struct ovs_key_icmp) },
> +       [OVS_KEY_ATTR_ICMPV6] = { .len = sizeof(struct ovs_key_icmpv6) },
> +       [OVS_KEY_ATTR_ARP] = { .len = sizeof(struct ovs_key_arp) },
> +       [OVS_KEY_ATTR_ND] = { .len = sizeof(struct ovs_key_nd) },
> +       [OVS_KEY_ATTR_DP_HASH] = { .len = sizeof(u32) },
> +       [OVS_KEY_ATTR_RECIRC_ID] = { .len = sizeof(u32) },
> +       [OVS_KEY_ATTR_TUNNEL] = { .type = NLA_NESTED },
> +       [OVS_KEY_ATTR_MPLS] = { .len = sizeof(struct ovs_key_mpls) },
>  };
>
> -static bool is_all_zero(const u8 *fp, size_t size)
> +static int parse_nlattrs(const struct nlattr *attr,
> +                        const struct nlattr *a[], int maxtype,
> +                        const struct nla_policy *policy,
> +                        u64 *attrsp, bool dup, bool nz)
>  {
> -       int i;
> -
> -       if (!fp)
> -               return false;
> -
> -       for (i = 0; i < size; i++)
> -               if (fp[i])
> -                       return false;
> -
> -       return true;
> -}
> -
> -static int __parse_flow_nlattrs(const struct nlattr *attr,
> -                               const struct nlattr *a[],
> -                               u64 *attrsp, bool nz)
> -{
> -       const struct nlattr *nla;
> -       u64 attrs;
> -       int rem;
> -
> -       attrs = *attrsp;
> -       nla_for_each_nested(nla, attr, rem) {
> -               u16 type = nla_type(nla);
> -               int expected_len;
> -
> -               if (type > OVS_KEY_ATTR_MAX) {
> -                       OVS_NLERR("Unknown key attribute (type=%d, max=%d).\n",
> -                                 type, OVS_KEY_ATTR_MAX);
> -                       return -EINVAL;
> -               }
> -
> -               if (attrs & (1ULL << type)) {
> -                       OVS_NLERR("Duplicate key attribute (type %d).\n", type);
> -                       return -EINVAL;
> -               }
> -
> -               expected_len = ovs_key_lens[type];
> -               if (nla_len(nla) != expected_len && expected_len != -1) {
> -                       OVS_NLERR("Key attribute has unexpected length (type=%d"
> -                                 ", length=%d, expected=%d).\n", type,
> -                                 nla_len(nla), expected_len);
> -                       return -EINVAL;
> -               }
> +       u64 attrs = 0;
> +       int i, err;
>
> -               if (!nz || !is_all_zero(nla_data(nla), expected_len)) {
> -                       attrs |= 1ULL << type;
> -                       a[type] = nla;
> -               }
> -       }
> -       if (rem) {
> -               OVS_NLERR("Message has %d unknown bytes.\n", rem);
> -               return -EINVAL;
> -       }
> +       err = nla_parse_strict(a, maxtype, nla_data(attr), nla_len(attr),
> +                              policy, dup, nz);
> +       if (err)
> +               return err;
>
> +       for (i = 0; i < maxtype; i++)
> +               if (a[i])
> +                       attrs |= (1ULL << i);
>         *attrsp = attrs;
> +

I have not finished review but I was wonder if you can get rid of the
attributes bit mask also?

>         return 0;
>  }
>
>  static int parse_flow_mask_nlattrs(const struct nlattr *attr,
>                                    const struct nlattr *a[], u64 *attrsp)
>  {
> -       return __parse_flow_nlattrs(attr, a, attrsp, true);
> +       return parse_nlattrs(attr, a, OVS_KEY_ATTR_MAX, ovs_key_policy,
> +                            attrsp, false, true);
>  }
>
>  static int parse_flow_nlattrs(const struct nlattr *attr,
>                               const struct nlattr *a[], u64 *attrsp)
>  {
> -       return __parse_flow_nlattrs(attr, a, attrsp, false);
> +       return parse_nlattrs(attr, a, OVS_KEY_ATTR_MAX, ovs_key_policy,
> +                            attrsp, false, false);
>  }
>
>  static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>                                 struct sw_flow_match *match, bool is_mask)
>  {
> -       struct nlattr *a;
> -       int rem;
> +       static const struct nla_policy ovs_tunnel_key_policy[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
> +               [OVS_TUNNEL_KEY_ATTR_ID] = { .len = sizeof(u64) },
> +               [OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = { .len = sizeof(u32) },
> +               [OVS_TUNNEL_KEY_ATTR_IPV4_DST] = { .len = sizeof(u32) },
> +               [OVS_TUNNEL_KEY_ATTR_TOS] = { .len = 1 },
> +               [OVS_TUNNEL_KEY_ATTR_TTL] = { .len = 1 },
> +               [OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT] = { .len = 0 },
> +               [OVS_TUNNEL_KEY_ATTR_CSUM] = { .len = 0 },
> +               [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] = { .type = NLA_NESTED },
> +       };
> +       const struct nlattr *a[OVS_TUNNEL_KEY_ATTR_MAX + 1];
>         bool ttl = false;
>         __be16 tun_flags = 0;
> +       enum ovs_tunnel_key_attr type;
> +       u64 attrs = 0;
> +       int err;
>
> -       nla_for_each_nested(a, attr, rem) {
> -               int type = nla_type(a);
> -               static const u32 ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
> -                       [OVS_TUNNEL_KEY_ATTR_ID] = sizeof(u64),
> -                       [OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = sizeof(u32),
> -                       [OVS_TUNNEL_KEY_ATTR_IPV4_DST] = sizeof(u32),
> -                       [OVS_TUNNEL_KEY_ATTR_TOS] = 1,
> -                       [OVS_TUNNEL_KEY_ATTR_TTL] = 1,
> -                       [OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT] = 0,
> -                       [OVS_TUNNEL_KEY_ATTR_CSUM] = 0,
> -                       [OVS_TUNNEL_KEY_ATTR_TP_SRC] = sizeof(u16),
> -                       [OVS_TUNNEL_KEY_ATTR_TP_DST] = sizeof(u16),
> -                       [OVS_TUNNEL_KEY_ATTR_OAM] = 0,
> -                       [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = -1,
> -               };
> +       err = parse_nlattrs(attr, a, OVS_TUNNEL_KEY_ATTR_MAX,
> +                           ovs_tunnel_key_policy, &attrs, true, is_mask);
> +       if (err)
> +               return err;
>
> -               if (type > OVS_TUNNEL_KEY_ATTR_MAX) {
> -                       OVS_NLERR("Unknown IPv4 tunnel attribute (type=%d, max=%d).\n",
> -                       type, OVS_TUNNEL_KEY_ATTR_MAX);
> -                       return -EINVAL;
> -               }
> +       for (type = 0; type <= OVS_TUNNEL_KEY_ATTR_MAX; type++) {
> +               const struct nlattr *nla;
>
> -               if (ovs_tunnel_key_lens[type] != nla_len(a) &&
> -                   ovs_tunnel_key_lens[type] != -1) {
> -                       OVS_NLERR("IPv4 tunnel attribute type has unexpected "
> -                                 " length (type=%d, length=%d, expected=%d).\n",
> -                                 type, nla_len(a), ovs_tunnel_key_lens[type]);
> -                       return -EINVAL;
> -               }
> +               if (!(attrs & (1ULL << type)))
> +                       continue;
>
> +               nla = a[type];
>                 switch (type) {
>                 case OVS_TUNNEL_KEY_ATTR_ID:
>                         SW_FLOW_KEY_PUT(match, tun_key.tun_id,
> -                                       nla_get_be64(a), is_mask);
> +                                       nla_get_be64(nla), is_mask);
>                         tun_flags |= TUNNEL_KEY;
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
>                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_src,
> -                                       nla_get_be32(a), is_mask);
> +                                       nla_get_be32(nla), is_mask);
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_IPV4_DST:
>                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_dst,
> -                                       nla_get_be32(a), is_mask);
> +                                       nla_get_be32(nla), is_mask);
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_TOS:
>                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_tos,
> -                                       nla_get_u8(a), is_mask);
> +                                       nla_get_u8(nla), is_mask);
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_TTL:
>                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_ttl,
> -                                       nla_get_u8(a), is_mask);
> +                                       nla_get_u8(nla), is_mask);
>                         ttl = true;
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT:
> @@ -452,29 +412,29 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_TP_SRC:
>                         SW_FLOW_KEY_PUT(match, tun_key.tp_src,
> -                                       nla_get_be16(a), is_mask);
> +                                       nla_get_be16(nla), is_mask);
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_TP_DST:
>                         SW_FLOW_KEY_PUT(match, tun_key.tp_dst,
> -                                       nla_get_be16(a), is_mask);
> +                                       nla_get_be16(nla), is_mask);
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_OAM:
>                         tun_flags |= TUNNEL_OAM;
>                         break;
>                 case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
>                         tun_flags |= TUNNEL_OPTIONS_PRESENT;
> -                       if (nla_len(a) > sizeof(match->key->tun_opts)) {
> +                       if (nla_len(nla) > sizeof(match->key->tun_opts)) {
>                                 OVS_NLERR("Geneve option length exceeds "
>                                           "maximum size (len %d, max %zu).\n",
> -                                         nla_len(a),
> +                                         nla_len(nla),
>                                           sizeof(match->key->tun_opts));
>                                 return -EINVAL;
>                         }
>
> -                       if (nla_len(a) % 4 != 0) {
> +                       if (nla_len(nla) % 4 != 0) {
>                                 OVS_NLERR("Geneve option length is not "
>                                           "a multiple of 4 (len %d).\n",
> -                                         nla_len(a));
> +                                         nla_len(nla));
>                                 return -EINVAL;
>                         }
>
> @@ -483,7 +443,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>                          * additional options will be silently matched.
>                          */
>                         if (!is_mask) {
> -                               SW_FLOW_KEY_PUT(match, tun_opts_len, nla_len(a),
> +                               SW_FLOW_KEY_PUT(match, tun_opts_len, nla_len(nla),
>                                                 false);
>                         } else {
>                                 /* This is somewhat unusual because it looks at
> @@ -496,10 +456,10 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>                                  * variable length and we won't have the
>                                  * information later.
>                                  */
> -                               if (match->key->tun_opts_len != nla_len(a)) {
> +                               if (match->key->tun_opts_len != nla_len(nla)) {
>                                         OVS_NLERR("Geneve option key length (%d)"
>                                            " is different from mask length (%d).",
> -                                          match->key->tun_opts_len, nla_len(a));
> +                                          match->key->tun_opts_len, nla_len(nla));
>                                         return -EINVAL;
>                                 }
>
> @@ -509,8 +469,8 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>
>                         SW_FLOW_KEY_MEMCPY_OFFSET(match,
>                                 (unsigned long)GENEVE_OPTS((struct sw_flow_key *)0,
> -                                                          nla_len(a)),
> -                               nla_data(a), nla_len(a), is_mask);
> +                                                          nla_len(nla)),
> +                               nla_data(nla), nla_len(nla), is_mask);
>                         break;
>                 default:
>                         OVS_NLERR("Unknown IPv4 tunnel attribute (%d).\n", type);
> @@ -520,11 +480,6 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>
>         SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);
>
> -       if (rem > 0) {
> -               OVS_NLERR("IPv4 tunnel attribute has %d unknown bytes.\n", rem);
> -               return -EINVAL;
> -       }
> -
>         if (!is_mask) {
>                 if (!match->key->tun_key.ipv4_dst) {
>                         OVS_NLERR("IPv4 tunnel destination address is zero.\n");
> @@ -903,10 +858,8 @@ static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
>
>         /* The nlattr stream should already have been validated */
>         nla_for_each_nested(nla, attr, rem) {
> -               /* We assume that ovs_key_lens[type] == -1 means that type is a
> -                * nested attribute
> -                */
> -               if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
> +               if (is_attr_mask_key &&
> +                   ovs_key_policy[nla_type(nla)].type == NLA_NESTED)
>                         nlattr_set(nla, val, false);
>                 else
>                         memset(nla_data(nla), val, nla_len(nla));
> @@ -1605,8 +1558,8 @@ static int validate_set(const struct nlattr *a,
>                 return -EINVAL;
>
>         if (key_type > OVS_KEY_ATTR_MAX ||
> -           (ovs_key_lens[key_type] != nla_len(ovs_key) &&
> -            ovs_key_lens[key_type] != -1))
> +           (ovs_key_policy[key_type].len != nla_len(ovs_key) &&
> +            ovs_key_policy[key_type].type != NLA_NESTED))
>                 return -EINVAL;
>
>         switch (key_type) {
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list