Re: [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2022-06-20 at 12:15 +0200, Paolo Abeni wrote:
> On Fri, 2022-06-17 at 17:51 -0700, Mat Martineau wrote:
> > On Fri, 17 Jun 2022, Paolo Abeni wrote:
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 1e182301e58b..db83db1b3c4c 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -1723,6 +1723,56 @@ static void subflow_state_change(struct sock *sk)
> > > 	}
> > > }
> > > 
> > > +void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> > > +{
> > > +	struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
> > > +	struct mptcp_sock *msk, *next, *head = NULL;
> > > +	struct request_sock *req;
> > > +
> > > +	/* build a list of all unaccepted mptcp sockets */
> > > +	spin_lock_bh(&queue->rskq_lock);
> > > +	for (req = queue->rskq_accept_head; req; req = req->dl_next) {
> > > +		struct mptcp_subflow_context *subflow;
> > > +		struct sock *ssk = req->sk;
> > > +		struct mptcp_sock *msk;
> > > +
> > > +		if (!sk_is_mptcp(ssk))
> > > +			continue;
> > > +
> > > +		subflow = mptcp_subflow_ctx(ssk);
> > > +		if (!subflow || !subflow->conn)
> > > +			continue;
> > > +
> > > +		/* skip if already in list */
> > > +		msk = mptcp_sk(subflow->conn);
> > > +		if (msk->dl_next || msk == head)
> > > +			continue;
> > > +
> > > +		msk->dl_next = head;
> > 
> > Why is it ok to read/modify msk->dl_next without the msk locked here, but 
> > the msk lock is needed below?
> 
> the dl_next field is protected from the listener socket lock. Such lock
> is held both here and below, see the caller of this function.
> 

I see the above is not very clear; I mean: this whole function runs
under the listener _msk_ socket lock. The 'dl_next' list is under such
protection.

/P