[1/2] nl80211: add support to enable/disable bss color collision detection

As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
(BCCA) from AP and switch to new color, but some STAs aren't processing
BCCA from AP and not doing color switch, causing them to drop data
frames from AP post color change.

Provide an option to disable color collision detection and therefore
not to do BCCA to mitigate the same from AP. If it's required in case
where STA supports BCCA handling, then it can enabled in AP using this
option.

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
---
 include/net/cfg80211.h       | 2 ++
 include/uapi/linux/nl80211.h | 3 +++
 net/mac80211/rx.c            | 3 ++-
 net/wireless/nl80211.c       | 3 +++
 4 files changed, 10 insertions(+), 1 deletion(-)


base-commit: 44bacbdf9066c590423259dbd6d520baac99c1a8

On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote:
> As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
> (BCCA) from AP and switch to new color, but some STAs aren't processing
> BCCA from AP and not doing color switch, causing them to drop data
> frames from AP post color change.
> 
> Provide an option to disable color collision detection and therefore
> not to do BCCA to mitigate the same from AP. If it's required in case
> where STA supports BCCA handling, then it can enabled in AP using this
> option.
> 

You should probably split this into cfg80211 and mac80211.

Also, this doesn't really seem to make a lot of _sense_ since nothing in
the kernel actually acts on detection of a color collision - hostapd is
acting on that.

So since you can easily make hostapd ignore the event, why do you even
need this?

johannes

On 19/01/2023 15:02, Johannes Berg wrote:
> On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote:
>> As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
>> (BCCA) from AP and switch to new color, but some STAs aren't processing
>> BCCA from AP and not doing color switch, causing them to drop data
>> frames from AP post color change.
>>
>> Provide an option to disable color collision detection and therefore
>> not to do BCCA to mitigate the same from AP. If it's required in case
>> where STA supports BCCA handling, then it can enabled in AP using this
>> option.
>>
> 
> You should probably split this into cfg80211 and mac80211.
> 
> Also, this doesn't really seem to make a lot of _sense_ since nothing in
> the kernel actually acts on detection of a color collision - hostapd is
> acting on that.
> 
> So since you can easily make hostapd ignore the event, why do you even
> need this?

This may not be related, but the software color collision detection 
sends a netlink message for every colliding frame and it can hose up the 
system if the other network is very active.

Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held.

On Thu, 2023-01-19 at 15:52 +0100, Nicolas Cavallari wrote:
> 
> This may not be related, but the software color collision detection 
> sends a netlink message for every colliding frame and it can hose up the 
> system if the other network is very active.
> 
> Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held.

Yay.

Lorenzo can you take a look at that?

johannes

> On 19/01/2023 15:02, Johannes Berg wrote:
> > On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote:
> > > As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
> > > (BCCA) from AP and switch to new color, but some STAs aren't processing
> > > BCCA from AP and not doing color switch, causing them to drop data
> > > frames from AP post color change.
> > > 
> > > Provide an option to disable color collision detection and therefore
> > > not to do BCCA to mitigate the same from AP. If it's required in case
> > > where STA supports BCCA handling, then it can enabled in AP using this
> > > option.
> > > 
> > 
> > You should probably split this into cfg80211 and mac80211.
> > 
> > Also, this doesn't really seem to make a lot of _sense_ since nothing in
> > the kernel actually acts on detection of a color collision - hostapd is
> > acting on that.
> > 
> > So since you can easily make hostapd ignore the event, why do you even
> > need this?
> 
> This may not be related, but the software color collision detection sends a
> netlink message for every colliding frame and it can hose up the system if
> the other network is very active.
> 
> Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held.

Hi Nicolas,

I agree, I think we can ratelimit netlink messages sent by the kernel to
userspace (e.g. to hostapd), I would say every 500ms is ok.
I guess we can move cfg80211_obss_color_collision_notify() in a dedicated
delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is
currently running in interrupt context).
To give an idea, what do you think about patch below? (please note it is just
compiled tested so far).

Regards,
Lorenzo

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f5d43f42f6d8..0aefaca989aa 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -4652,6 +4652,20 @@ void ieee80211_color_change_finalize_work(struct work_struct *work)
 	sdata_unlock(sdata);
 }
 
