netdev - Re: [PATCH wpan-next v4 00/11] ieee802154: Synchronous Tx support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK-6q+if-dNbpbneTfUtj6MrZXiYPq9npZfMkatXKo8cfU1m9w@mail.gmail.com>
Date:   Fri, 3 Jun 2022 21:50:15 -0400
From:   Alexander Aring <aahringo@...hat.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Stefan Schmidt <stefan@...enfreihafen.org>,
        Alexander Aring <alex.aring@...il.com>,
        linux-wpan - ML <linux-wpan@...r.kernel.org>,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH wpan-next v4 00/11] ieee802154: Synchronous Tx support

Hi,

On Fri, Jun 3, 2022 at 1:55 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Stefan, Alex,
>
> stefan@...enfreihafen.org wrote on Wed, 1 Jun 2022 23:01:51 +0200:
>
> > Hello.
> >
> > On 01.06.22 05:30, Alexander Aring wrote:
> > > Hi,
> > >
> > > On Thu, May 19, 2022 at 11:06 AM Miquel Raynal
> > > <miquel.raynal@...tlin.com> wrote:
> > >>
> > >> Hello,
> > >>
> > >> This series brings support for that famous synchronous Tx API for MLME
> > >> commands.
> > >>
> > >> MLME commands will be used during scan operations. In this situation,
> > >> we need to be sure that all transfers finished and that no transfer
> > >> will be queued for a short moment.
> > >>
> > >
> > > Acked-by: Alexander Aring <aahringo@...hat.com>
> >
> > These patches have been applied to the wpan-next tree. Thanks!
> >
> > > There will be now functions upstream which will never be used, Stefan
> > > should wait until they are getting used before sending it to net-next.
> >
> > Indeed this can wait until we have a consumer of the functions before pushing this forward to net-next. Pretty sure Miquel is happy to finally move on to other pieces of his puzzle and use them. :-)
>
> Next part is coming!
>
> In the mean time I've experienced a new lockdep warning:
>
> All the netlink commands are executed with the rtnl taken.
> In my current implementation, when I configure/edit a scan request or a
> beacon request I take a scan_lock or a beacons_lock, so they may only
> be taken after the rtnl in this case, which leads to this sequence of
> events:
> - the rtnl is taken (by the net core)
> - the beacon's lock is taken
>
> But now in a beacon's work or an active scan work, what happens is:
> - work gets woken up
> - the beacon/scan lock is taken
> - a beacon/beacon-request frame is transmitted
> - the rtnl lock is taken during this transmission
>
> Lockdep then detects a possible circular dependency:
> [  490.153387]        CPU0                    CPU1
> [  490.153391]        ----                    ----
> [  490.153394]   lock(&local->beacons_lock);
> [  490.153400]                                lock(rtnl_mutex);
> [  490.153406]                                lock(&local->beacons_lock);
> [  490.153412]   lock(rtnl_mutex);
>
> So in practice, I always need to have the rtnl lock taken when
> acquiring these other locks (beacon/scan_lock) which I think is far
> from optimal.
>

*Note that those can also be false positives.

> 1# One solution is to drop the beacons/scan locks because they are not
> useful anymore and simply rely on the rtnl.
>

depends on how long it will be held.

> 2# Another solution would be to change the mlme_tx() implementation to
> finally not need the rtnl at all.
>
> Note that just calling ASSERT_RTNL() makes no difference in 2#, it
> still means that I always need to acquire the rtnl before acquiring the
> beacons/scan locks, which greatly reduces their usefulness and leads to
> solution 1# in the end.
>
> IIRC I decided to introduce the rtnl to avoid ->ndo_stop() calls during
> an MLME transmission. I don't know if it has another use there. If not,
> we may perhaps get rid of the rtnl in mlme_tx() by really handling the
> stop calls (but I was too lazy so far to do that).
>
> What direction would you advise?

Hard to say without code. Please show us some code of the current
state... there should also be some stacktrace of the circular lock
dependency, please provide the full output _matching_ the provided
code.

Thanks.

- Alex