| Message ID | 20210715070745.5033-1-alokad@codeaurora.org |
|---|---|
| Headers | show |
| Series | multiple bssid and EMA support in AP mode | expand |
Hi, I don't know if this issue was already present before, but it's certainly due to the locking changes I had made with the RTNL some time ago... > +static int nl80211_parse_mbssid_config(struct wiphy *wiphy, > + struct net_device *dev, > + struct nlattr *attrs, > + struct cfg80211_mbssid_config *config, > + u8 num_elems) > +{ > + struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1]; > + struct net_device *tx_dev = dev; Here tx_dev defaults to the dev, that's fine, it might be the transmitting interface. > + if (tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]) { > + tx_ifindex = > + nla_get_u32(tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]); > + > + if (!config->index && tx_ifindex != dev->ifindex) > + return -EINVAL; > + > + tx_dev = __dev_get_by_index(wiphy_net(wiphy), tx_ifindex); Here you try to look up the other transmitting device, and use __dev_get_by_index() for that - but we don't hold any relevant lock here! This is (only) called from nl80211_start_ap(), which doesn't hold the RTNL since commit a05829a7222e ("cfg80211: avoid holding the RTNL when calling the driver"): { .cmd = NL80211_CMD_START_AP, .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, .flags = GENL_UNS_ADMIN_PERM, .doit = nl80211_start_ap, - .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | - NL80211_FLAG_NEED_RTNL, + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP, }, I'd fix this, but it's not really trivial - we'd need to use dev_get_by_index() and ensure we dev_put() appropriately, but *only* if it's different from the original dev ... could probably do that in this function. All told though this doesn't make me really very confident you tested this recently, seems like something would've complained here? + if (!tx_dev || !tx_dev->ieee80211_ptr || + tx_dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP) + return -EINVAL; Btw, this really should also check tx_dev->ieee80211_ptr->wiphy == wiphy because we don't want to have a transmitting dev that's for a different wiphy. + } else if (config->index && + !tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]) { + return -EINVAL; + } + + config->tx_dev = tx_dev->ieee80211_ptr; + config->ema = nla_get_flag(tb[NL80211_MBSSID_CONFIG_ATTR_EMA]); + if (config->ema) { + if (!wiphy->ema_max_profile_periodicity) + return -EOPNOTSUPP; + + if (num_elems > wiphy->ema_max_profile_periodicity) + return -EINVAL; + } + + return 0; +} and given the structure of this code it's hard to dev_put() in the right place. + +static struct cfg80211_mbssid_elems * +nl80211_parse_mbssid_elems(struct wiphy *wiphy, struct nlattr *attrs) +{ + struct nlattr *nl_elems; + struct cfg80211_mbssid_elems *elems = NULL; + int rem_elems; + u8 i = 0, num_elems = 0; + + if (!wiphy->mbssid_max_interfaces) + return NULL; + + nla_for_each_nested(nl_elems, attrs, rem_elems) + num_elems++; + + elems = kzalloc(struct_size(elems, elem, num_elems), GFP_KERNEL); + if (!elems) + return NULL; This seems like something we might want to distinguish from the unsupported case? + if (attrs[NL80211_ATTR_MBSSID_ELEMS]) { + bcn->mbssid_ies = + nl80211_parse_mbssid_elems(&rdev->wiphy, + attrs[NL80211_ATTR_MBSSID_ELEMS]); + if (!bcn->mbssid_ies) + return -EINVAL; and actually get -ENOMEM here, so maybe don't return pointer/NULL but pointer/ERR_PTR? out: - kfree(params.acl); - + if (!IS_ERR(params.acl)) + kfree(params.acl); + kfree(params.beacon.mbssid_ies); I guess this also gives you a place to dev_put(). johannes
On Thu, 2021-07-15 at 00:07 -0700, Aloka Dixit wrote: > This patchset adds support for multiple BSSID and > enhanced multi-BSSID advertisements for AP mode. > Individual patches describe the changes in this version. How about adding the trivial advertisement to hwsim so we can have some tests in hostapd? johannes
On 2021-08-17 03:35, Johannes Berg wrote: > On Thu, 2021-07-15 at 00:07 -0700, Aloka Dixit wrote: >> This patchset adds support for multiple BSSID and >> enhanced multi-BSSID advertisements for AP mode. >> Individual patches describe the changes in this version. > > How about adding the trivial advertisement to hwsim so we can have some > tests in hostapd? > > johannes Hi Johannes, Yes, I plan to add hwsim advertisement separately once kernel changes are accepted. Will also add hostapd testcases at the same time. Thanks.
On 2021-08-17 03:33, Johannes Berg wrote: > Hi, > > I don't know if this issue was already present before, but it's > certainly due to the locking changes I had made with the RTNL some time > ago... > >> +static int nl80211_parse_mbssid_config(struct wiphy *wiphy, >> + struct net_device *dev, >> + struct nlattr *attrs, >> + struct cfg80211_mbssid_config *config, >> + u8 num_elems) >> +{ >> + struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1]; >> + struct net_device *tx_dev = dev; > > Here tx_dev defaults to the dev, that's fine, it might be the > transmitting interface. > >> + if (tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]) { >> + tx_ifindex = >> + nla_get_u32(tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]); >> + >> + if (!config->index && tx_ifindex != dev->ifindex) >> + return -EINVAL; >> + >> + tx_dev = __dev_get_by_index(wiphy_net(wiphy), tx_ifindex); > > Here you try to look up the other transmitting device, and use > __dev_get_by_index() for that - but we don't hold any relevant lock > here! > > This is (only) called from nl80211_start_ap(), which doesn't hold the > RTNL since commit a05829a7222e ("cfg80211: avoid holding the RTNL when > calling the driver"): > > { > .cmd = NL80211_CMD_START_AP, > .validate = GENL_DONT_VALIDATE_STRICT | > GENL_DONT_VALIDATE_DUMP, > .flags = GENL_UNS_ADMIN_PERM, > .doit = nl80211_start_ap, > - .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | > - NL80211_FLAG_NEED_RTNL, > + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP, > }, > > > I'd fix this, but it's not really trivial - we'd need to use > dev_get_by_index() and ensure we dev_put() appropriately, but *only* if > it's different from the original dev ... could probably do that in this > function. > > All told though this doesn't make me really very confident you tested > this recently, seems like something would've complained here? > I tested a flavored version, testing without that this time. Other instances of calls to __dev_get_by_index() which don't already hold RTNL explicitly call rtnl_lock()/unlock(). Is it okay to do same here? Regarding the reference, I will call dev_hold() before assigning the value to 'tx_dev' pointer if different than the current net_device, and dev_put() after the processing is done. Thanks.
On Tue, 2021-09-14 at 21:00 -0700, Aloka Dixit wrote: > > > > > + if (tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]) { > > > + tx_ifindex = > > > + nla_get_u32(tb[NL80211_MBSSID_CONFIG_ATTR_TRANSMITTING_IFINDEX]); > > > + > > > + if (!config->index && tx_ifindex != dev->ifindex) > > > + return -EINVAL; > > > + > > > + tx_dev = __dev_get_by_index(wiphy_net(wiphy), tx_ifindex); > > > > Here you try to look up the other transmitting device, and use > > __dev_get_by_index() for that - but we don't hold any relevant lock > > here! > > > > This is (only) called from nl80211_start_ap(), which doesn't hold the > > RTNL since commit a05829a7222e ("cfg80211: avoid holding the RTNL when > > calling the driver"): > > > > { > > .cmd = NL80211_CMD_START_AP, > > .validate = GENL_DONT_VALIDATE_STRICT | > > GENL_DONT_VALIDATE_DUMP, > > .flags = GENL_UNS_ADMIN_PERM, > > .doit = nl80211_start_ap, > > - .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | > > - NL80211_FLAG_NEED_RTNL, > > + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP, > > }, > > > > > > I'd fix this, but it's not really trivial - we'd need to use > > dev_get_by_index() and ensure we dev_put() appropriately, but *only* if > > it's different from the original dev ... could probably do that in this > > function. > > > > All told though this doesn't make me really very confident you tested > > this recently, seems like something would've complained here? > > > > I tested a flavored version, testing without that this time. > > Other instances of calls to __dev_get_by_index() which don't already > hold > RTNL explicitly call rtnl_lock()/unlock(). > > Is it okay to do same here? > I don't think so, we're holding other locks, so that would create an ABBA deadlock - sometimes we do take RTNL->wiphy_mutex, but here you'd end up doing the opposite. But why not just use dev_get_by_index()? > Regarding the reference, I will call dev_hold() before assigning the > value > to 'tx_dev' pointer if different than the current net_device, > and dev_put() after the processing is done. Then you also don't need dev_hold(), since that's implied by dev_get_by_index()? An example in nl80211 would be get_vlan(). johannes
On Tue, 2021-09-14 at 20:47 -0700, Aloka Dixit wrote: > On 2021-08-17 03:35, Johannes Berg wrote: > > On Thu, 2021-07-15 at 00:07 -0700, Aloka Dixit wrote: > > > This patchset adds support for multiple BSSID and > > > enhanced multi-BSSID advertisements for AP mode. > > > Individual patches describe the changes in this version. > > > > How about adding the trivial advertisement to hwsim so we can have some > > tests in hostapd? > > > > johannes > > Hi Johannes, > Yes, I plan to add hwsim advertisement separately once kernel > changes are accepted. Will also add hostapd testcases at the > same time. OK, great! But adding it in hwsim is a kernel change, so why not just send that patch together? johannes
On 2021-09-15 03:47, Johannes Berg wrote: > On Tue, 2021-09-14 at 20:47 -0700, Aloka Dixit wrote: >> On 2021-08-17 03:35, Johannes Berg wrote: >> > On Thu, 2021-07-15 at 00:07 -0700, Aloka Dixit wrote: >> > > This patchset adds support for multiple BSSID and >> > > enhanced multi-BSSID advertisements for AP mode. >> > > Individual patches describe the changes in this version. >> > >> > How about adding the trivial advertisement to hwsim so we can have some >> > tests in hostapd? >> > >> > johannes >> >> Hi Johannes, >> Yes, I plan to add hwsim advertisement separately once kernel >> changes are accepted. Will also add hostapd testcases at the >> same time. > > OK, great! But adding it in hwsim is a kernel change, so why not just > send that patch together? > > johannes Hi Johannes, Trivial advertisement will not work because mac80211_hwsim_beacon_tx() does not handle the EMA case which may have more than one beacons transmitted. It will need to be changed to use the newly added ieee80211_beacon_get_template(). Hence I will add a separate change for HWSIM. Thanks.