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...

quoted

+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.

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?

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.