Re: [PATCH mptcp-net] mptcp: fix lockdep false positive

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2022-12-12 at 13:06 +0100, Paolo Abeni wrote:
> Hello,
> 
> Sorry for the extra latency
> On Wed, 2022-12-07 at 17:50 -0800, Mat Martineau wrote:
> > > @@ -1813,8 +1813,18 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> > > 
> > > 		do_cancel_work = __mptcp_close(sk, 0);
> > > 		release_sock(sk);
> > > -		if (do_cancel_work)
> > > +		if (do_cancel_work) {
> > > +			/* lockdep will report a false positive ABBA deadlock between
> > > +			 * cancel_work_sync and the listener socket. The involved locks
> > > +			 * belong to differen sockets WRT the existing AB chain.
> > 
> > s/differen/different/
> > 
> > > +			 * Using a per socket key would be very invasive and heavy, just
> > > +			 * tell lockdep to consider the listener socket released here
> > > +			 */
> > > +			mutex_release(&listener_sk->sk_lock.dep_map, _RET_IP_);
> > > 			mptcp_cancel_work(sk);
> > > +			mutex_acquire(&listener_sk->sk_lock.dep_map,
> > > +				      SINGLE_DEPTH_NESTING, 0, _RET_IP_);
> > 
> > Any concerns about this change breaking lockdep if the existing 
> > assumptions about listener sockets no longer apply? For example, if 
> > someone made the unfortunate choice to start using mptcp_worker() with a 
> > MPTCP listener sock?
> 
> I'm not sure I read the above correctly. This change does not bring
> extra assumption about listener sockets. Starting the worker on the
> listener socket will not impact the above annotation: the listener
> socker will try to catch/shutdown/cancel the releated worker in the
> related mptcp_close() call.
> 
> I'm unsure I explained the current problem in a clear way.
> 
> Here we have 2 msk sockets ('listener_sk' and 'sk'). The above code is
> invoked under the listener sk socket lock.  
> 
> Lockdep see:
> 
> workqueue(sk) waits for lock(sk)
> 
> lock(listener_sk) waits for workqueue(sk)
> 
> and barks loudly since listener_sk and sk have the same 'key' - are
> basically the same thing from lockdep perspective. A 'cleaner' fix
> would be setting different lockdep key either on the mptcp workqueue or
> on the socket. I refrained from that since it would be quite more
> invasive (dynamic lockdep key allocation /cleanup).
> 
> Let me check if it is as painful as I thought...

It looks like that alternative is not feasible at all: we will need to
de-register the per msk lockdep key at sk free time, sk_destructor is
(/can be) called in atomic context and lockdep_unregister_key()
requires process context. 

I think this is the better option we have.

Cheers,

Paolo