2020-10-05 16:48:38

by Thomas Pedersen

Subject: [PATCH v2 1/2] mac80211: handle lack of sband->bitrates in rates

Even though a driver or mac80211 shouldn't produce a
legacy bitrate if sband->bitrates doesn't exist, don't
crash if that is the case either.

This fixes a kernel panic if station dump is run before
last_rate can be updated with a data frame when
sband->bitrates is missing (eg. in S1G bands).

Signed-off-by: Thomas Pedersen <[email protected]>

---

v2:
WARN_ON -> WARN_ON_ONCE (Ben)
---
net/mac80211/cfg.c | 3 ++-
net/mac80211/sta_info.c | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index da70f174d629..eb52aafd9381 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -709,7 +709,8 @@ void sta_set_rate_info_tx(struct sta_info *sta,
u16 brate;

sband = ieee80211_get_sband(sta->sdata);
- if (sband) {
+ WARN_ON_ONCE(sband && !sband->bitrates);
+ if (sband && sband->bitrates) {
brate = sband->bitrates[rate->idx].bitrate;
rinfo->legacy = DIV_ROUND_UP(brate, 1 << shift);
}
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index f2840d1d95cf..fb4f2b9b294f 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2122,6 +2122,10 @@ static void sta_stats_decode_rate(struct ieee80211_local *local, u32 rate,
int rate_idx = STA_STATS_GET(LEGACY_IDX, rate);

sband = local->hw.wiphy->bands[band];
+
+ if (WARN_ON_ONCE(!sband->bitrates))
+ break;
+
brate = sband->bitrates[rate_idx].bitrate;
if (rinfo->bw == RATE_INFO_BW_5)
shift = 2;
--
2.20.1

2020-10-08 09:03:51

by Johannes Berg

Subject: Re: [PATCH v2 1/2] mac80211: handle lack of sband->bitrates in rates

On Mon, 2020-10-05 at 09:45 -0700, Thomas Pedersen wrote:
> Even though a driver or mac80211 shouldn't produce a
> legacy bitrate if sband->bitrates doesn't exist, don't
> crash if that is the case either.
>
> This fixes a kernel panic if station dump is run before
> last_rate can be updated with a data frame when
> sband->bitrates is missing (eg. in S1G bands).
>
> Signed-off-by: Thomas Pedersen <[email protected]>
>
> ---
>
> v2:
> WARN_ON -> WARN_ON_ONCE (Ben)
> ---
> net/mac80211/cfg.c | 3 ++-
> net/mac80211/sta_info.c | 4 ++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index da70f174d629..eb52aafd9381 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -709,7 +709,8 @@ void sta_set_rate_info_tx(struct sta_info *sta,
> u16 brate;
>
> sband = ieee80211_get_sband(sta->sdata);
> - if (sband) {
> + WARN_ON_ONCE(sband && !sband->bitrates);
> + if (sband && sband->bitrates) {
> brate = sband->bitrates[rate->idx].bitrate;
> rinfo->legacy = DIV_ROUND_UP(brate, 1 << shift);
> }
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index f2840d1d95cf..fb4f2b9b294f 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -2122,6 +2122,10 @@ static void sta_stats_decode_rate(struct ieee80211_local *local, u32 rate,
> int rate_idx = STA_STATS_GET(LEGACY_IDX, rate);
>
> sband = local->hw.wiphy->bands[band];
> +
> + if (WARN_ON_ONCE(!sband->bitrates))
> + break;
> +

Given the premise described in the commit message, neither of these
really seem right - why would we want to trigger a WARN_ON(_ONCE) if
userspace requests the information that early? Should be silent IMHO?

johannes