Re: [PATCH] station: check for roam timeout before rearming

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 8/27/24 9:10 AM, James Prestwood wrote:
> A user reported a crash which was due to the roam trigger timeout
> being overwritten, followed by a disconnect. Post-disconnect the
> timer would fire and result in a crash. Its not clear exactly where
> the overwrite was happening but upon code inspection it could
> happen in the following scenario:
> 
> 1. Beacon loss event, start roam timeout
> 2. Signal low event, no check if timeout is running and the timeout
>     gets overwritten.
> 
> The reported crash actually didn't appear to be from the above
> scenario but something else, so now all instances where the timer
> is rearmed we also check if there is already a timer set.
> ---
>   src/station.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/station.c b/src/station.c
> index 30a1232a..e188aed4 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -2221,7 +2221,7 @@ static void station_roamed(struct station *station)
>   	 * Schedule another roaming attempt in case the signal continues to
>   	 * remain low. A subsequent high signal notification will cancel it.
>   	 */
> -	if (station->signal_low)
> +	if (station->signal_low && L_WARN_ON(!station->roam_trigger_timeout))

I'm reading this as:
if signal_low and roam_trigger_timeout is NULL:
	1. print a warning
	2. rearm the timer

Is 1. intended?

>   		station_roam_timeout_rearm(station, roam_retry_interval);
>   
>   	if (station->netconfig)
> @@ -2260,7 +2260,7 @@ static void station_roam_retry(struct station *station)
>   	station->roam_scan_full = false;
>   	station->ap_directed_roaming = false;
>   
> -	if (station->signal_low)
> +	if (station->signal_low && !station->roam_trigger_timeout)
>   		station_roam_timeout_rearm(station, roam_retry_interval);
>   }
>   
> @@ -3202,6 +3202,9 @@ static void station_low_rssi(struct station *station)
>   	if (station_cannot_roam(station))
>   		return;
>   
> +	if (station->roam_trigger_timeout)
> +		return;
> +

One thing I'm a bit worried about with such a simple approach is that we might 
have different roam timeouts.  For example, the 60 second roam retry interval is 
active and we get a low rssi event.  In this case we probably should schedule a 
roam sooner.  Maybe we should book-keep timeout information better better and 
make sure the timeout happens within the minimum time (active time remaining / 
new timeout)?

>   	/* Set a 5-second initial timeout */
>   	station_roam_timeout_rearm(station, 5);
>   }

Regards,
-Denis