netdev - Re: [PATCH net v3] net: mptcp: fix unreleased socket in accept queue

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

Date:   Thu, 08 Sep 2022 16:44:58 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Matthieu Baerts <matthieu.baerts@...sares.net>,
        menglong8.dong@...il.com
Cc:     mathew.j.martineau@...ux.intel.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, fw@...len.de,
        peter.krystad@...ux.intel.com, netdev@...r.kernel.org,
        mptcp@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Menglong Dong <imagedong@...cent.com>,
        Jiang Biao <benbjiang@...cent.com>,
        Mengen Sun <mengensun@...cent.com>
Subject: Re: [PATCH net v3] net: mptcp: fix unreleased socket in accept queue

On Thu, 2022-09-08 at 15:56 +0200, Matthieu Baerts wrote:
> Hi Menglong,
> 
> On 07/09/2022 13:11, menglong8.dong@...il.com wrote:
> > From: Menglong Dong <imagedong@...cent.com>
> > 
> > The mptcp socket and its subflow sockets in accept queue can't be
> > released after the process exit.
> > 
> > While the release of a mptcp socket in listening state, the
> > corresponding tcp socket will be released too. Meanwhile, the tcp
> > socket in the unaccept queue will be released too. However, only init
> > subflow is in the unaccept queue, and the joined subflow is not in the
> > unaccept queue, which makes the joined subflow won't be released, and
> > therefore the corresponding unaccepted mptcp socket will not be released
> > to.
> 
> Thank you for the v3.
> 
> Unfortunately, our CI found a possible recursive locking:
> 
> > - KVM Validation: debug:
> >   - Unstable: 1 failed test(s): selftest_mptcp_join - Critical: 1 Call Trace(s) ❌:
> >   - Task: https://cirrus-ci.com/task/5418283233968128
> >   - Summary: https://api.cirrus-ci.com/v1/artifact/task/5418283233968128/summary/summary.txt
> 
> https://lore.kernel.org/mptcp/4e6d3d9e-1f1a-23ae-cb56-2d4f043f17ae@gmail.com/T/#u
> 
> Do you mind looking at it please?

Ah, that is actually a false positive, but we must silence it. The main
point is that the lock_sock() in mptcp_close() rightfully lacks the
_nested annotation.

Instead of adding such annotation only for this call site, which would
be both ugly and dangerous, I suggest to factor_out from mptcp_close()
all the code the run under the socket lock, say in:
 
bool __mptcp_close(struct sock *sk, long timeout)
	// return true if the caller need to cancel the mptcp worker
	// (outside the socket lock)

and then in mptcp_subflow_queue_clean():

	sock_hold(sk);

	slow = lock_sock_fast_nested(sk);
        next = msk->dl_next;
        msk->first = NULL;
        msk->dl_next = NULL;
	do_cancel_work = __mptcp_close(sk, 0);
	unlock_sock_fast(sk, slow);

	if (do_cancel_work)
		mptcp_cancel_work(sk);
	sock_put(sk);

All the above could require 2 different patches, 1 to factor-out the
helper, and 1 to actually implement the fix.

Cheers,

Paolo