[PATCH v2] staging: rtl8723bs: Fix potential race in expire_timeout_chk

[PATCH v2] staging: rtl8723bs: Fix potential race in expire_timeout_chk

[Minu Jin]

The expire_timeout_chk function currently do lock and unlock inside the
loop before calling rtw_free_stainfo().

This can be risky as the list might be changed
when the lock is briefly released.

To fix this, move expired sta_info entries into a local free_list while
holding the lock, and then perform the actual freeing after the lock is
released.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
Changes in v2:
    - Use LIST_HEAD for init list (suggested by Dan Carpenter)
    - Replace list_for_each_safe with list_for_each_entry_safe
    - Clean up unused variable 'plist' and fix type of 'tmp' iterator.
    - Remove redundant "free free_list" comment.

 drivers/staging/rtl8723bs/core/rtw_ap.c | 27 ++++++++++++-------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c b/drivers/staging/rtl8723bs/core/rtw_ap.c
index 67197c7d4a4d..d0a26134b67d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ap.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
@@ -172,45 +172,44 @@ static u8 chk_sta_is_alive(struct sta_info *psta)
 
 void expire_timeout_chk(struct adapter *padapter)
 {
-	struct list_head *phead, *plist, *tmp;
+	struct list_head *phead;
 	u8 updated = false;
-	struct sta_info *psta = NULL;
+	struct sta_info *psta = NULL, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	u8 chk_alive_num = 0;
 	char chk_alive_list[NUM_STA];
 	int i;
 
+	LIST_HEAD(free_list);
+
 	spin_lock_bh(&pstapriv->auth_list_lock);
 
 	phead = &pstapriv->auth_list;
 	/* check auth_queue */
-	list_for_each_safe(plist, tmp, phead) {
-		psta = list_entry(plist, struct sta_info, auth_list);
-
+	list_for_each_entry_safe(psta, tmp, phead, auth_list) {
 		if (psta->expire_to > 0) {
 			psta->expire_to--;
 			if (psta->expire_to == 0) {
-				list_del_init(&psta->auth_list);
+				list_move(&psta->auth_list, &free_list);
 				pstapriv->auth_list_cnt--;
-
-				spin_unlock_bh(&pstapriv->auth_list_lock);
-
-				rtw_free_stainfo(padapter, psta);
-
-				spin_lock_bh(&pstapriv->auth_list_lock);
 			}
 		}
 	}
 
 	spin_unlock_bh(&pstapriv->auth_list_lock);
+
+	list_for_each_entry_safe(psta, tmp, &free_list, auth_list) {
+		list_del_init(&psta->auth_list);
+		rtw_free_stainfo(padapter, psta);
+	}
+
 	psta = NULL;
 
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 
 	phead = &pstapriv->asoc_list;
 	/* check asoc_queue */
-	list_for_each_safe(plist, tmp, phead) {
-		psta = list_entry(plist, struct sta_info, asoc_list);
+	list_for_each_entry_safe(psta, tmp, phead, asoc_list) {
 		if (chk_sta_is_alive(psta) || !psta->expire_to) {
 			psta->expire_to = pstapriv->expire_to;
 			psta->keep_alive_trycnt = 0;
-- 
2.43.0

Re: [PATCH v2] staging: rtl8723bs: Fix potential race in expire_timeout_chk

[Dan Carpenter]

On Thu, Jan 29, 2026 at 11:32:14AM +0900, Minu Jin wrote:
> The expire_timeout_chk function currently do lock and unlock inside the
> loop before calling rtw_free_stainfo().
> 
> This can be risky as the list might be changed
> when the lock is briefly released.
> 
> To fix this, move expired sta_info entries into a local free_list while
> holding the lock, and then perform the actual freeing after the lock is
> released.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
> Changes in v2:
>     - Use LIST_HEAD for init list (suggested by Dan Carpenter)
>     - Replace list_for_each_safe with list_for_each_entry_safe
>     - Clean up unused variable 'plist' and fix type of 'tmp' iterator.
>     - Remove redundant "free free_list" comment.

Sorry, you have gone overboard this time.  I only wanted you to clean up
the new code which you introduced in the patch.  Please don't clean up
the existing code in bugfix patch.  If you want to do that, it has to be
done separately.

> 
>  drivers/staging/rtl8723bs/core/rtw_ap.c | 27 ++++++++++++-------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c b/drivers/staging/rtl8723bs/core/rtw_ap.c
> index 67197c7d4a4d..d0a26134b67d 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
> @@ -172,45 +172,44 @@ static u8 chk_sta_is_alive(struct sta_info *psta)
>  
>  void expire_timeout_chk(struct adapter *padapter)
>  {
> -	struct list_head *phead, *plist, *tmp;
> +	struct list_head *phead;
>  	u8 updated = false;
> -	struct sta_info *psta = NULL;
> +	struct sta_info *psta = NULL, *tmp;
>  	struct sta_priv *pstapriv = &padapter->stapriv;
>  	u8 chk_alive_num = 0;
>  	char chk_alive_list[NUM_STA];
>  	int i;
>  
> +	LIST_HEAD(free_list);

Delete the blank line before "LIST_HEAD(free_list);"  Don't put a blank
line in the declaration block.

regards,
dan carpenter

[PATCH v3] staging: rtl8723bs: fix potential race in expire_timeout_chk

[Minu Jin]

The expire_timeout_chk function currently do lock and unlock inside the
loop before calling rtw_free_stainfo().

This can be risky as the list might be changed
when the lock is briefly released.

To fix this, move expired sta_info entries into a local free_list while
holding the lock, and then perform the actual freeing after the lock is
released.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
Changes in v3:
    Suggested-by Dan Carpenter
    - Use list_for_each_entry_safe() only for the new code.
    - Remove blank line in the declaration block.
    - Keep existing code unchanged

 drivers/staging/rtl8723bs/core/rtw_ap.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c b/drivers/staging/rtl8723bs/core/rtw_ap.c
index 67197c7d4a4d..2ee7cc0ebaf6 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ap.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
@@ -178,6 +178,8 @@ void expire_timeout_chk(struct adapter *padapter)
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	u8 chk_alive_num = 0;
 	char chk_alive_list[NUM_STA];
+	struct sta_info *psta_tmp;
+	LIST_HEAD(free_list);
 	int i;
 
 	spin_lock_bh(&pstapriv->auth_list_lock);
@@ -190,19 +192,19 @@ void expire_timeout_chk(struct adapter *padapter)
 		if (psta->expire_to > 0) {
 			psta->expire_to--;
 			if (psta->expire_to == 0) {
-				list_del_init(&psta->auth_list);
+				list_move(&psta->auth_list, &free_list);
 				pstapriv->auth_list_cnt--;
-
-				spin_unlock_bh(&pstapriv->auth_list_lock);
-
-				rtw_free_stainfo(padapter, psta);
-
-				spin_lock_bh(&pstapriv->auth_list_lock);
 			}
 		}
 	}
 
 	spin_unlock_bh(&pstapriv->auth_list_lock);
+
+	list_for_each_entry_safe(psta, psta_tmp, &free_list, auth_list) {
+		list_del_init(&psta->auth_list);
+		rtw_free_stainfo(padapter, psta);
+	}
+
 	psta = NULL;
 
 	spin_lock_bh(&pstapriv->asoc_list_lock);
-- 
2.43.0

Re: [PATCH v3] staging: rtl8723bs: fix potential race in expire_timeout_chk

[Dan Carpenter]

On Thu, Jan 29, 2026 at 09:33:42PM +0900, Minu Jin wrote:
> The expire_timeout_chk function currently do lock and unlock inside the
> loop before calling rtw_free_stainfo().
> 
> This can be risky as the list might be changed
> when the lock is briefly released.
> 
> To fix this, move expired sta_info entries into a local free_list while
> holding the lock, and then perform the actual freeing after the lock is
> released.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
> Changes in v3:
>     Suggested-by Dan Carpenter
>     - Use list_for_each_entry_safe() only for the new code.
>     - Remove blank line in the declaration block.
>     - Keep existing code unchanged

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter