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
