[PATCH] drivers: staging: r8188eu: Fix sleep-in-atomic-context bug in rtw_join_timeout_handler

[PATCH] drivers: staging: r8188eu: Fix sleep-in-atomic-context bug in rtw_join_timeout_handler

[Duoming Zhou]

The rtw_join_timeout_handler() is a timer handler that
runs in atomic context, but it could call msleep().
As a result, the sleep-in-atomic-context bug will happen.
The process is shown below:

     (atomic context)
rtw_join_timeout_handler
 _rtw_join_timeout_handler
  rtw_do_join
   rtw_select_and_join_from_scanned_queue
    rtw_indicate_disconnect
     rtw_lps_ctrl_wk_cmd
      lps_ctrl_wk_hdl
       LPS_Leave
        LPS_RF_ON_check
         msleep //sleep in atomic context

Fix by removing msleep() and replacing with mdelay().

Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 drivers/staging/r8188eu/core/rtw_pwrctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
index 870d81735b8..5290ac36f08 100644
--- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
+++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
@@ -273,7 +273,7 @@ static s32 LPS_RF_ON_check(struct adapter *padapter, u32 delay_ms)
 			err = -1;
 			break;
 		}
-		msleep(1);
+		mdelay(1);
 	}
 
 	return err;
-- 
2.17.1

Re: [PATCH] drivers: staging: r8188eu: Fix sleep-in-atomic-context bug in rtw_join_timeout_handler

[Greg KH]

On Tue, Oct 18, 2022 at 04:34:24PM +0800, Duoming Zhou wrote:
> The rtw_join_timeout_handler() is a timer handler that
> runs in atomic context, but it could call msleep().
> As a result, the sleep-in-atomic-context bug will happen.
> The process is shown below:
> 
>      (atomic context)
> rtw_join_timeout_handler

Wait, how is this an atomic timeout?

When can that happen?

>  _rtw_join_timeout_handler
>   rtw_do_join
>    rtw_select_and_join_from_scanned_queue
>     rtw_indicate_disconnect
>      rtw_lps_ctrl_wk_cmd
>       lps_ctrl_wk_hdl
>        LPS_Leave
>         LPS_RF_ON_check
>          msleep //sleep in atomic context

How was this found?

> Fix by removing msleep() and replacing with mdelay().

Wouldn't people have seen an error already if msleep() was really called
in atomic context?

And what about the other drivers that have this identical code, why only
fix one?

thanks,

greg k-h

Re: [PATCH] drivers: staging: r8188eu: Fix sleep-in-atomic-context bug in rtw_join_timeout_handler

[duoming]

Hello,

On Thu, 20 Oct 2022 17:46:47 +0200 Greg KH wrote:

> On Tue, Oct 18, 2022 at 04:34:24PM +0800, Duoming Zhou wrote:
> > The rtw_join_timeout_handler() is a timer handler that
> > runs in atomic context, but it could call msleep().
> > As a result, the sleep-in-atomic-context bug will happen.
> > The process is shown below:
> > 
> >      (atomic context)
> > rtw_join_timeout_handler
> 
> Wait, how is this an atomic timeout?

Because this function is defined as a timer handler of "assoc_timer".

The following is the detail:

void rtw_init_mlme_timer(struct adapter *padapter)
{
	struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;

	timer_setup(&pmlmepriv->assoc_timer, rtw_join_timeout_handler, 0);
        ...
}

https://elixir.bootlin.com/linux/latest/source/drivers/staging/r8188eu/os_dep/mlme_linux.c#L36

> When can that happen?

When the adapter trys to join and selects scanning queue successfully,
the assoc_timer will be actived. If this process is timeout, the callback
function rtw_join_timeout_handler will run.

> >  _rtw_join_timeout_handler
> >   rtw_do_join
> >    rtw_select_and_join_from_scanned_queue
> >     rtw_indicate_disconnect
> >      rtw_lps_ctrl_wk_cmd
> >       lps_ctrl_wk_hdl
> >        LPS_Leave
> >         LPS_RF_ON_check
> >          msleep //sleep in atomic context
> 
> How was this found?
> 
> > Fix by removing msleep() and replacing with mdelay().
> 
> Wouldn't people have seen an error already if msleep() was really called
> in atomic context?

I am sorry, This is the false alarm.

rtw_indicate_disconnect()
  -->rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_DISCONNECT, 1);

u8 rtw_lps_ctrl_wk_cmd(struct adapter *padapter, u8 lps_ctrl_type, u8 enqueue)
{
...
if (enqueue) {
...
}else {
    lps_ctrl_wk_hdl(padapter, lps_ctrl_type);
}

The enqueue equals to 1 and the lps_ctrl_wk_hdl() will not execute.

I will check carefully and avoid false alarm next time. Thank you for your reply.

Best regards,
Duoming Zhou