2022-11-26 16:24:28

by Martin Kaiser

[permalink] [raw]
Subject: [PATCH 00/10] staging: r8188eu: clean up OnBeacon

This series cleans up the OnBeacon function. It tries to replace
driver-sepcific message parsing with ieee80211 helper functions.

Please apply this after the "another round of cleanups" series.

Martin Kaiser (10):
staging: r8188eu: replace one GetAddr3Ptr call
staging: r8188eu: read timestamp from ieee80211_mgmt
staging: r8188eu: replace GetAddr2Ptr calls
staging: r8188eu: pass only ies to process_p2p_ps_ie
staging: r8188eu: use ie buffer in update_beacon_info
staging: r8188eu: simplify update_sta_support_rate params
staging: r8188eu: exit if beacon is not from our bss
staging: r8188eu: stop beacon processing if kmalloc fails
staging: r8188eu: simplify error handling for missing station
staging: r8188eu: remove a variable

drivers/staging/r8188eu/core/rtw_mlme_ext.c | 130 ++++++++----------
drivers/staging/r8188eu/core/rtw_p2p.c | 11 +-
drivers/staging/r8188eu/core/rtw_wlan_util.c | 9 +-
.../staging/r8188eu/include/rtw_mlme_ext.h | 3 +-
4 files changed, 66 insertions(+), 87 deletions(-)

--
2.30.2


2022-11-26 16:25:22

by Martin Kaiser

[permalink] [raw]
Subject: [PATCH 07/10] staging: r8188eu: exit if beacon is not from our bss

Do not process an incoming beacon message in the OnBeacon function if the
beacon was sent by a base station other than the one to which we're
connected.

This patch does not modify the behaviour of the code. It reverts the if
condition and returns if the beacon should not be processed. This is
simpler than wrapping the entire processing into a large if clause.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/core/rtw_mlme_ext.c | 103 ++++++++++----------
1 file changed, 52 insertions(+), 51 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index f7d3ecf551bf..a15998d912a7 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -569,67 +569,68 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)
return;
}

