Fedor Pchelkin <pchelkin@xxxxxxxxx> wrote:
> Now that nullfunc skbs are recycled in a separate work item in the driver, the following race
> during initialization and processing of those skbs might lead to noticeable bugs:
>
> Waiting thread Completing thread
>
> rtw89_core_send_nullfunc()
> rtw89_core_tx_write_link()
> ...
> rtw89_pci_txwd_submit()
> skb_data->wait = NULL
> /* add skb to the queue */
> skb_queue_tail(&txwd->queue, skb)
> rtw89_pci_napi_poll()
> ...
> rtw89_pci_release_txwd_skb()
> /* get skb from the queue */
> skb_unlink(skb, &txwd->queue)
> rtw89_pci_tx_status()
> rtw89_core_tx_wait_complete()
> /* use incorrect skb_data->wait
> */
> rtw89_core_tx_kick_off_and_wait()
> /* assign skb_data->wait but too late */
>
> The value of skb_data->wait indicates whether skb is passed on to the core ieee80211 stack
> or released by the driver itself. So assure that by the time skb is added to txwd queue and
> becomes visible to the completing side, it has already allocated tx_wait-related data (in case
> it's needed).
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Fixes: 1ae5ca615285 ("wifi: rtw89: add function to wait for completion of TX skbs")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> ---
> drivers/net/wireless/realtek/rtw89/core.c | 30 ++++++++++++++---------
> drivers/net/wireless/realtek/rtw89/pci.c | 2 --
> 2 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> b/drivers/net/wireless/realtek/rtw89/core.c
> index 5c964283cd67..57f20559dfde 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1094,20 +1094,12 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev,
> struct sk_buff *sk
> int qsel, unsigned int timeout) {
> struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> - struct rtw89_tx_wait_info *wait;
> + struct rtw89_tx_wait_info *wait = skb_data->wait;
wiphy_dereference(rtwdev->hw->wiphy, skb_data->wait)
> unsigned long time_left;
> int ret = 0;
>
> lockdep_assert_wiphy(rtwdev->hw->wiphy);
>
> - wait = kzalloc(sizeof(*wait), GFP_KERNEL);
> - if (!wait) {
> - rtw89_core_tx_kick_off(rtwdev, qsel);
> - return 0;
> - }
> -
> - init_completion(&wait->completion);
> - rcu_assign_pointer(skb_data->wait, wait);
> wait->skb = skb;
Also, move "wait->skb = skb" to where wait is initialized.
>
> rtw89_core_tx_kick_off(rtwdev, qsel); @@ -1172,10 +1164,12 @@ int
> rtw89_h2c_tx(struct rtw89_dev *rtwdev, static int rtw89_core_tx_write_link(struct
> rtw89_dev *rtwdev,
> struct rtw89_vif_link *rtwvif_link,
> struct rtw89_sta_link *rtwsta_link,
> - struct sk_buff *skb, int *qsel, bool sw_mld)
> + struct sk_buff *skb, int *qsel, bool sw_mld,
> + struct rtw89_tx_wait_info *wait)
> {
> struct ieee80211_sta *sta = rtwsta_link_to_sta_safe(rtwsta_link);
> struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link);
> + struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> struct rtw89_vif *rtwvif = rtwvif_link->rtwvif;
> struct rtw89_core_tx_request tx_req = {};
> int ret;
> @@ -1192,6 +1186,8 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
> rtw89_core_tx_update_desc_info(rtwdev, &tx_req);
> rtw89_core_tx_wake(rtwdev, &tx_req);
>
> + rcu_assign_pointer(skb_data->wait, wait);
> +
> ret = rtw89_hci_tx_write(rtwdev, &tx_req);
> if (ret) {
> rtw89_err(rtwdev, "failed to transmit skb to HCI\n"); @@ -1228,7 +1224,8
> @@ int rtw89_core_tx_write(struct rtw89_dev *rtwdev, struct ieee80211_vif *vif,
> }
> }
>
> - return rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, qsel, false);
> + return rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, qsel, false,
> + NULL);
> }
>
> static __le32 rtw89_build_txwd_body0(struct rtw89_tx_desc_info *desc_info) @@ -3426,6
> +3423,7 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct rtw89_vif_link
> *rt
> struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link);
> int link_id = ieee80211_vif_is_mld(vif) ? rtwvif_link->link_id : -1;
> struct rtw89_sta_link *rtwsta_link;
> + struct rtw89_tx_wait_info *wait;
> struct ieee80211_sta *sta;
> struct ieee80211_hdr *hdr;
> struct rtw89_sta *rtwsta;
> @@ -3435,6 +3433,12 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct
> rtw89_vif_link *rt
> if (vif->type != NL80211_IFTYPE_STATION || !vif->cfg.assoc)
> return 0;
>
> + wait = kzalloc(sizeof(*wait), GFP_KERNEL);
> + if (!wait)
> + return -ENOMEM;
> +
> + init_completion(&wait->completion);
> +
> rcu_read_lock();
> sta = ieee80211_find_sta(vif, vif->cfg.ap_addr);
> if (!sta) {
> @@ -3459,7 +3463,8 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct
> rtw89_vif_link *rt
> goto out;
> }
>
Fill wait->skb as mentioned above.
> - ret = rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, &qsel, true);
> + ret = rtw89_core_tx_write_link(rtwdev, rtwvif_link, rtwsta_link, skb, &qsel, true,
> + wait);
> if (ret) {
> rtw89_warn(rtwdev, "nullfunc transmit failed: %d\n", ret);
> dev_kfree_skb_any(skb);
> @@ -3472,6 +3477,7 @@ int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev, struct
> rtw89_vif_link *rt
> timeout);
> out:
> rcu_read_unlock();
> + kfree(wait);
>
> return ret;
> }
> diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
> index 4e3034b44f56..cb9682f306a6 100644
> --- a/drivers/net/wireless/realtek/rtw89/pci.c
> +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> @@ -1372,7 +1372,6 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev,
> struct pci_dev *pdev = rtwpci->pdev;
> struct sk_buff *skb = tx_req->skb;
> struct rtw89_pci_tx_data *tx_data = RTW89_PCI_TX_SKB_CB(skb);
> - struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> bool en_wd_info = desc_info->en_wd_info;
> u32 txwd_len;
> u32 txwp_len;
> @@ -1388,7 +1387,6 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev,
> }
>
> tx_data->dma = dma;
> - rcu_assign_pointer(skb_data->wait, NULL);
>
> txwp_len = sizeof(*txwp_info);
> txwd_len = chip->txwd_body_size;
> --
> 2.51.0
>
