Maybe problem with ieee80211_tear_down_links return value.

Maybe problem with ieee80211_tear_down_links return value.

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Maybe problem with ieee80211_tear_down_links return value.

[Rameshkumar Sundaram]

On 2/28/2026 6:28 AM, Ben Greear wrote:
> 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) {
> ??
> 


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

Re: Maybe problem with ieee80211_tear_down_links return value.

[Ben Greear]

On 2/27/26 22:45, Rameshkumar Sundaram wrote:
> 
> 
> On 2/28/2026 6:28 AM, Ben Greear wrote:
>> 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) {
>> ??
>>
> 
> 
> 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).

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

> 
> -- 
> Ramesh
> 
> 

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com