| Message ID | 20190426095448.3169-1-chaitanya.mgit@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211: Handle bss expiry during connection | expand |
On Fri, 2019-04-26 at 15:24 +0530, chaitanya.mgit@gmail.com wrote: > From: Chaitanya Tata <chaitanya.tata@bluwireless.co.uk> > > If the BSS is expired during connection, the connect result will > trigger a kernel warning. Ideally cfg80211 should hold the BSS > before the connection is attempted, but as the BSSID is not known > in case of auth/assoc MLME offload (connect op) it doesn't. > > For those drivers without the connect op cfg80211 holds down the > reference so it wil not be removed from list. > > Fix this by removing the warning and silently adding the BSS back to > the bss list which is return by the driver (with proper BSSID set). > The requirements for drivers are documented in the API's. This looks good, mostly :-) > * @bssid: The BSSID of the AP (may be %NULL) > * @bss: Entry of bss to which STA got connected to, can be obtained through > - * cfg80211_get_bss() (may be %NULL). Only one parameter among @bssid and > - * @bss needs to be specified. > + * cfg80211_get_bss() (may be %NULL) but it is recommended to store the > + * bss from the connect_request and hold a reference to it and return > + * through this param to avoid warning if the bss is expired during the > + * connection, esp. for those drivers implementing connect op. > + * Only one parameter among @bssid and @bss needs to be specified. So while we're at it, we should probably amend the documentation to say that the reference to the BSS passes from the driver to cfg80211, right? > + /* bss is not NULL, so even though bss might have expired, the driver > + * is still holding a reference to it. > + */ is -> was? It's our reference now. > if (params->bss) { > - /* Make sure the bss entry provided by the driver is valid. */ > struct cfg80211_internal_bss *ibss = bss_from_pub(params->bss); > > - if (WARN_ON(list_empty(&ibss->list))) { > - cfg80211_put_bss(wdev->wiphy, params->bss); That's why we had a put_bss() here. > + /* Meanwhile if BSS is expired then add it back to the list as > + * we have just connected with it. > + */ > + if (list_empty(&ibss->list)) > + cfg80211_bss_update(rdev, ibss, ibss->pub.signal); But I think this adds *another* reference, which is wrong? We do need one reference (the original one the driver gave us) but not a second one? Also, not sure the last argument (signal_valid) is right? Basically all that really just means that cfg80211_bss_update() is probably not the right function to call. It also updates the timestamp, which is not true, we haven't received updated information we just used it and thus held on to it. I think we should probably just instead of a "cfg80211_bss_relink()" function exposed, and that just does something like list_add_tail(&new->list, &rdev->bss_list); rdev->bss_entries++; rb_insert_bss(rdev, new); rdev->bss_generation++; though I'm not - off the top of my head - sure about cfg80211_combine_bsses() being needed or not. Maybe the copy we do in cfg80211_bss_update() isn't so bad, but then I'd probably expose it without the signal_valid part and signal/timestamp updates, and just copy the information raw there? However, the "if (found)" part should never be possible here, right? Actually, maybe it is, if we got a *new* entry in the meantime, but then we'd override the newer information with the older, which is kinda bad? johannes
On Fri, Apr 26, 2019 at 3:44 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Fri, 2019-04-26 at 15:24 +0530, chaitanya.mgit@gmail.com wrote: > > From: Chaitanya Tata <chaitanya.tata@bluwireless.co.uk> > > > > If the BSS is expired during connection, the connect result will > > trigger a kernel warning. Ideally cfg80211 should hold the BSS > > before the connection is attempted, but as the BSSID is not known > > in case of auth/assoc MLME offload (connect op) it doesn't. > > > > For those drivers without the connect op cfg80211 holds down the > > reference so it wil not be removed from list. > > > > Fix this by removing the warning and silently adding the BSS back to > > the bss list which is return by the driver (with proper BSSID set). > > The requirements for drivers are documented in the API's. > > This looks good, mostly :-) > > > * @bssid: The BSSID of the AP (may be %NULL) > > * @bss: Entry of bss to which STA got connected to, can be obtained through > > - * cfg80211_get_bss() (may be %NULL). Only one parameter among @bssid and > > - * @bss needs to be specified. > > + * cfg80211_get_bss() (may be %NULL) but it is recommended to store the > > + * bss from the connect_request and hold a reference to it and return > > + * through this param to avoid warning if the bss is expired during the > > + * connection, esp. for those drivers implementing connect op. > > + * Only one parameter among @bssid and @bss needs to be specified. > > So while we're at it, we should probably amend the documentation to say > that the reference to the BSS passes from the driver to cfg80211, right? Yes > > > + /* bss is not NULL, so even though bss might have expired, the driver > > + * is still holding a reference to it. > > + */ > > is -> was? It's our reference now. Yes > > > if (params->bss) { > > - /* Make sure the bss entry provided by the driver is valid. */ > > struct cfg80211_internal_bss *ibss = bss_from_pub(params->bss); > > > > - if (WARN_ON(list_empty(&ibss->list))) { > > - cfg80211_put_bss(wdev->wiphy, params->bss); > > That's why we had a put_bss() here. I will add a change to put bss after we update. > > > + /* Meanwhile if BSS is expired then add it back to the list as > > + * we have just connected with it. > > + */ > > + if (list_empty(&ibss->list)) > > + cfg80211_bss_update(rdev, ibss, ibss->pub.signal); > > But I think this adds *another* reference, which is wrong? We do need > one reference (the original one the driver gave us) but not a second > one? This was assuming found will be false so refcnt will be reset and a fresh ref is taken. > Also, not sure the last argument (signal_valid) is right? > > Basically all that really just means that cfg80211_bss_update() is > probably not the right function to call. It also updates the timestamp, > which is not true, we haven't received updated information we just used > it and thus held on to it. > > I think we should probably just instead of a "cfg80211_bss_relink()" > function exposed, and that just does something like > > list_add_tail(&new->list, &rdev->bss_list); > rdev->bss_entries++; > rb_insert_bss(rdev, new); > > rdev->bss_generation++; > > though I'm not - off the top of my head - sure about > cfg80211_combine_bsses() being needed or not. > > Maybe the copy we do in cfg80211_bss_update() isn't so bad, but then I'd > probably expose it without the signal_valid part and signal/timestamp > updates, and just copy the information raw there? However, the "if > (found)" part should never be possible here, right? Actually, maybe it > is, if we got a *new* entry in the meantime, but then we'd override the > newer information with the older, which is kinda bad? If we get new information (scan during connection) the bss would not have expired right? so this code will not be triggered. My initial stab was in similar lines, something like below: But as the bss is expired we need to udpate ts, signal and other fields anyway, so I went ahead with full update. +void cfg80211_add_expired_bss(struct cfg80211_registered_device *rdev, + struct cfg80211_internal_bss *new) +{ + if (!new) + return; + + spin_lock_bh(&rdev->bss_lock); + new->ts = jiffies + list_add_tail(&new->list, &rdev->bss_list); + rdev->bss_entries++; + + rb_insert_bss(rdev, new); + rdev->bss_generation++; + + bss_ref_get(rdev, new); + + spin_unlock_bh(&rdev->bss_lock); +} > johannes >
On Fri, 2019-04-26 at 16:05 +0530, Krishna Chaitanya wrote: > > > + /* Meanwhile if BSS is expired then add it back to the list as > > > + * we have just connected with it. > > > + */ > > > + if (list_empty(&ibss->list)) > > > + cfg80211_bss_update(rdev, ibss, ibss->pub.signal); > > > > But I think this adds *another* reference, which is wrong? We do need > > one reference (the original one the driver gave us) but not a second > > one? > > This was assuming found will be false so refcnt will be reset and a fresh ref > is taken. Yes, it actually adds a new entry entirely, not refs the old entry. But then you have a new entry returned from cfg80211_bss_update() with a reference, and leak that reference in the code as written. You probably need to unref the old one, and keep the new one to pass on to the next function etc. > > Maybe the copy we do in cfg80211_bss_update() isn't so bad, but then I'd > > probably expose it without the signal_valid part and signal/timestamp > > updates, and just copy the information raw there? However, the "if > > (found)" part should never be possible here, right? Actually, maybe it > > is, if we got a *new* entry in the meantime, but then we'd override the > > newer information with the older, which is kinda bad? > > If we get new information (scan during connection) the bss would not have > expired right? so this code will not be triggered. It could have expired and been re-added, no? Ok, I'll admit that's a stretch since the driver is busy connecting and not scanning, but I suppose it could happen with multiple VIFs etc? > My initial stab was in similar lines, something like below: But as the > bss is expired > we need to udpate ts, signal and other fields anyway, so I went ahead > with full update. Why would we want to update ts, signal etc.? We just keep the old information, and then the entry will be held for the duration of the connection, presumably it'll get new updates while you're connected. > +void cfg80211_add_expired_bss(struct cfg80211_registered_device *rdev, > + struct cfg80211_internal_bss *new) > +{ > + if (!new) > + return; > + > + spin_lock_bh(&rdev->bss_lock); > + new->ts = jiffies > + list_add_tail(&new->list, &rdev->bss_list); > + rdev->bss_entries++; > + > + rb_insert_bss(rdev, new); > + rdev->bss_generation++; > + > + bss_ref_get(rdev, new); > + > + spin_unlock_bh(&rdev->bss_lock); > +} Yeah, I was thinking along those lines too, except there's a bit of an issue with the hidden and non-transmitting lists? johannes
On Fri, Apr 26, 2019 at 4:15 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Fri, 2019-04-26 at 16:05 +0530, Krishna Chaitanya wrote: > > > > > + /* Meanwhile if BSS is expired then add it back to the list as > > > > + * we have just connected with it. > > > > + */ > > > > + if (list_empty(&ibss->list)) > > > > + cfg80211_bss_update(rdev, ibss, ibss->pub.signal); > > > > > > But I think this adds *another* reference, which is wrong? We do need > > > one reference (the original one the driver gave us) but not a second > > > one? > > > > This was assuming found will be false so refcnt will be reset and a fresh ref > > is taken. > > Yes, it actually adds a new entry entirely, not refs the old entry. But > then you have a new entry returned from cfg80211_bss_update() with a > reference, and leak that reference in the code as written. > > You probably need to unref the old one, and keep the new one to pass on > to the next function etc. > Right, we should unref after the update. > > > Maybe the copy we do in cfg80211_bss_update() isn't so bad, but then I'd > > > probably expose it without the signal_valid part and signal/timestamp > > > updates, and just copy the information raw there? However, the "if > > > (found)" part should never be possible here, right? Actually, maybe it > > > is, if we got a *new* entry in the meantime, but then we'd override the > > > newer information with the older, which is kinda bad? > > > > If we get new information (scan during connection) the bss would not have > > expired right? so this code will not be triggered. > > It could have expired and been re-added, no? Ok, I'll admit that's a > stretch since the driver is busy connecting and not scanning, but I > suppose it could happen with multiple VIFs etc? Yes, it might be possible with multiple VIF's, when one VIF is connecting and other VIF is scanning, it can find the connecting bssid in its scan results. > > My initial stab was in similar lines, something like below: But as the > > bss is expired > > we need to udpate ts, signal and other fields anyway, so I went ahead > > with full update. > > Why would we want to update ts, signal etc.? We just keep the old > information, and then the entry will be held for the duration of the > connection, presumably it'll get new updates while you're connected. If we are holding it, then we will not be using ts anyway, so updating or not should not matter. But as its old info better to leave it that way, and update these fields only from scan results. > > +void cfg80211_add_expired_bss(struct cfg80211_registered_device *rdev, > > + struct cfg80211_internal_bss *new) > > +{ > > + if (!new) > > + return; > > + > > + spin_lock_bh(&rdev->bss_lock); > > + new->ts = jiffies > > + list_add_tail(&new->list, &rdev->bss_list); > > + rdev->bss_entries++; > > + > > + rb_insert_bss(rdev, new); > > + rdev->bss_generation++; > > + > > + bss_ref_get(rdev, new); > > + > > + spin_unlock_bh(&rdev->bss_lock); > > +} > > Yeah, I was thinking along those lines too, except there's a bit of an > issue with the hidden and non-transmitting lists? Ok, let me think for a bit and will send you V2. Thanks for quick review. > johannes >
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index bb307a11ee63..91fee5ab2433 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -6137,8 +6137,12 @@ struct cfg80211_fils_resp_params { * case. * @bssid: The BSSID of the AP (may be %NULL) * @bss: Entry of bss to which STA got connected to, can be obtained through - * cfg80211_get_bss() (may be %NULL). Only one parameter among @bssid and - * @bss needs to be specified. + * cfg80211_get_bss() (may be %NULL) but it is recommended to store the + * bss from the connect_request and hold a reference to it and return + * through this param to avoid warning if the bss is expired during the + * connection, esp. for those drivers implementing connect op. + * Only one parameter among @bssid and @bss needs to be specified. + * @req_ie: Association request IEs (may be %NULL) * @req_ie_len: Association request IEs length * @resp_ie: Association response IEs (may be %NULL) @@ -6187,7 +6191,10 @@ void cfg80211_connect_done(struct net_device *dev, * @dev: network device * @bssid: the BSSID of the AP * @bss: entry of bss to which STA got connected to, can be obtained - * through cfg80211_get_bss (may be %NULL) + * through cfg80211_get_bss (may be %NULL), but it is recommended to store + * the bss from the connect_request and hold a reference to it and return + * through this param to avoid warning if the bss is expired during the + * connection, esp. for those drivers implementing connect op. * @req_ie: association request IEs (maybe be %NULL) * @req_ie_len: association request IEs length * @resp_ie: association response IEs (may be %NULL) diff --git a/net/wireless/core.h b/net/wireless/core.h index 84d36ca7a7ab..7ad461845d07 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -531,6 +531,11 @@ void cfg80211_stop_p2p_device(struct cfg80211_registered_device *rdev, void cfg80211_stop_nan(struct cfg80211_registered_device *rdev, struct wireless_dev *wdev); +struct cfg80211_internal_bss * +cfg80211_bss_update(struct cfg80211_registered_device *rdev, + struct cfg80211_internal_bss *tmp, + bool signal_valid); + #ifdef CONFIG_CFG80211_DEVELOPER_WARNINGS #define CFG80211_DEV_WARN_ON(cond) WARN_ON(cond) #else diff --git a/net/wireless/scan.c b/net/wireless/scan.c index 287518c6caa4..0f5ae54c7644 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -1035,7 +1035,7 @@ struct cfg80211_non_tx_bss { }; /* Returned bss is reference counted and must be cleaned up appropriately. */ -static struct cfg80211_internal_bss * +struct cfg80211_internal_bss * cfg80211_bss_update(struct cfg80211_registered_device *rdev, struct cfg80211_internal_bss *tmp, bool signal_valid) diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 7d34cb884840..6881d0b151b2 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -795,14 +795,17 @@ void cfg80211_connect_done(struct net_device *dev, unsigned long flags; u8 *next; + /* bss is not NULL, so even though bss might have expired, the driver + * is still holding a reference to it. + */ if (params->bss) { - /* Make sure the bss entry provided by the driver is valid. */ struct cfg80211_internal_bss *ibss = bss_from_pub(params->bss); - if (WARN_ON(list_empty(&ibss->list))) { - cfg80211_put_bss(wdev->wiphy, params->bss); - return; - } + /* Meanwhile if BSS is expired then add it back to the list as + * we have just connected with it. + */ + if (list_empty(&ibss->list)) + cfg80211_bss_update(rdev, ibss, ibss->pub.signal); } ev = kzalloc(sizeof(*ev) + (params->bssid ? ETH_ALEN : 0) +