[PATCH v2 07/10] wifi: mac80211: extend ifcomb check functions for multi-radio

[Johannes Berg]

On Sun, 2024-06-30 at 09:50 +0200, Felix Fietkau wrote:
> 
> +++ b/net/mac80211/ieee80211_i.h
> @@ -2043,6 +2043,7 @@ static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
>  {
>  	return test_bit(SDATA_STATE_RUNNING, &sdata->state);
>  }
> +u32 ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev);

nit: I feel like maybe there's a better place, and even if not there
should be a blank line? :)

>  
>  /* link handling */
>  void ieee80211_link_setup(struct ieee80211_link_data *link);
> @@ -2640,8 +2641,8 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
>  int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>  				 const struct cfg80211_chan_def *chandef,
>  				 enum ieee80211_chanctx_mode chanmode,
> -				 u8 radar_detect);
> -int ieee80211_max_num_channels(struct ieee80211_local *local);
> +				 u8 radar_detect, int radio_idx);
> +int ieee80211_max_num_channels(struct ieee80211_local *local, int radio_idx);
>  void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
>  				       struct ieee80211_chanctx *ctx);

but maybe just put it next to ieee80211_max_num_channels()?

> +static u32
> +__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_bss_conf *link_conf;
> +	struct ieee80211_chanctx_conf *conf;
> +	unsigned int link_id;
> +	u32 mask = 0;
> +
> +	for_each_vif_active_link(&sdata->vif, link_conf, link_id) {
> +		conf = rcu_dereference(link_conf->chanctx_conf);
> +		if (!conf || conf->radio_idx < 0)
> +			continue;
> +
> +		mask |= BIT(conf->radio_idx);
> +	}
> +
> +	return mask;
> +}
> +
> +u32 ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev)
> +{
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	return __ieee80211_get_radio_mask(sdata);

Here's the context stuff I was talking about - you're using
rcu_dereference() when it almost seems it should be sdata_dereference()
(or so, it's all equivalent now with wiphy dereference)?

I'm not sure how this even gets to be in an RCU critical section, e.g.
when called via

> +static bool
> +ieee80211_sdata_uses_radio(struct ieee80211_sub_if_data *sdata, int radio_idx)
> +{
> +	if (radio_idx < 0)
> +		return true;
> +
> +	return __ieee80211_get_radio_mask(sdata) & BIT(radio_idx);
> +}

...

> +static void
> +ieee80211_fill_ifcomb_params(struct ieee80211_local *local,
> +			     struct iface_combination_params *params,
> +			     const struct cfg80211_chan_def *chandef,
> +			     struct ieee80211_sub_if_data *sdata)
> +{

...

> +	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> +		struct wireless_dev *wdev_iter;
> +
> +		wdev_iter = &sdata_iter->wdev;
> +
> +		if (sdata_iter == sdata ||
> +		    !ieee80211_sdata_running(sdata_iter) ||
> +		    cfg80211_iftype_allowed(local->hw.wiphy,
> +					    wdev_iter->iftype, 0, 1))
> +			continue;
> +
> +		if (!ieee80211_sdata_uses_radio(sdata_iter, params->radio_idx))
> +			continue;


here?

Also the list_for_each_entry_rcu() seems weird (although I see that we
even have pre-existing code like that now), because e.g.

 ieee80211_open()
-> ieee80211_check_concurrent_iface()
-> ieee80211_check_combinations()

is called outside RCU critical section? There's an existing
list_for_each_entry_rcu() inside of it which seems like it should be a
problem, so maybe I'm missing something?

... investigates a bit ...

yeah I can't get even CONFIG_PROVE_RCU_LIST to spit out warnings, but
checking manually, we (generally) aren't in RCU critical section here,
at least not unconditionally.

johannes