wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data

From: Ondrej Jirman <megi@xff.cz>

Internally wiphy writes to individual channels in this structure,
so we must not share one static definition of channel list between
multiple device instances, because that causes hard to debug
breakage.

For example, with two rtw89 driven devices in the system, channel
information may get incoherent, preventing channel use.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---

This patch relates to this report of mine:

  https://lore.kernel.org/linux-wireless/2goskmst4na36v42p2bs47uernp6kh3gzpadhr3u3r2yvyoxlg@bfprgq2qae7p/T/#u

 drivers/net/wireless/realtek/rtw89/core.c | 48 ++++++++++++++++++-----
 1 file changed, 38 insertions(+), 10 deletions(-)

Hello,

On Mon, Apr 28, 2025 at 01:53:44AM +0000, Ping-Ke Shih wrote:
> Ondřej Jirman <megi@xff.cz> wrote:
> > 
> > Internally wiphy writes to individual channels in this structure,
> > so we must not share one static definition of channel list between
> > multiple device instances, because that causes hard to debug
> > breakage.
> > 
> > For example, with two rtw89 driven devices in the system, channel
> > information may get incoherent, preventing channel use.
> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > ---
> > 
> > This patch relates to this report of mine:
> > 
> > 
> > https://lore.kernel.org/linux-wireless/2goskmst4na36v42p2bs47uernp6kh3gzpadhr3u3r2yvyoxlg@bfprgq2qae7p
> > /T/#u
> > 
> >  drivers/net/wireless/realtek/rtw89/core.c | 48 ++++++++++++++++++-----
> >  1 file changed, 38 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > index cc9b014457ac..ae22954f5f5c 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -4398,16 +4398,44 @@ static void rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
> >         _ieee80211_set_sband_iftype_data(sband, iftype_data, idx);
> >  }
> > 
> > +static struct ieee80211_supported_band *rtw89_copy_sband(const struct ieee80211_supported_band *sband)
> 
> prefer naming rtw89_core_sband_dup().
> 
> > +{
> > +       struct ieee80211_supported_band *copy = kmemdup(sband, sizeof(*sband), GFP_KERNEL);
> 
> Then, '*dup'. 
> 
> > +
> > +       copy->channels = kmemdup(sband->channels, sizeof(struct ieee80211_channel) * sband->n_channels,
> > GFP_KERNEL);
> 
> I'm planning to use devm_ series to manage sband data, so we don't need to
> free them one by one. Do you interest to adjust that along with this patchset? 
> I mean adding additional patches to adjust the code before this patch, and
> make them as a patchset. 

Yes.

> For kmemdup, the corresponding one is devm_kmemdup. 
> 
> The line is too long. Less than 80 characters is preferred. 
> 
> > +       if (!copy->channels) {
> > +               kfree(copy);
> > +               return NULL;
> > +       }
> > +
> > +       copy->bitrates = kmemdup(sband->bitrates, sizeof(struct ieee80211_rate) * sband->n_bitrates,
> > GFP_KERNEL);
> 
> Since you have duplicated arrays of channels and bitrate, we should add const
> to them, like:
> 
>   static const struct ieee80211_channel rtw89_channels_{2ghz,5ghz,6ghz}[]
>   static const struct ieee80211_rate rtw89_bitrates[]

That will produce:

  initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]

warnings, because  struct ieee80211_supported_band doesn't have these fields
as const. The discarding of const qualifiers is apparently safe in this case,
so I can either cast the pointers to non-const when assigned here:

 270 static const struct ieee80211_supported_band rtw89_sband_2ghz = {
 271         .band           = NL80211_BAND_2GHZ,
 272         .channels       = rtw89_channels_2ghz,
 273         .n_channels     = ARRAY_SIZE(rtw89_channels_2ghz),
 274         .bitrates       = rtw89_bitrates,
 275         .n_bitrates     = ARRAY_SIZE(rtw89_bitrates),
 276         .ht_cap         = {0},
 277         .vht_cap        = {0},
 278 };

Or the code would have to get quite a bit less readable in
rtw89_core_set_supported_band() to duplicate and assign copies of bitrates and
channels there for each supported band individually instead of relying on
a common implementation in rtw89_core_sband_dup() function, and remove
assignments from const ieee80211_supported_band definitions.

Do you have a preference?

kind regards,
	o.

