2018-06-27 15:00:32

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH] mwifiex: fix nested rtnl locking on BG_SCAN_STOPPED

Hi Kalle,

> > >
> > > case EVENT_BG_SCAN_STOPPED:
> > > dev_dbg(adapter->dev, "event: BGS_STOPPED\n");
> > > - cfg80211_sched_scan_stopped(priv->wdev.wiphy, 0);
> > > + if (rtnl_is_locked())
> > > + cfg80211_sched_scan_stopped_rtnl(priv-
> > >wdev.wiphy, 0);
> > > + else
> > > + cfg80211_sched_scan_stopped(priv->wdev.wiphy,
> > 0);
> >
> > IMHO checking if a lock is taking is rather ugly and an indication there's a
> > problem with the locking. Instead making an ugly workaround like this I
> > would rather investigate who is holding the rtnl and solve that.
> There can be applications trying to access driver(via cfg80211), holding
> rtnl_lock.
> For example(in our case):
> 1. "iw dev" was called, when BG_SCAN was active.
> 2. NL80211_CMD_GET_INTERFACE requires rtnl_lock to be hold(specified in
> internal_flags)
> 3. cfg80211 will hold rtnl_lock before calling "get_tx_power"(in pre_doit).
> 4. mwifiex will download RF_TX_PWR command to firmware
> 5. firmware will send BG_SCAN_STOPPED event(since BG_SCAN was active).
> 6. mwifiex will call "cfg80211_sched_scan_stopped" causing nested rtnl
> locking.
>
> Please share your comments further.
Let us know your thoughts on above sequence.

Regards,
Ganapathi


2018-06-27 15:22:52

by Kalle Valo

[permalink] [raw]
Subject: Re: [EXT] Re: [PATCH] mwifiex: fix nested rtnl locking on BG_SCAN_STOPPED

Ganapathi Bhat <[email protected]> writes:

> Hi Kalle,
>
>> > >
>> > > case EVENT_BG_SCAN_STOPPED:
>> > > dev_dbg(adapter->dev, "event: BGS_STOPPED\n");
>> > > - cfg80211_sched_scan_stopped(priv->wdev.wiphy, 0);
>> > > + if (rtnl_is_locked())
>> > > + cfg80211_sched_scan_stopped_rtnl(priv-
>> > >wdev.wiphy, 0);
>> > > + else
>> > > + cfg80211_sched_scan_stopped(priv->wdev.wiphy,
>> > 0);
>> >
>> > IMHO checking if a lock is taking is rather ugly and an indication there's a
>> > problem with the locking. Instead making an ugly workaround like this I
>> > would rather investigate who is holding the rtnl and solve that.
>> There can be applications trying to access driver(via cfg80211), holding
>> rtnl_lock.
>> For example(in our case):
>> 1. "iw dev" was called, when BG_SCAN was active.
>> 2. NL80211_CMD_GET_INTERFACE requires rtnl_lock to be hold(specified in
>> internal_flags)
>> 3. cfg80211 will hold rtnl_lock before calling "get_tx_power"(in pre_doit).
>> 4. mwifiex will download RF_TX_PWR command to firmware
>> 5. firmware will send BG_SCAN_STOPPED event(since BG_SCAN was active).
>> 6. mwifiex will call "cfg80211_sched_scan_stopped" causing nested rtnl
>> locking.
>>
>> Please share your comments further.
>
> Let us know your thoughts on above sequence.

Sorry, I don't have time to help with this. Hopefully someone else can.

--
Kalle Valo