+void ieee80211_color_collision_detection_work(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct ieee80211_link_data *link =
+		container_of(delayed_work, struct ieee80211_link_data,
+			     dfs_cac_timer_work);
+	struct ieee80211_sub_if_data *sdata = link->sdata;
+
+	sdata_lock(sdata);
+	cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap,
+					     GFP_KERNEL);
+	sdata_unlock(sdata);
+}
+
 void ieee80211_color_change_finish(struct ieee80211_vif *vif)
 {
 	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
@@ -4666,11 +4680,21 @@ ieee80211_obss_color_collision_notify(struct ieee80211_vif *vif,
 				       u64 color_bitmap, gfp_t gfp)
 {
 	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct ieee80211_link_data *link = &sdata->deflink;
 
 	if (sdata->vif.bss_conf.color_change_active || sdata->vif.bss_conf.csa_active)
 		return;
 
-	cfg80211_obss_color_collision_notify(sdata->dev, color_bitmap, gfp);
+	if (delayed_work_pending(&link->color_collision_detect_work))
+		return;
+
+	link->color_bitmap = color_bitmap;
+	/* queue the color collision detection event every 500 ms in order to
+	 * avoid sending too much netlink messages to userspace.
+	 */
+	ieee80211_queue_delayed_work(&sdata->local->hw,
+				     &link->color_collision_detect_work,
+				     msecs_to_jiffies(500)); /* 500 ms */
 }
 EXPORT_SYMBOL_GPL(ieee80211_obss_color_collision_notify);
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d16606e84e22..7ca9bde3c6d2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -974,6 +974,8 @@ struct ieee80211_link_data {
 	struct cfg80211_chan_def csa_chandef;
 
 	struct work_struct color_change_finalize_work;
+	struct delayed_work color_collision_detect_work;
+	u64 color_bitmap;
 
 	/* context reservation -- protected with chanctx_mtx */
 	struct ieee80211_chanctx *reserved_chanctx;
@@ -1929,6 +1931,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 
 /* color change handling */
 void ieee80211_color_change_finalize_work(struct work_struct *work);
+void ieee80211_color_collision_detection_work(struct work_struct *work);
 
 /* interface handling */
 #define MAC80211_SUPPORTED_FEATURES_TX	(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 23ed13f15067..1ef970b457d1 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -541,6 +541,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do
 	cancel_work_sync(&sdata->deflink.color_change_finalize_work);
 
 	cancel_delayed_work_sync(&sdata->deflink.dfs_cac_timer_work);
+	cancel_delayed_work_sync(&sdata->deflink.color_collision_detect_work);
 
 	if (sdata->wdev.cac_started) {
 		chandef = sdata->vif.bss_conf.chandef;
diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index d1f5a9f7c647..acab8309d2d6 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -39,6 +39,8 @@ void ieee80211_link_init(struct ieee80211_sub_if_data *sdata,
 		  ieee80211_csa_finalize_work);
 	INIT_WORK(&link->color_change_finalize_work,
 		  ieee80211_color_change_finalize_work);
+	INIT_DELAYED_WORK(&link->color_collision_detect_work,
+			  ieee80211_color_collision_detection_work);
 	INIT_LIST_HEAD(&link->assigned_chanctx_list);
 	INIT_LIST_HEAD(&link->reserved_chanctx_list);
 	INIT_DELAYED_WORK(&link->dfs_cac_timer_work,

On 20/01/2023 16:55, Lorenzo Bianconi wrote:
>> On 19/01/2023 15:02, Johannes Berg wrote:
>>> On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote:
>>>> As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
>>>> (BCCA) from AP and switch to new color, but some STAs aren't processing
>>>> BCCA from AP and not doing color switch, causing them to drop data
>>>> frames from AP post color change.
>>>>
>>>> Provide an option to disable color collision detection and therefore
>>>> not to do BCCA to mitigate the same from AP. If it's required in case
>>>> where STA supports BCCA handling, then it can enabled in AP using this
>>>> option.
>>>>
>>>
>>> You should probably split this into cfg80211 and mac80211.
>>>
>>> Also, this doesn't really seem to make a lot of _sense_ since nothing in
>>> the kernel actually acts on detection of a color collision - hostapd is
>>> acting on that.
>>>
>>> So since you can easily make hostapd ignore the event, why do you even
>>> need this?
>>
>> This may not be related, but the software color collision detection sends a
>> netlink message for every colliding frame and it can hose up the system if
>> the other network is very active.
>>
>> Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held.
> 
> Hi Nicolas,
> 
> I agree, I think we can ratelimit netlink messages sent by the kernel to
> userspace (e.g. to hostapd), I would say every 500ms is ok.
> I guess we can move cfg80211_obss_color_collision_notify() in a dedicated
> delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is
> currently running in interrupt context).
> To give an idea, what do you think about patch below? (please note it is just
> compiled tested so far).

I think it should fix the problem, I'll try to test it.

Thanks!

> Regards,
> Lorenzo

> -----Original Message-----
> From: Johannes Berg <johannes@sipsolutions.net>
> Sent: Thursday, January 19, 2023 7:32 PM
> To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>;
> ath11k@lists.infradead.org
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 1/2] nl80211: add support to enable/disable bss color
> collision detection
> 
> On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote:
> > As per 802.11ax-2021, STAs shall process BSS Color Change Announcement
> > (BCCA) from AP and switch to new color, but some STAs aren't
> > processing BCCA from AP and not doing color switch, causing them to
> > drop data frames from AP post color change.
> >
> > Provide an option to disable color collision detection and therefore
> > not to do BCCA to mitigate the same from AP. If it's required in case
> > where STA supports BCCA handling, then it can enabled in AP using this
> > option.
> >
> 
> You should probably split this into cfg80211 and mac80211.
Sure I will do it.
> 
> Also, this doesn't really seem to make a lot of _sense_ since nothing in the
> kernel actually acts on detection of a color collision - hostapd is acting on
> that.
> 
> So since you can easily make hostapd ignore the event, why do you even
> need this?
> 
Kernel will keep sending collision events until we switch color/disable BSS color
Hence wanted to avoid the detection itself.

> johannes

Hi Lorenzo,

On 20/01/2023 16:55, Lorenzo Bianconi wrote:
> I agree, I think we can ratelimit netlink messages sent by the kernel to
> userspace (e.g. to hostapd), I would say every 500ms is ok.
> I guess we can move cfg80211_obss_color_collision_notify() in a dedicated
> delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is
> currently running in interrupt context).
> To give an idea, what do you think about patch below? (please note it is just
> compiled tested so far).

The patch does not work, the fix appears easy:
> +void ieee80211_color_collision_detection_work(struct work_struct *work)
> +{
> +	struct delayed_work *delayed_work = to_delayed_work(work);
> +	struct ieee80211_link_data *link =
> +		container_of(delayed_work, struct ieee80211_link_data,
> +			     dfs_cac_timer_work);

This should probably be color_collision_detect_work.

> +	struct ieee80211_sub_if_data *sdata = link->sdata;
> +
> +	sdata_lock(sdata);

It crashed here, link is NULL.

> +	cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap,
> +					     GFP_KERNEL);
> +	sdata_unlock(sdata);
> +}
Will test the fixed version later.