> 
> > +       if (!copy->bitrates) {
> > +               kfree(copy->channels);
> > +               kfree(copy);
> > +               return NULL;
> > +       }
> > +
> > +       return copy;
> > +}
> > +
> > +static void rtw89_free_sband(const struct ieee80211_supported_band *sband)
> > +{
> > +       if (sband) {
> > +               kfree(sband->bitrates);
> > +               kfree(sband->channels);
> > +               kfree(sband);
> > +       }
> > +}
> > +
> >  static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
> >  {
> >         struct ieee80211_hw *hw = rtwdev->hw;
> >         struct ieee80211_supported_band *sband_2ghz = NULL, *sband_5ghz = NULL;
> >         struct ieee80211_supported_band *sband_6ghz = NULL;
> > -       u32 size = sizeof(struct ieee80211_supported_band);
> >         u8 support_bands = rtwdev->chip->support_bands;
> > 
> >         if (support_bands & BIT(NL80211_BAND_2GHZ)) {
> > -               sband_2ghz = kmemdup(&rtw89_sband_2ghz, size, GFP_KERNEL);
> > +               sband_2ghz = rtw89_copy_sband(&rtw89_sband_2ghz);
> >                 if (!sband_2ghz)
> >                         goto err;
> >                 rtw89_init_ht_cap(rtwdev, &sband_2ghz->ht_cap);
> > @@ -4416,7 +4444,7 @@ static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
> >         }
> > 
> >         if (support_bands & BIT(NL80211_BAND_5GHZ)) {
> > -               sband_5ghz = kmemdup(&rtw89_sband_5ghz, size, GFP_KERNEL);
> > +               sband_5ghz = rtw89_copy_sband(&rtw89_sband_5ghz);
> >                 if (!sband_5ghz)
> >                         goto err;
> >                 rtw89_init_ht_cap(rtwdev, &sband_5ghz->ht_cap);
> > @@ -4426,7 +4454,7 @@ static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
> >         }
> > 
> >         if (support_bands & BIT(NL80211_BAND_6GHZ)) {
> > -               sband_6ghz = kmemdup(&rtw89_sband_6ghz, size, GFP_KERNEL);
> > +               sband_6ghz = rtw89_copy_sband(&rtw89_sband_6ghz);
> >                 if (!sband_6ghz)
> >                         goto err;
> >                 rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_6GHZ, sband_6ghz);
> > @@ -4445,9 +4473,9 @@ static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
> >                 kfree((__force void *)sband_5ghz->iftype_data);
> >         if (sband_6ghz)
> >                 kfree((__force void *)sband_6ghz->iftype_data);
> > -       kfree(sband_2ghz);
> > -       kfree(sband_5ghz);
> > -       kfree(sband_6ghz);
> > +       rtw89_free_sband(sband_2ghz);
> > +       rtw89_free_sband(sband_5ghz);
> > +       rtw89_free_sband(sband_6ghz);
> >         return -ENOMEM;
> >  }
> > 
> > @@ -4461,9 +4489,9 @@ static void rtw89_core_clr_supported_band(struct rtw89_dev *rtwdev)
> >                 kfree((__force void *)hw->wiphy->bands[NL80211_BAND_5GHZ]->iftype_data);
> >         if (hw->wiphy->bands[NL80211_BAND_6GHZ])
> >                 kfree((__force void *)hw->wiphy->bands[NL80211_BAND_6GHZ]->iftype_data);
> > -       kfree(hw->wiphy->bands[NL80211_BAND_2GHZ]);
> > -       kfree(hw->wiphy->bands[NL80211_BAND_5GHZ]);
> > -       kfree(hw->wiphy->bands[NL80211_BAND_6GHZ]);
> > +       rtw89_free_sband(hw->wiphy->bands[NL80211_BAND_2GHZ]);
> > +       rtw89_free_sband(hw->wiphy->bands[NL80211_BAND_5GHZ]);
> > +       rtw89_free_sband(hw->wiphy->bands[NL80211_BAND_6GHZ]);
> >         hw->wiphy->bands[NL80211_BAND_2GHZ] = NULL;
> >         hw->wiphy->bands[NL80211_BAND_5GHZ] = NULL;
> >         hw->wiphy->bands[NL80211_BAND_6GHZ] = NULL;
> 
> Like I mentioned above, with devm_ series, I suppose this function can be
> removed entirely. 
> 
>