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 for Android: free password hash cracker in your pocket

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

Message-ID: <20220603195509.73cf888f@xps-13>
Date:   Fri, 3 Jun 2022 19:55:09 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Stefan Schmidt <stefan@...enfreihafen.org>
Cc:     Alexander Aring <aahringo@...hat.com>,
        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 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.

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

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?

Thanks,
Miquèl