Re: patch 46/47 causes NULL pointer deref on mt7921

[Bert Karwatzki <spasswolf@xxxxxx>]

Am Mittwoch, dem 17.07.2024 um 19:05 +0200 schrieb Bert Karwatzki:
> Am
>
> Your fix works. (I added a WARN() statement on the early return, to see if mvif-
> > phy actually was NULL during testing).
>
> Bert Karwatzki
>

While your fix works there was still the question why the driver is suddenly
forgetting mvif->phy? So I've been testing with this script:

#!/bin/sh
for i in $(seq 1 100);
do
		nmcli radio wifi off
		nmcli radio wifi on
	# wait for wifi
	sleep 3
done

together with this patch:

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
index 4f30426afbb7..206f10473d92 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
@@ -1182,6 +1182,10 @@ static void mt7921_ipv6_addr_change(struct ieee80211_hw
*hw,
                                    struct inet6_dev *idev)
 {
        struct mt792x_vif *mvif = (struct mt792x_vif *)vif->drv_priv;
+       if (!mvif->phy) {
+               WARN(1, "mvif->phy == NULL\n");
+               return;
+       }
        struct mt792x_dev *dev = mvif->phy->dev;
        struct inet6_ifaddr *ifa;
        struct in6_addr ns_addrs[IEEE80211_BSS_ARP_ADDR_LIST_LEN];


On linux-6.10 the script can be run several times without triggering a warning
while on linux-next-20240716 running the script will trigger several warnings.
In the end the result of a bisection is this as the first commit to trigger the
warning:
commit 574e609c4e6a0843a9ed53de79e00da8fb3e7437
Author: Felix Fietkau <nbd@xxxxxxxx>
Date:   Thu Jul 4 15:09:47 2024 +0200

    wifi: mac80211: clear vif drv_priv after remove_interface when stopping

    Avoid reusing stale driver data when an interface is brought down and up
    again. In order to avoid having to duplicate the memset in every single
    driver, do it here.

    Signed-off-by: Felix Fietkau <nbd@xxxxxxxx>
    Link: https://patch.msgid.link/20240704130947.48609-1-nbd@xxxxxxxx
    Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx>

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 6d969d9f1ac9..97aee0a1a39a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -689,8 +689,12 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data
*sdata, bool going_do

 		fallthrough;
 	default:
-		if (going_down)
-			drv_remove_interface(local, sdata);
+		if (!going_down)
+			break;
+		drv_remove_interface(local, sdata);
+
+		/* Clear private driver data to prevent reuse */
+		memset(sdata->vif.drv_priv, 0, local->hw.vif_data_size);
 	}

 	ieee80211_recalc_ps(local);

As this is in generic mac80211 code this could probably also affect other
drivers and their ipv6_addr_change function. (which in turn could easily be
fixed by an early exit when mvif->phy == NULL)

Bert Karwatzki