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

[Paolo Abeni <pabeni@redhat.com>]

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...

thanks,

Paolo