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