> Hi Lorenzo,
> 
> On 20/01/2023 16:55, Lorenzo Bianconi wrote:
> > I agree, I think we can ratelimit netlink messages sent by the kernel to
> > userspace (e.g. to hostapd), I would say every 500ms is ok.
> > I guess we can move cfg80211_obss_color_collision_notify() in a dedicated
> > delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is
> > currently running in interrupt context).
> > To give an idea, what do you think about patch below? (please note it is just
> > compiled tested so far).
> 
> The patch does not work, the fix appears easy:
> > +void ieee80211_color_collision_detection_work(struct work_struct *work)
> > +{
> > +	struct delayed_work *delayed_work = to_delayed_work(work);
> > +	struct ieee80211_link_data *link =
> > +		container_of(delayed_work, struct ieee80211_link_data,
> > +			     dfs_cac_timer_work);
> 
> This should probably be color_collision_detect_work.

Yep, sorry. It is just a copy paste issue :)
I will share a new version.

Regards,
Lorenzo

> 
> > +	struct ieee80211_sub_if_data *sdata = link->sdata;
> > +
> > +	sdata_lock(sdata);
> 
> It crashed here, link is NULL.
> 
> > +	cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap,
> > +					     GFP_KERNEL);
> > +	sdata_unlock(sdata);
> > +}
> Will test the fixed version later.