Possible memleak in system_bridge_vlan_check ?

[Karolis1661]

Describe the bug

Netifd version: 2023-02-25

Scenario: Plugging in and out ethernet cable causes netifd unreasonable memory grow when vlans configured, inspected via /proc/PID/status by vmRSS line.

Found function which might be the case in system-linux.c:

int system_bridge_vlan_check(struct device *dev, char *ifname)
{
    struct bridge_vlan_check_data data = {
        .check_dev = dev,
        .ifindex = if_nametoindex(ifname),
        .ret = -1,
        .pending = true,
    };
    static struct ifinfomsg ifi = {
        .ifi_family = AF_BRIDGE
    };
    static struct rtattr ext_req = {
        .rta_type = IFLA_EXT_MASK,
        .rta_len = RTA_LENGTH(sizeof(uint32_t)),
    };
    uint32_t filter = RTEXT_FILTER_BRVLAN;
    struct nl_cb *cb = nl_cb_alloc(NL_CB_DEFAULT);
    struct bridge_vlan *vlan;
    struct nl_msg *msg;
    int i;

    if (!data.ifindex)
        return 0;

    msg = nlmsg_alloc_simple(RTM_GETLINK, NLM_F_DUMP);

    if (nlmsg_append(msg, &ifi, sizeof(ifi), 0) ||
        nlmsg_append(msg, &ext_req, sizeof(ext_req), NLMSG_ALIGNTO) ||
        nlmsg_append(msg, &filter, sizeof(filter), 0))
        goto free;

    vlist_for_each_element(&dev->vlans, vlan, node) {
        struct bridge_vlan_hotplug_port *port;

        for (i = 0; i < vlan->n_ports; i++) {
            if (!strcmp(vlan->ports[i].ifname, ifname))
                vlan->ports[i].check = 0;
            else
                vlan->ports[i].check = -1;
        }

        list_for_each_entry(port, &vlan->hotplug_ports, list) {
            if (!strcmp(port->port.ifname, ifname))
                port->port.check = 0;
            else
                port->port.check = -1;
        }
    }

    nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, bridge_vlan_check_cb, &data);
    nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, bridge_vlan_ack_cb, &data);
    nl_cb_set(cb, NL_CB_ACK, NL_CB_CUSTOM, bridge_vlan_ack_cb, &data);
    nl_cb_err(cb, NL_CB_CUSTOM, bridge_vlan_error_cb, &data);

    if (nl_send_auto_complete(sock_rtnl, msg) < 0)
        goto free;

    data.ret = 0;
    while (data.pending)
        nl_recvmsgs(sock_rtnl, cb);

    vlist_for_each_element(&dev->vlans, vlan, node) {
        struct bridge_vlan_hotplug_port *port;

        for (i = 0; i < vlan->n_ports; i++) {
            if (!vlan->ports[i].check) {
                data.ret = 1;
                break;
            }
        }

        list_for_each_entry(port, &vlan->hotplug_ports, list) {
            if (!port->port.check) {
                data.ret = 1;
                break;
            }
        }
    }

    goto out;

free:
    nlmsg_free(msg);
out:
    nl_cb_put(cb);
    return data.ret;
}

seems like this goto out; skips nlmsg_free(msg);

After removing 'goto out;' and testing, seems like vmRSS does not grow unreasonably, altought it grows and decreases, but stays more likely stable.

Is this intentional or a memory leak?

[ptpt52]

ping @ynezz @hauke

[ptpt52]

If the execution reaches the goto out statement, the msg variable will not be freed before returning.

[ynezz]

If the execution reaches the goto out statement, the msg variable will not be freed before returning.

IMO the code is correct, you shouldn't free the buffer once nl_send_auto_complete() succeeds, so probably the memleak is caused by something else.

[nbd168]

I believe the report is correct. Other places do free the original msg after calling nl_send_auto_complete.