On Fri, Jun 21, 2024 at 11:07:27AM +0200, Sascha Hauer wrote:
> I am running plain wpa_supplicant -i mlan0 with this config:
>
> network={
> ssid="somessid"
> mode=2
> frequency=2412
> key_mgmt=WPA-PSK WPA-PSK-SHA256
> proto=RSN
> group=CCMP
> pairwise=CCMP
> psk="12345678"
> }
>
> wait for the AP to be established, <ctrl-c> wpa_supplicant and start it
> again.
Thanks. I still can't reproduce, but that's OK. From your fuller
description now, it seems it depends on the particulars of the bss
indices in use, and maybe my device/firmware is behaving differently.
Anyway, your current description and patch below make a lot more sense.
> It doesn't seem to be a locking problem, see the patch below which fixes
> my problem.
Sure. It's probably harder to trigger real issues out of some of these
kinds of race conditions, and the logical problem below sounds a lot
more likely.
> At some point during incoming events the correct adapter->priv[]
> is selected based on bss_num and bss_type. when adapter->priv[0] is used
> for AP mode then an incoming event with type MWIFIEX_BSS_TYPE_STA leads
> to adapter->priv[1] being picked which is unused and doesn't have a
> wiphy attached to it.
Ack, that makes a lot of sense -- although I think it also implies
either you're getting some kind of corrupt event buffer from your device
too, or there's something else logically weird with your firmware when
you're receiving MWIFIEX_BSS_TYPE_STA events for a *_AP interface. (Or
maybe that's also racy, as you're changing the virtual interface from
STA to AP? Not sure.)
It also highlights that I find this fallback code from
mwifiex_process_event() weird:
/* Get BSS number and corresponding priv */
priv = mwifiex_get_priv_by_id(adapter, EVENT_GET_BSS_NUM(eventcause),
EVENT_GET_BSS_TYPE(eventcause));
if (!priv)
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
^^ // if we didn't match the right BSS/event
// metadata, we'll deliver the event to an
// arbitrary interface?
But I don't think that's your problem. And at least so far, I don't
think the AP and STA event IDs collide in any way, so I don't think
we're likely to end up misbehaving even if something is misdelievered.
>
> -------------------------8<----------------------------
>
> From 3357963821294ff7de26259a154a1cb5bab760cb Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Date: Tue, 18 Jun 2024 12:20:20 +0200
> Subject: [PATCH] mwifiex: Do not return unused priv in
> mwifiex_get_priv_by_id()
>
> mwifiex_get_priv_by_id() returns the priv pointer corresponding to the
> bss_num and bss_type, but without checking if the priv is actually
> currently in use.
> Unused priv pointers do not have a wiphy attached to them which can lead
> to NULL pointer dereferences further down the callstack.
> Fix this by returning only used priv pointers which have priv->bss_mode
> set to something else than NL80211_IFTYPE_UNSPECIFIED.
>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/marvell/mwifiex/main.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 175882485a195..c5164ae41b547 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1287,6 +1287,9 @@ mwifiex_get_priv_by_id(struct mwifiex_adapter *adapter,
>
> for (i = 0; i < adapter->priv_num; i++) {
> if (adapter->priv[i]) {
> + if (adapter->priv[i]->bss_mode == NL80211_IFTYPE_UNSPECIFIED)
> + continue;
> +
> if ((adapter->priv[i]->bss_num == bss_num) &&
> (adapter->priv[i]->bss_type == bss_type))
> break;
Acked-by: Brian Norris <briannorris@xxxxxxxxxxxx>
Something about this formatting didn't get properly picked up by
patchwork though, so you'll need to resubmit. Feel free to carry the
above tag with it.
Brian
