Hi Alexander,
aahringo@xxxxxxxxxx wrote on Tue, 5 Jul 2022 21:23:21 -0400:
> Hi,
>
> On Fri, Jul 1, 2022 at 10:37 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >
> > There is no need to ensure the rtnl is locked when changing a driver's
> > channel. This cause issues when scanning and this is the only driver
> > relying on it. Just drop this dependency because it does not seem
> > legitimate.
> >
>
> switching channels relies on changing pib attributes, pib attributes
> are protected by rtnl. If you experience issues here then it's
> probably because you do something wrong. All drivers assuming here
> that rtnl lock is held.
---8<---
> especially this change could end in invalid free. Maybe we can solve
> this problem in a different way, what exactly is the problem by
> helding rtnl lock?
--->8---
During a scan we need to change channels. So when the background job
kicks-in, we first acquire scan_lock, then we check the internal
parameters of the structure protected by this lock, like the next
channel to use and the sdata pointer. A channel change must be
performed, preceded by an rtnl_lock(). This will again trigger a
possible circular lockdep dependency warning because the triggering path
acquires the rtnl (as part of the netlink layer) before the scan lock.
One possible solution would be to do the following:
scan_work() {
acquire(scan_lock);
// do some config
release(scan_lock);
rtnl_lock();
perform_channel_change();
rtnl_unlock();
acquire(scan_lock);
// reinit the scan struct ptr and the sdata ptr
// do some more things
release(scan_lock);
}
It looks highly non-elegant IMHO. Otherwise I need to stop verifying in
the drivers that the rtnl is taken. Any third option here?
BTW I don't see where the invalid free situation you mentioned can
happen here, can you explain?
Thanks,
Miquèl
