* [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data @ 2025-04-27 0:24 Ondřej Jirman 2025-04-28 1:53 ` Ping-Ke Shih 0 siblings, 1 reply; 4+ messages in thread From: Ondřej Jirman @ 2025-04-27 0:24 UTC (permalink / raw) To: linux-kernel Cc: Ondrej Jirman, Ping-Ke Shih, open list:REALTEK WIRELESS DRIVER (rtw89) 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(-) 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) +{ + struct ieee80211_supported_band *copy = kmemdup(sband, sizeof(*sband), GFP_KERNEL); + + copy->channels = kmemdup(sband->channels, sizeof(struct ieee80211_channel) * sband->n_channels, GFP_KERNEL); + if (!copy->channels) { + kfree(copy); + return NULL; + } + + copy->bitrates = kmemdup(sband->bitrates, sizeof(struct ieee80211_rate) * sband->n_bitrates, GFP_KERNEL); + 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; -- 2.49.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data 2025-04-27 0:24 [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data Ondřej Jirman @ 2025-04-28 1:53 ` Ping-Ke Shih 2025-04-28 21:50 ` Ondřej Jirman 0 siblings, 1 reply; 4+ messages in thread From: Ping-Ke Shih @ 2025-04-28 1:53 UTC (permalink / raw) To: Ondřej Jirman, linux-kernel@vger.kernel.org Cc: open list:REALTEK WIRELESS DRIVER (rtw89) 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. 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[] > + 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data 2025-04-28 1:53 ` Ping-Ke Shih @ 2025-04-28 21:50 ` Ondřej Jirman 2025-04-29 0:21 ` Ping-Ke Shih 0 siblings, 1 reply; 4+ messages in thread From: Ondřej Jirman @ 2025-04-28 21:50 UTC (permalink / raw) To: Ping-Ke Shih Cc: linux-kernel@vger.kernel.org, open list:REALTEK WIRELESS DRIVER (rtw89) 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. > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data 2025-04-28 21:50 ` Ondřej Jirman @ 2025-04-29 0:21 ` Ping-Ke Shih 0 siblings, 0 replies; 4+ messages in thread From: Ping-Ke Shih @ 2025-04-29 0:21 UTC (permalink / raw) To: Ondřej Jirman Cc: linux-kernel@vger.kernel.org, open list:REALTEK WIRELESS DRIVER (rtw89) > > 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 }; > Ah. I was not aware of this point. Please keep it as was. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-04-29 0:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-27 0:24 [PATCH] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data Ondřej Jirman 2025-04-28 1:53 ` Ping-Ke Shih 2025-04-28 21:50 ` Ondřej Jirman 2025-04-29 0:21 ` Ping-Ke Shih
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox