Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Wed, 05 Apr 2023 13:40:34 +0200
From:   Michael Walle <michael@walle.cc>
To:     Ajay.Kathat@microchip.com,
        Johannes Berg <johannes@sipsolutions.net>,
        Kalle Valo <kvalo@kernel.org>
Cc:     linux-wireless@vger.kernel.org, Claudiu.Beznea@microchip.com,
        Sripad.Balwadgi@microchip.com, mwalle@kernel.org
Subject: Re: [PATCH] wifi: wilc1000: fix kernel oops during interface down
 during background scan
In-Reply-To: <20230404012010.15261-1-ajay.kathat@microchip.com>
References: <20230404012010.15261-1-ajay.kathat@microchip.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <f10d69fb17e03eb093d846d005d7496a@walle.cc>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

[+ wireless and cfg80211 maintainers because I'm not familiar with
cfg80211 ]

> Fix for kernel crash observed with following test procedure [1]:
>   while true;
>     do ifconfig wlan0 up;
>     iw dev wlan0 scan &
>     ifconfig wlan0 down;
>   done
> 
> During the above test procedure, the scan results are received from 
> firmware
> for 'iw scan' command gets queued even when the interface is going 
> down. It
> was causing the kernel oops when dereferencing the freed pointers.
> 
> For synchronization, 'mac_close()' calls flush_workqueue() to block its
> execution till all pending work is completed. Afterwards 'wilc->close' 
> flag
> which is set before the flush_workqueue() should avoid adding new work.
> Added 'wilc->close' check in wilc_handle_isr() which is common for
> SPI/SDIO bus to ignore the interrupts from firmware that inturns adds 
> the
> work since the interface is getting closed.

With this patch I'm now getting
    wilc1000_sdio mmc0:0001:1 wlan0: Failed to send setup multicast

when you close the interface.

> 
> 1.
> https://lore.kernel.org/linux-wireless/20221024135407.7udo3dwl3mqyv2yj@0002.3ffe.de/

should be Link:

> Reported-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>

Missing Fixes: tag. In this regard, most of the previous wilc fixes 
patches
miss a proper Fixes tag which makes the wilc1000 pretty unusable on 
stable
kernels IMHO :/

> ---
>  drivers/net/wireless/microchip/wilc1000/netdev.c | 9 +++------
>  drivers/net/wireless/microchip/wilc1000/wlan.c   | 3 +++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c
> b/drivers/net/wireless/microchip/wilc1000/netdev.c
> index e9f59de31b0b..40edee10a81f 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.c
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
> @@ -38,11 +38,6 @@ static irqreturn_t isr_bh_routine(int irq, void 
> *userdata)
>  {
>  	struct wilc *wilc = userdata;
> 
> -	if (wilc->close) {
> -		pr_err("Can't handle BH interrupt\n");
> -		return IRQ_HANDLED;
> -	}
> -

This check is still in the top half of the interrupt processing.
Shouldn't it be removed there, too? That way you can get rid of
the top half entirely and just let the irq subsys use the default
top half implementation.

>  	wilc_handle_isr(wilc);
> 
>  	return IRQ_HANDLED;
> @@ -781,13 +776,15 @@ static int wilc_mac_close(struct net_device 
> *ndev)
>  	if (vif->ndev) {
>  		netif_stop_queue(vif->ndev);
> 
> +		if (wl->open_ifcs == 0)
> +			wl->close = 1;

Ignoring the fact that this isn't protected somehow and that
there is no write barrier (maybe I'm overthinking this and
it isn't really needed for an 'int' field), this and your
reasoning with the flush_workqueue() sounds legit.

But I'm still not convinced a lock is not required.
wilc_user_scan_req::scan_result is at least updated in
wilc_disconnect() and wilc_deinit().

wilc_disconnect() is called from the cfg80211_ops::disconnect
callback. wilc_deinit() is called from net_device_ops::ndo_stop.
Is there any lock which prevents both functions be called in
parallel? wl->close is checked in the .disconnect op, but as
mentioned above, it is not protected by any lock.

-michael

> +
>  		wilc_handle_disconnect(vif);
>  		wilc_deinit_host_int(vif->ndev);
>  	}
> 
>  	if (wl->open_ifcs == 0) {
>  		netdev_dbg(ndev, "Deinitializing wilc1000\n");
> -		wl->close = 1;
>  		wilc_wlan_deinitialize(ndev);
>  	}
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
> b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 58bbf50081e4..700cb657be00 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -1066,6 +1066,9 @@ void wilc_handle_isr(struct wilc *wilc)
>  {
>  	u32 int_status;
> 
> +	if (wilc->close)
> +		return;
> +
>  	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
>  	wilc->hif_func->hif_read_int(wilc, &int_status);
> 
> --
> 2.34.1