Maybe problem with ieee80211_tear_down_links return value.

While checking on some other problems, I ended up adding logging to the code path below from net/mac80211/link.c. This path is hit very often on my system, and if I understand the code correctly, it should only hit in error cases where MLO links have duplicated MAC addresses.

	ret = ieee80211_check_dup_link_addrs(sdata);
	if (!ret) {
		/* for keys we will not be able to undo this */
		ieee80211_tear_down_links(sdata, to_free, rem);

The ieee80211_check_dup_link_addrs method appears to return 0 when there are no duplicates, and -EALREADY when there are duplicates. So maybe the check above should be reversed to be:

if (ret) {
??

static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata)

{
	unsigned int i, j;

	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) {
		struct ieee80211_link_data *link1;

		link1 = sdata_dereference(sdata->link[i], sdata);
		if (!link1)
			continue;
		for (j = i + 1; j < IEEE80211_MLD_MAX_NUM_LINKS; j++) {
			struct ieee80211_link_data *link2;

			link2 = sdata_dereference(sdata->link[j], sdata);
			if (!link2)
				continue;

			if (ether_addr_equal(link1->conf->addr,
					     link2->conf->addr))
				return -EALREADY;
		}
	}

	return 0;
}

Thanks,
Ben

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

The ieee80211_check_dup_link_addrs() helper returns 0 when no duplicates are found and -EALREADY on duplicates, as you described.

However, in the caller the pattern:

        if (!ret) {
                /* for keys we will not be able to undo this */
                ieee80211_tear_down_links(sdata, to_free, rem);

                ieee80211_set_vif_links_bitmaps(sdata, new_links, 

dormant_links);

                /* tell the driver */
                if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
                        ret = drv_change_vif_links(sdata->local, sdata,
                                                   old_links & old_active,
                                                   new_links & 
sdata->vif.active_links,
                                                   old);
                if (!new_links)
                        ieee80211_debugfs_recreate_netdev(sdata, false);

                if (sdata->vif.type == NL80211_IFTYPE_AP)
                        ieee80211_update_apvlan_links(sdata);
        }

treats ret == 0 as the success/commit path (i.e., only proceed with tearing down removed links, updating link bitmaps, and notifying the driver after validating the new link configuration)

So I don’t think the condition should be reversed. If it were changed to if (ret), we’d end up committing the update only when duplicates are detected (-EALREADY), which seems backwards given the current flow (and the later rollback handling when ret is non-zero).

--
Ramesh

Hello Ramesh,

Thanks for the response. Yes, I was confused...I thought that branch of code was the error case, but it is actually expected case.

I'm seeing some rare debugfs corruption/crash in that code branch and started thinking it was error handling.

Thanks,
Ben

--Ben

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com