- if (!memcmp(mgmt->bssid, get_my_bssid(&pmlmeinfo->network), ETH_ALEN)) {
- if (pmlmeinfo->state & WIFI_FW_AUTH_NULL) {
- /* we should update current network before auth, or some IE is wrong */
- pbss = kmalloc(sizeof(struct wlan_bssid_ex), GFP_ATOMIC);
- if (pbss) {
- if (collect_bss_info(padapter, precv_frame, pbss) == _SUCCESS) {
- update_network(&pmlmepriv->cur_network.network, pbss, padapter, true);
- rtw_get_bcn_info(&pmlmepriv->cur_network);
- }
- kfree(pbss);
+ if (memcmp(mgmt->bssid, get_my_bssid(&pmlmeinfo->network), ETH_ALEN))
+ return;
+
+ if (pmlmeinfo->state & WIFI_FW_AUTH_NULL) {
+ /* we should update current network before auth, or some IE is wrong */
+ pbss = kmalloc(sizeof(struct wlan_bssid_ex), GFP_ATOMIC);
+ if (pbss) {
+ if (collect_bss_info(padapter, precv_frame, pbss) == _SUCCESS) {
+ update_network(&pmlmepriv->cur_network.network, pbss, padapter, true);
+ rtw_get_bcn_info(&pmlmepriv->cur_network);
}
+ kfree(pbss);
+ }

- /* check the vendor of the assoc AP */
- pmlmeinfo->assoc_AP_vendor = check_assoc_AP(pframe + sizeof(struct ieee80211_hdr_3addr), len - sizeof(struct ieee80211_hdr_3addr));
+ /* check the vendor of the assoc AP */
+ pmlmeinfo->assoc_AP_vendor = check_assoc_AP(pframe + sizeof(struct ieee80211_hdr_3addr), len - sizeof(struct ieee80211_hdr_3addr));

- pmlmeext->TSFValue = le64_to_cpu(mgmt->u.beacon.timestamp);
+ pmlmeext->TSFValue = le64_to_cpu(mgmt->u.beacon.timestamp);

- /* start auth */
- start_clnt_auth(padapter);
+ /* start auth */
+ start_clnt_auth(padapter);

- return;
- }
+ return;
+ }

- if (((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE) && (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS)) {
- psta = rtw_get_stainfo(pstapriv, mgmt->sa);
- if (psta) {
- ret = rtw_check_bcn_info(padapter, pframe, len);
- if (!ret) {
- receive_disconnect(padapter,
- pmlmeinfo->network.MacAddress, 0);
- return;
- }
- /* update WMM, ERP in the beacon */
- /* todo: the timer is used instead of the number of the beacon received */
- if ((sta_rx_pkts(psta) & 0xf) == 0)
- update_beacon_info(padapter, ie_ptr, ie_len, psta);
- process_p2p_ps_ie(padapter, ie_ptr, ie_len);
+ if (((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE) && (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS)) {
+ psta = rtw_get_stainfo(pstapriv, mgmt->sa);
+ if (psta) {
+ ret = rtw_check_bcn_info(padapter, pframe, len);
+ if (!ret) {
+ receive_disconnect(padapter,
+ pmlmeinfo->network.MacAddress, 0);
+ return;
}
- } else if ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE) {
- psta = rtw_get_stainfo(pstapriv, mgmt->sa);
- if (psta) {
- /* update WMM, ERP in the beacon */
- /* todo: the timer is used instead of the number of the beacon received */
- if ((sta_rx_pkts(psta) & 0xf) == 0)
- update_beacon_info(padapter, ie_ptr, ie_len, psta);
- } else {
- /* allocate a new CAM entry for IBSS station */
- cam_idx = allocate_fw_sta_entry(padapter);
- if (cam_idx == NUM_STA)
- return;
+ /* update WMM, ERP in the beacon */
+ /* todo: the timer is used instead of the number of the beacon received */
+ if ((sta_rx_pkts(psta) & 0xf) == 0)
+ update_beacon_info(padapter, ie_ptr, ie_len, psta);
+ process_p2p_ps_ie(padapter, ie_ptr, ie_len);
+ }
+ } else if ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE) {
+ psta = rtw_get_stainfo(pstapriv, mgmt->sa);
+ if (psta) {
+ /* update WMM, ERP in the beacon */
+ /* todo: the timer is used instead of the number of the beacon received */
+ if ((sta_rx_pkts(psta) & 0xf) == 0)
+ update_beacon_info(padapter, ie_ptr, ie_len, psta);
+ } else {
+ /* allocate a new CAM entry for IBSS station */
+ cam_idx = allocate_fw_sta_entry(padapter);
+ if (cam_idx == NUM_STA)
+ return;

- /* get supported rate */
- if (update_sta_support_rate(padapter, ie_ptr, ie_len, cam_idx) == _FAIL) {
- pmlmeinfo->FW_sta_info[cam_idx].status = 0;
- return;
- }
+ /* get supported rate */
+ if (update_sta_support_rate(padapter, ie_ptr, ie_len, cam_idx) == _FAIL) {
+ pmlmeinfo->FW_sta_info[cam_idx].status = 0;
+ return;
+ }

- pmlmeext->TSFValue = le64_to_cpu(mgmt->u.beacon.timestamp);
+ pmlmeext->TSFValue = le64_to_cpu(mgmt->u.beacon.timestamp);

- report_add_sta_event(padapter, mgmt->sa, cam_idx);
- }
+ report_add_sta_event(padapter, mgmt->sa, cam_idx);
}
}
}
--
2.30.2

2022-11-26 17:15:25

by Martin Kaiser

[permalink] [raw]
Subject: [PATCH 09/10] staging: r8188eu: simplify error handling for missing station

Simplify the code to handle the case where we're associated to a station
that is not in our list of known stations.

We can simply exit in this case. This patch reverts the if-condition and
saves one level of indentation.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/core/rtw_mlme_ext.c | 24 ++++++++++-----------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 76424bcba416..362313c49c52 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -597,19 +597,19 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)

if (((pmlmeinfo->state & 0x03) == WIFI_FW_STATION_STATE) && (pmlmeinfo->state & WIFI_FW_ASSOC_SUCCESS)) {
psta = rtw_get_stainfo(pstapriv, mgmt->sa);
- if (psta) {
- ret = rtw_check_bcn_info(padapter, pframe, len);
- if (!ret) {
- receive_disconnect(padapter,
- pmlmeinfo->network.MacAddress, 0);
- return;
- }
- /* update WMM, ERP in the beacon */
- /* todo: the timer is used instead of the number of the beacon received */
- if ((sta_rx_pkts(psta) & 0xf) == 0)
- update_beacon_info(padapter, ie_ptr, ie_len, psta);
- process_p2p_ps_ie(padapter, ie_ptr, ie_len);
+ if (!psta)
+ return;
+
+ ret = rtw_check_bcn_info(padapter, pframe, len);
+ if (!ret) {
+ receive_disconnect(padapter, pmlmeinfo->network.MacAddress, 0);
+ return;
}
+ /* update WMM, ERP in the beacon */
+ /* todo: the timer is used instead of the number of the beacon received */
+ if ((sta_rx_pkts(psta) & 0xf) == 0)
+ update_beacon_info(padapter, ie_ptr, ie_len, psta);
+ process_p2p_ps_ie(padapter, ie_ptr, ie_len);
} else if ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE) {
psta = rtw_get_stainfo(pstapriv, mgmt->sa);
if (psta) {
--
2.30.2

2022-11-26 17:36:16

by Martin Kaiser

[permalink] [raw]
Subject: [PATCH 10/10] staging: r8188eu: remove a variable

Check the result of rtw_check_bcn_info directly and remove the ret
variable.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/core/rtw_mlme_ext.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 362313c49c52..d32b2d569e23 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -555,7 +555,6 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)
u8 *pframe = precv_frame->rx_data;
uint len = precv_frame->len;
struct wlan_bssid_ex *pbss;
- int ret = _SUCCESS;
u8 *ie_ptr;
u32 ie_len;

@@ -600,8 +599,7 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)
if (!psta)
return;

- ret = rtw_check_bcn_info(padapter, pframe, len);
- if (!ret) {
+ if (rtw_check_bcn_info(padapter, pframe, len) != _SUCCESS) {
receive_disconnect(padapter, pmlmeinfo->network.MacAddress, 0);
return;
}
--
2.30.2

2022-11-26 18:05:51

by Martin Kaiser

[permalink] [raw]
Subject: [PATCH 04/10] staging: r8188eu: pass only ies to process_p2p_ps_ie

The process_p2p_ps_ie function parses the information elements of a beacon
message and extracts p2p-related info.

process_p2p_ps_ie does not receive a pointer to the information elements
as one would expect. Instead it receives a pointer to the timestamp field
in the beacon message. process_p2p_ps_ie increments this pointer by
_BEACON_IE_OFFSET_ to jump to the start of the information elements (and
decreases the buffer length accordingly).

This is clumsy and hard to understand. Rewrite this such that
process_p2p_ps_ie takes a pointer to the information elements and the
total length of all elements. Check up-front that the total length is
not negative.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/core/rtw_mlme_ext.c | 9 ++++++++-
drivers/staging/r8188eu/core/rtw_p2p.c | 11 ++---------
2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 5a31b20dc46d..07c57a2b61b9 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -556,6 +556,13 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)
uint len = precv_frame->len;
struct wlan_bssid_ex *pbss;
int ret = _SUCCESS;
+ u8 *ie_ptr;
+ u32 ie_len;
+
+ ie_ptr = (u8 *)&mgmt->u.beacon.variable;
+ if (precv_frame->len < offsetof(struct ieee80211_mgmt, u.beacon.variable))
+ return;
+ ie_len = precv_frame->len - offsetof(struct ieee80211_mgmt, u.beacon.variable);

if (pmlmeext->sitesurvey_res.state == SCAN_PROCESS) {
report_survey_event(padapter, precv_frame);
@@ -598,7 +605,7 @@ static void OnBeacon(struct adapter *padapter, struct recv_frame *precv_frame)
/* todo: the timer is used instead of the number of the beacon received */
if ((sta_rx_pkts(psta) & 0xf) == 0)
update_beacon_info(padapter, pframe, len, psta);
- process_p2p_ps_ie(padapter, (pframe + WLAN_HDR_A3_LEN), (len - WLAN_HDR_A3_LEN));
+ process_p2p_ps_ie(padapter, ie_ptr, ie_len);
}
} else if ((pmlmeinfo->state & 0x03) == WIFI_FW_ADHOC_STATE) {
psta = rtw_get_stainfo(pstapriv, mgmt->sa);
diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index dc159e58f428..ce05458bd1ad 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -1505,8 +1505,6 @@ void p2p_protocol_wk_hdl(struct adapter *padapter, int intCmdType)

void process_p2p_ps_ie(struct adapter *padapter, u8 *IEs, u32 IELength)
{
- u8 *ies;
- u32 ies_len;
u8 *p2p_ie;
u32 p2p_ielen = 0;
u8 noa_attr[MAX_P2P_IE_LEN] = { 0x00 };/* NoA length should be n*(13) + 2 */
@@ -1518,13 +1516,8 @@ void process_p2p_ps_ie(struct adapter *padapter, u8 *IEs, u32 IELength)

if (rtw_p2p_chk_state(pwdinfo, P2P_STATE_NONE))
return;
- if (IELength <= _BEACON_IE_OFFSET_)
- return;

- ies = IEs + _BEACON_IE_OFFSET_;
- ies_len = IELength - _BEACON_IE_OFFSET_;
-
- p2p_ie = rtw_get_p2p_ie(ies, ies_len, NULL, &p2p_ielen);
+ p2p_ie = rtw_get_p2p_ie(IEs, IELength, NULL, &p2p_ielen);

while (p2p_ie) {
find_p2p = true;
@@ -1579,7 +1572,7 @@ void process_p2p_ps_ie(struct adapter *padapter, u8 *IEs, u32 IELength)
}

/* Get the next P2P IE */
- p2p_ie = rtw_get_p2p_ie(p2p_ie + p2p_ielen, ies_len - (p2p_ie - ies + p2p_ielen), NULL, &p2p_ielen);
+ p2p_ie = rtw_get_p2p_ie(p2p_ie + p2p_ielen, IELength - (p2p_ie - IEs + p2p_ielen), NULL, &p2p_ielen);
}

if (find_p2p) {
--
2.30.2

2022-11-27 15:14:15

by Philipp Hortmann

[permalink] [raw]
Subject: Re: [PATCH 00/10] staging: r8188eu: clean up OnBeacon

On 11/26/22 17:01, Martin Kaiser wrote:
> This series cleans up the OnBeacon function. It tries to replace
> driver-sepcific message parsing with ieee80211 helper functions.
>
> Please apply this after the "another round of cleanups" series.
>
> Martin Kaiser (10):
> staging: r8188eu: replace one GetAddr3Ptr call
> staging: r8188eu: read timestamp from ieee80211_mgmt
> staging: r8188eu: replace GetAddr2Ptr calls
> staging: r8188eu: pass only ies to process_p2p_ps_ie
> staging: r8188eu: use ie buffer in update_beacon_info
> staging: r8188eu: simplify update_sta_support_rate params
> staging: r8188eu: exit if beacon is not from our bss
> staging: r8188eu: stop beacon processing if kmalloc fails
> staging: r8188eu: simplify error handling for missing station
> staging: r8188eu: remove a variable
>
> drivers/staging/r8188eu/core/rtw_mlme_ext.c | 130 ++++++++----------
> drivers/staging/r8188eu/core/rtw_p2p.c | 11 +-
> drivers/staging/r8188eu/core/rtw_wlan_util.c | 9 +-
> .../staging/r8188eu/include/rtw_mlme_ext.h | 3 +-
> 4 files changed, 66 insertions(+), 87 deletions(-)
>
Tested-by: Philipp Hortmann <[email protected]> # Edimax N150