linux-kernel - Re: [PATCH] wifi: cfg80211: fix WARN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a074905b-34c5-4cc3-a38f-a2c4bc281e5d@quicinc.com>
Date: Tue, 19 Nov 2024 11:23:25 +0530
From: Aditya Kumar Singh <quic_adisi@...cinc.com>
To: Johannes Berg <johannes@...solutions.net>
CC: <linux-wireless@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] wifi: cfg80211: fix WARN_ON during CAC cancelling

On 11/15/24 13:44, Johannes Berg wrote:
> On Wed, 2024-11-13 at 21:50 +0530, Aditya Kumar Singh wrote:
>>
>> Because link ID is cleared from the bitmap well before link stop is
>> called. As mentioned in commit message, this is the flow -
>>
>> nl80211_remove_link
>>     > cfg80211_remove_link                -> link ID gets updated here
>>       > ieee80211_del_intf_link
>>         > ieee80211_vif_set_links
>>           > ieee80211_vif_update_links
>>             > ieee80211_link_stop         -> this ultimately tries to stop
>> 					   CAC if it is ongoing.
>>
> 
> OK, but why does it have to be that way? It's all under wiphy mutex, so
> perhaps we could just clear it later?
> 

Yeah. I tried below diff, hwsim test cases shows no regression.

--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -5046,10 +5046,11 @@ static void ieee80211_del_intf_link(struct wiphy 
*wiphy,
                                     unsigned int link_id)
  {
         struct ieee80211_sub_if_data *sdata = 
IEEE80211_WDEV_TO_SUB_IF(wdev);
+       u16 new_links = wdev->valid_links & ~BIT(link_id);

         lockdep_assert_wiphy(sdata->local->hw.wiphy);

-       ieee80211_vif_set_links(sdata, wdev->valid_links, 0);
+       ieee80211_vif_set_links(sdata, new_links, 0);
  }

  static int
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 040d62051eb9..65c8e47246b7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2843,10 +2843,9 @@ void cfg80211_remove_link(struct wireless_dev 
*wdev, unsigned int link_id)
                 break;
         }

-       wdev->valid_links &= ~BIT(link_id);
-
         rdev_del_intf_link(rdev, wdev, link_id);

+       wdev->valid_links &= ~BIT(link_id);
         eth_zero_addr(wdev->links[link_id].addr);
  }

So I will submit this as patch then?

> There's necessarily going to be some temporary inconsistency here, I'm
> not sure it matters too much if it isn't very visible?
> 

Any particular case you suspect and want me to test?

> Alternatively, could do something like
> 
>    wdev->valid_links &= ~BIT(link_id);
>    wdev->removing_link = link_id;
>    ...
>    wdev->removing_link = -1;
> 
> and accept the wdev->removing_link in these APIs like CAC?

Umm.. will work for CAC stopping from user space. But if radar is 
detected, in this flow (driver -> mac80211 -> cfg80211), valid_link 
bitmap is still valid and hence valid_bitmap check works and there will 
be no removing_links set.

So then may be both needs to be checked? Like either the link_id should 
be present in valid_link or it should be the removing_link?

if (WARN_ON(wdev->removing_link != link_id &&
	    wdev->valid_links && !(wdev->valid_links & BIT(link_id))))
	return;

I'm more inclining towards the first suggestion you gave - clearing the 
bitmap later. What's your suggestion?

-- 
Aditya