[lldp-devel] [PATCH 1/3] vdp22 filter interface support

Thomas-Mich Richter tmricht at linux.vnet.ibm.com
Tue Nov 26 10:07:49 UTC 2013


On 11/25/2013 10:55 PM, John Fastabend wrote:

>> +/*
>> + * Parse the IFLA_VF_PORT block of the netlink message.
>>    * Return zero on success and errno else.
>> + *
>> + * Code to parse netlink message format 1. May contain IFLA_PORT_VSI_TYPE22
>> + * and IFLA_PORT_VSI_FILTER attributes which make it netlink message format 2.
> 
> If it may contain a IFLA_PORT_VSI_TYPE22 type why is it not in the
> nla_policy for pc_vfport?
> 
>>    */
>> -static int vdpnl_vfports(struct nlattr *vfports, struct vdpnl_vsi *vsi)
>> +static struct nla_policy  pc_vfport[IFLA_MAX + 1] = {
>> +    [IFLA_PORT_VF] = {
>> +                .type = NLA_U32
>> +            },
>> +    [IFLA_PORT_VSI_TYPE] = {
>> +                .minlen = sizeof(struct ifla_port_vsi)
>> +            },
> 
> I expected to see another entry for type22 here like this,
> 
>     [IFLA_PORT_VSI_TYPE22] = { ... },
> 
Will do.
>> +    [IFLA_PORT_INSTANCE_UUID] = {
>> +                .minlen = PORT_UUID_MAX
> 
> should this be '.len = PORT_UUID_MAX' I think that this is a max length
> not a minimum length?

Correct, will set .maxlen.
> 
>> +            },
>> +    [IFLA_PORT_HOST_UUID] = {
>> +                .minlen = PORT_UUID_MAX
> 
> Same here max?
Yep

> 
>> +            },
>> +    [IFLA_PORT_REQUEST] = {
>> +                .type = NLA_U8
>> +            },
> 
> Also same question for IFLA_PORT_VSI_FILTER?

Yes, will set minlen/maxlen.
> 
>> +};
>> +
>> +static int parse_vf_port(struct vdpnl_vsi *vsi, struct nlattr *tb)
>>   {
>>       char instance[VDP_UUID_STRLEN + 2];
>> -    struct nlattr *tb_vf_ports, *tb3[IFLA_PORT_MAX + 1];
>> -    int rem;
>> +    struct nlattr *vf[IFLA_PORT_MAX + 1];
>> +    size_t dim = sizeof(vf) / sizeof(vf[0]);
>> +    int rc;
>>
>> -    if (!vfports) {
>> -        LLDPAD_DBG("%s:FOUND NO IFLA_VF_PORTS\n", __func__);
>> +    memset(vf, 0, sizeof(vf));
>> +    rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vfport);
> 
> Because you don't have the IFLA_PORT_VSI_FILTER and IFLA_PORT_VSI_TYPE22
> in the policy these attributes are not validated.
> 
> 
>> +    if (rc) {
>> +        LLDPAD_ERR("%s:IFLA_VF_PORT parsing error\n", __func__);
>>           return -EINVAL;
>>       }
>> -
>> -    nla_for_each_nested(tb_vf_ports, vfports, rem) {
>> -        if (nla_type(tb_vf_ports) != IFLA_VF_PORT) {
>> -            LLDPAD_DBG("%s:not a IFLA_VF_PORT skipping\n",
>> +    if (vf[IFLA_PORT_VF])
>> +        vsi->vf = nla_get_u32(vf[IFLA_PORT_VF]);
>> +    if (vf[IFLA_PORT_HOST_UUID]) {
>> +        unsigned char *uuid;
>> +
>> +        uuid = (unsigned char *)nla_data(vf[IFLA_PORT_HOST_UUID]);
>> +        vdp_uuid2str(uuid, instance, sizeof(instance));
>> +        LLDPAD_DBG("%s:IFLA_PORT_HOST_UUID:%s\n", __func__, instance);
>> +    }
>> +    if (vf[IFLA_PORT_RESPONSE]) {
>> +        vsi->response = nla_get_u16(vf[IFLA_PORT_RESPONSE]);
>> +        LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
>> +                vsi->response);
>> +    }
>> +    if (vf[IFLA_PORT_VSI_TYPE]) {
>> +        struct ifla_port_vsi *p;
>> +
>> +        p = (struct ifla_port_vsi *)nla_data(vf[IFLA_PORT_VSI_TYPE]);
>> +        vsi->vsi_typeid = p->vsi_type_id[2] << 16
>> +                | p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
>> +        vsi->vsi_mgrid = p->vsi_mgr_id;
>> +        vsi->vsi_typeversion = p->vsi_type_version;
>> +        vsi->nl_version = vdpnl_nlf1;
>> +        if (!vsi->maclist) {
>> +            LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>>                      __func__);
>> -            continue;
>> +            return -EINVAL;
>>           }
>> -        if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports,
>> -            ifla_port_policy)) {
>> -            LLDPAD_ERR("%s:IFLA_PORT_MAX parsing failed\n",
>> +    } else if(vf[IFLA_PORT_VSI_TYPE22] && vf[IFLA_PORT_VSI_FILTER]) {
>> +        struct ifla_port_vsi22 *p;
>> +
>> +        if (vsi->maclist) {
>> +            LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>>                      __func__);
>>               return -EINVAL;
> 
> 
> [...]
> 
>> +        p = (struct ifla_port_vsi22 *)
>> +                    nla_data(vf[IFLA_PORT_VSI_TYPE22]);
>> +        vsi->vsi_typeid = p->vsi_type_id[2] << 16
>> +                | p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
>> +        vsi->vsi_typeversion = p->vsi_type_version;
>> +        vsi->hints = p->vsi_hints;
>> +        vsi->filter_fmt = p->vsi_filter_fmt;
>> +        vsi->vsiid_fmt = p->vsi_uuidfmt;
>> +        memcpy(vsi->vsi_mgrid2, p->vsi_mgrid, PORT_UUID_MAX);
>> +        vsi->nl_version = vdpnl_nlf2;
>> +        vsi->macsz = p->vsi_filter_num;
>> +        vsi->maclist = calloc(vsi->macsz, sizeof(*vsi->maclist));
>> +        if (!vsi->maclist)
>> +            return -ENOMEM;
>> +        parse_filter_data(vsi, vf[IFLA_PORT_VSI_FILTER]);
>> +    } else {
>> +        LLDPAD_ERR("%s:IFLA_PORT_VSI_TYPE/FILTER missing\n", __func__);
>> +        return -EINVAL;
>> +    }
>> +    if (vf[IFLA_PORT_INSTANCE_UUID])
>> +        nla_memcpy(vsi->vsi_uuid, vf[IFLA_PORT_INSTANCE_UUID],
>> +               sizeof(vsi->vsi_uuid));
>> +    else {
>> +        LLDPAD_ERR("%s:IFLA_PORT_INSTANCE_UUID missing\n", __func__);
> 
> do you need to free vsi->maclist at this point?

No not necessary. The function call chain is
vdpnl_setlink --> vdpnl_parse_request +--> parse_vfinfo->list --> parse_vf_info
                                      |
                                      +--> parse_vf_ports --> parse_vf_port
Functions parse_vf_info() and parse_vf_port() allocate memory for the 
filter data. When they fail (and when they succeed) the functions return to
vdpnl_parse_request() where the maclist is free'ed regardless if the called
functions return error or not.
> 
>> +        return -EINVAL;
>> +    }
>> +    if (vf[IFLA_PORT_REQUEST])
>> +        vsi->request = nla_get_u8(vf[IFLA_PORT_REQUEST]);
>> +    else {
> 
> same here free vsi->maclist?
see previous comment above.
> 
>> +        LLDPAD_ERR("%s:IFLA_PORT_REQUEST missing\n", __func__);
>> +        return -EINVAL;
>>       }
>>       return 0;
>>   }
>>
> 
> [...]
> 
>> +/*
>> + * Parse the IFLA_VF_INFO block of the netlink message.
>> + * Return zero on success and errno else.
>> + *
>> + * Code and parse netlink message format 1.
>> + */
>> +static struct nla_policy  pc_vlanmac[IFLA_MAX + 1] = {
>> +    [IFLA_VF_MAC] = {
>> +                .minlen = sizeof(struct ifla_vf_mac)
>> +            },
>> +    [IFLA_VF_VLAN] = {
>> +                .minlen = sizeof(struct ifla_vf_vlan)
>> +            }
>> +};
>>
>> -        if (vf[IFLA_VF_MAC]) {
>> -            struct ifla_vf_mac *mac = RTA_DATA(vf[IFLA_VF_MAC]);
>> +static int parse_vf_info(struct vdpnl_vsi *vsi, struct nlattr *tb)
>> +{
>> +    struct nlattr *vf[IFLA_VF_MAX + 1];
>> +    size_t dim = sizeof(vf) / sizeof(vf[0]);
>> +    int rc;
>> +    bool have_mac = false, have_vid = false;
>>
>> -            memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
>> -            have_mac = true;
>> -        }
>> +    memset(vf, 0, sizeof(vf));
>> +    rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vlanmac);
>> +    if (rc || !vf[IFLA_VF_VLAN]) {
>> +        LLDPAD_ERR("%s:IFLA_VF_VLAN missing\n", __func__);
>> +        return -EINVAL;
>> +    }
>> +    vsi->nl_version = vdpnl_nlf1;
>> +    vsi->macsz = 1;
>> +    vsi->maclist = calloc(1, sizeof(*vsi->maclist));
>> +    if (!vsi->maclist)
>> +        return -ENOMEM;
>> +    if (vf[IFLA_VF_MAC]) {
>> +        struct ifla_vf_mac *mac = nla_data(vf[IFLA_VF_MAC]);
>> +
>> +        memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
>> +        have_mac = true;
>> +    }
>>
>> -        if (vf[IFLA_VF_VLAN]) {
>> -            struct ifla_vf_vlan *vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
>> +    if (vf[IFLA_VF_VLAN]) {
>> +        struct ifla_vf_vlan *vlan = nla_data(vf[IFLA_VF_VLAN]);
>>
>> -            vsi->maclist->vlan = vlan->vlan;
>> -            vsi->maclist->qos = vlan->qos;
>> +        vsi->maclist->vlan = vlan->vlan;
>> +        vsi->maclist->qos = vlan->qos;
>> +        if (vlan->vlan <= 4095 && vlan->qos <= 7)
>>               have_vid = true;
>> -        }
>> -        LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
>> +    }
>> +    LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
>>                  have_mac);
>> -        if (have_vid && have_mac)
>> -            vsi->filter_fmt = VDP22_FFMT_MACVID;
>> -        else if (have_vid)
>> -            vsi->filter_fmt = VDP22_FFMT_VID;
>> -        else
>> -            return -EINVAL;
>> +    if (have_vid && have_mac)
>> +        vsi->filter_fmt = VDP22_FFMT_MACVID;
>> +    else if (have_vid)
>> +        vsi->filter_fmt = VDP22_FFMT_VID;
>> +    else
> 
> free vsi->maclist?
dito

