Re: [PATCH v11 1/4] nl80211: MBSSID and EMA support in AP mode

[Aloka Dixit <alokad@codeaurora.org>]

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.