| Message ID | 20211122040412.17008-1-ouden.lin@realtek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | nl80211: Clear preq NL handle after Unsbcsribe mgmt | expand |
On Mon, Nov 22, 2021 at 12:04:12PM +0800, Ouden Lin wrote: > At the start of GO (start AP), Unsbcsribe mgmt will delete all management reports, including Probe Requst, > However, nl_preq is still not deleted. Well, it is for the drv->device_ap_sme == 0 case at the beginning of nl80211_setup_ap().. > Finally, Go cannot enable probe request reporting until the trigger is disabled. > > This patch will check nl_preq after Unsubscribe mgmt. Hmm.. It looks like something would indeed be needed here, but this looks a bit strange with the proposed change. > nl80211: Setup AP operations for P2P group (GO) > nl80211: Unsubscribe mgmt frames handle 0x8888dea1bcc6c2c9 (start AP) > nl80211: Setup AP(wlan1) - device_ap_sme=1 use_monitor=0 > nl80211: Subscribe to mgmt frames with AP handle 0x5629344e4a40 (device SME) In the device_ap_sme=0 case, there is already a "Disable Probe Request reporting.." between those last two lines.. > nl80211: Probe Request reporting already on! nl_preq=0x8888dea1bcdcbca9 So this does not happen. Or well, there is not even an attempt to enable this at all. In other words, the commit message should be clearer on this being an issue only for the AP SME in the driver case. > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > if (is_ap_interface(nlmode)) { > nl80211_mgmt_unsubscribe(bss, "start AP"); > + if (bss->nl_preq) { > + wpa_printf(MSG_DEBUG, "nl80211: Disable Probe Request " > + "reporting nl_preq=%p", bss->nl_preq); > + nl80211_destroy_eloop_handle(&bss->nl_preq, 0); > + } So this would end up unmasking the bss->nl_preq pointer, unregistering the eloop handler for the socket, and deleting that unmasked bss->nl_preq pointer while still leaving bss->nl_preq to point to that now freed handle. Is it clear that this really works in all cases? What wuld happen if wpa_driver_nl80211_deinit() were to call wpa_driver_nl80211_probe_req_report(bss, 0) after this? Wouldn't that end up dereferencing an invalid pointer?
Dear Sir, > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > if (is_ap_interface(nlmode)) { > nl80211_mgmt_unsubscribe(bss, "start AP"); > + if (bss->nl_preq) { > + wpa_printf(MSG_DEBUG, "nl80211: Disable Probe Request " > + "reporting nl_preq=%p", bss->nl_preq); > + nl80211_destroy_eloop_handle(&bss->nl_preq, 0); > + } > /* Setup additional AP mode functionality if needed */ > if (nl80211_setup_ap(bss)) > return -1; nl80211: Unsubscribe mgmt frames handle 0x8888dd655d343e09 (start AP) nl80211: Setup AP(wlan0) - device_ap_sme=0 use_monitor=0 device_ap_sme=0, is_ap=1, in_deinit=0, static_ap=0 nl80211: Disable Probe Request reporting nl_preq=0x8888dd655d347269 When device_ap_sme=0, nl80211_setup_ap() will call wpa_driver_nl80211_probe_req_report(bss, 0) first. If bss->nl_preq is not cleared. nl80211: Unsubscribe mgmt frames handle 0x8888ded86cffee09 (start AP) nl80211: Setup AP(wlan0) - device_ap_sme=1 use_monitor=0 nl80211: Probe Request reporting already on! nl_preq=0x8888ded86cf619f9 However, in device_ap_sme=1, no one will handle it. If we call wpa_driver_nl80211_probe_req_report(bss, 0), it will not work. So, regardless of device_ap_sme, if it always need to clear nl_preq first, I will refine the patch to nl80211_setup_ap(). @@ -5574,8 +5574,11 @@ static int nl80211_setup_ap(struct i802_bss *bss) * devices that include the AP SME, in the other case (unless using * monitor iface) we'll get it through the nl_mgmt socket instead. */ - if (!drv->device_ap_sme) - wpa_driver_nl80211_probe_req_report(bss, 0); + if (bss->nl_preq) { + wpa_printf(MSG_DEBUG, "nl80211: Disable Probe Request " + "reporting nl_preq=%p", bss->nl_preq); + nl80211_destroy_eloop_handle(&bss->nl_preq, 0); + } > Is it clear that this really works in all cases? What wuld happen if > wpa_driver_nl80211_deinit() were to call > wpa_driver_nl80211_probe_req_report(bss, 0) after this? Wouldn't that > end up dereferencing an invalid pointer? In wpa_driver_nl80211_deinit(), if bss->nl_preq exists, call wpa_driver_nl80211_probe_req_report(bss, 0). Therefore, after wpa_driver_nl80211_deinit(), bss->nl_preq is clear (null pointer). Also, wpa_driver_nl80211_probe_req_report(bss, 0) is valid only when bss->nl_preq exists. So, I think it works in all situations. Does it satisfy your question? Thank you.
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 9a9a146f7..48d278f17 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -6721,6 +6721,11 @@ done: if (is_ap_interface(nlmode)) { nl80211_mgmt_unsubscribe(bss, "start AP"); + if (bss->nl_preq) { + wpa_printf(MSG_DEBUG, "nl80211: Disable Probe Request " + "reporting nl_preq=%p", bss->nl_preq); + nl80211_destroy_eloop_handle(&bss->nl_preq, 0); + } /* Setup additional AP mode functionality if needed */ if (nl80211_setup_ap(bss)) return -1;