> 
>> +        rc = -EINVAL;
>> +    return rc;
>> +}
>> +
> 
> [...]
>>   /*
>>    * Build the variable part of the netlink reply message for status inquiry.
>>    * It contains the UUID and the response field for the VSI profile.
>> + *
>> + * If the vlan number changed, return the new vlan/qos to the caller.
>>    */
>> -static void vdpnl_reply2(struct vdpnl_vsi *p, struct nlmsghdr *nlh)
>> +static void vdpnl_reply2(struct vdpnl_vsi *p, struct nl_msg *nlh)
>>   {
>>       char instance[VDP_UUID_STRLEN + 2];
>>
>> -    mynla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
>> +    nla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
>>             p->vsi_uuid);
>>       vdp_uuid2str(p->vsi_uuid, instance, sizeof instance);
>>       LLDPAD_DBG("%s:IFLA_PORT_INSTANCE_UUID:%s\n", __func__, instance);
>> -    mynla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
>> +    nla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
>>       LLDPAD_DBG("%s:IFLA_PORT_VF:%d\n", __func__,  PORT_SELF_VF);
>>       if (p->response != VDP_RESPONSE_NO_RESPONSE) {
>> -        mynla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);
>> +        nla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);
> 
> Also we should start checking the return values from nla_put_*. They
> can fail if the payload becomes bigger than the default message size.
No not really.
Funtion vdpnl_reply2() is called from vdpnl_getlink() and this function
calls vdp_nllen() to calculate the length of the current filter information.
Only if there is enough space to append this filter information, function
vdpnl_reply2() is called to add the data.

But I will recheck this.

> 
>>           LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
>>                  p->response);
>> +        if (p->macsz && p->maclist->changed)
>> +            vdpnl_reply3(p, nlh);
>> +    }
>> +}
>> +
> 
> [...]
> 
> Besides the list of comments the big issue is the compatibilite with
> the kernel header files.
> 
> Thanks,
> John
> 
> 


-- 
Thomas Richter, Dept 3250, IBM LTC Boeblingen, Data Center Networking
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294



More information about the lldp-devel mailing list