Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Kuniyuki Iwashima <kuniyu@amazon.co.jp>
To:     <kafai@fb.com>
CC:     <andrii@kernel.org>, <ast@kernel.org>, <benh@amazon.com>,
        <bpf@vger.kernel.org>, <daniel@iogearbox.net>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <kuni1840@gmail.com>, <kuniyu@amazon.co.jp>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>
Subject: Re: [PATCH v5 bpf-next 07/11] tcp: Migrate TCP_NEW_SYN_RECV requests at receiving the final ACK.
Date:   Sat, 15 May 2021 13:18:34 +0900
Message-ID: <20210515041834.81591-1-kuniyu@amazon.co.jp>
In-Reply-To: <20210515011305.eeblnqnov4xlcjfy@kafai-mbp>
References: <20210515011305.eeblnqnov4xlcjfy@kafai-mbp>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

From:   Martin KaFai Lau <kafai@fb.com>
Date:   Fri, 14 May 2021 18:13:05 -0700
> On Mon, May 10, 2021 at 12:44:29PM +0900, Kuniyuki Iwashima wrote:
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index e690d1cff36e..fe666dc5c621 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -1075,10 +1075,38 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child,
> >  	if (own_req) {
> >  		inet_csk_reqsk_queue_drop(sk, req);
> >  		reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req);
> In the migration case 'sk != req->rsk_listener', is sk the right
> one to pass in the above two functions?

Good catch, 'sk' should be 'req->rsk_listener' here.
Thank you!


> 
> > -		if (inet_csk_reqsk_queue_add(sk, req, child))
> > +
> > +		if (sk != req->rsk_listener) {
> > +			/* another listening sk has been selected,
> > +			 * migrate the req to it.
> > +			 */
> > +			struct request_sock *nreq;
> > +
> > +			/* hold a refcnt for the nreq->rsk_listener
> > +			 * which is assigned in reqsk_clone()
> > +			 */
> > +			sock_hold(sk);
> > +			nreq = reqsk_clone(req, sk);
> > +			if (!nreq) {
> > +				inet_child_forget(sk, req, child);
> > +				goto child_put;
> > +			}
> > +
> > +			refcount_set(&nreq->rsk_refcnt, 1);
> > +			if (inet_csk_reqsk_queue_add(sk, nreq, child)) {
> > +				reqsk_migrate_reset(req);
> > +				reqsk_put(req);
> > +				return child;
> > +			}
> > +
> > +			reqsk_migrate_reset(nreq);
> > +			__reqsk_free(nreq);
> > +		} else if (inet_csk_reqsk_queue_add(sk, req, child)) {
> >  			return child;
> > +		}
> >  	}
> >  	/* Too bad, another child took ownership of the request, undo. */
> > +child_put:
> >  	bh_unlock_sock(child);
> >  	sock_put(child);
> >  	return NULL;