On Tue, 2021-09-14 at 21:00 -0700, Aloka Dixit wrote:

quoted

quoted

+	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