[PATCH] handshake: set hr_sk field earlier in handshake_req_submit

[PATCH] handshake: set hr_sk field earlier in handshake_req_submit

[Jeff Layton]

Otherwise, the request is always hashed with the socket set to a NULL
pointer.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/handshake/request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I think this is the problem I was hitting earlier. Chuck, feel free to
fold this into the original patch that adds this code.

diff --git a/net/handshake/request.c b/net/handshake/request.c
index 54ba304b5bef..44a097210e59 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -207,6 +207,7 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
 	if (READ_ONCE(hn->hn_pending) >= hn->hn_pending_max)
 		goto out_err;
 
+	req->hr_sk = sk;
 	req->hr_odestruct = sk->sk_destruct;
 	sk->sk_destruct = handshake_sk_destruct;
 	spin_lock(&hn->hn_lock);
@@ -230,7 +231,6 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
 
 	/* Prevent socket release while a handshake request is pending */
 	sock_hold(sk);
-	req->hr_sk = sk;
 
 	trace_handshake_submit(net, req, sk);
 	return 0;
-- 
2.39.2

Re: [PATCH] handshake: set hr_sk field earlier in handshake_req_submit

[Chuck Lever III]

> On Mar 28, 2023, at 2:10 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> Otherwise, the request is always hashed with the socket set to a NULL
> pointer.

Sounds plausible. I will need to check the clean-up paths too.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> net/handshake/request.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I think this is the problem I was hitting earlier. Chuck, feel free to
> fold this into the original patch that adds this code.
> 
> diff --git a/net/handshake/request.c b/net/handshake/request.c
> index 54ba304b5bef..44a097210e59 100644
> --- a/net/handshake/request.c
> +++ b/net/handshake/request.c
> @@ -207,6 +207,7 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
> 	if (READ_ONCE(hn->hn_pending) >= hn->hn_pending_max)
> 		goto out_err;
> 
> +	req->hr_sk = sk;
> 	req->hr_odestruct = sk->sk_destruct;
> 	sk->sk_destruct = handshake_sk_destruct;
> 	spin_lock(&hn->hn_lock);
> @@ -230,7 +231,6 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
> 
> 	/* Prevent socket release while a handshake request is pending */
> 	sock_hold(sk);
> -	req->hr_sk = sk;
> 
> 	trace_handshake_submit(net, req, sk);
> 	return 0;
> -- 
> 2.39.2
> 

--
Chuck Lever

Re: [PATCH] handshake: set hr_sk field earlier in handshake_req_submit

[Jeff Layton]

On Tue, 2023-03-28 at 18:12 +0000, Chuck Lever III wrote:
> 
> > On Mar 28, 2023, at 2:10 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > Otherwise, the request is always hashed with the socket set to a NULL
> > pointer.
> 
> Sounds plausible. I will need to check the clean-up paths too.
> 

You're right about the error paths. The sock refcounting will need to be
fixed up for sure.

With this but fixed on both ends though, it works for me pretty
consistently now. I guess I was just unlucky with the hashing!


> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > net/handshake/request.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > I think this is the problem I was hitting earlier. Chuck, feel free to
> > fold this into the original patch that adds this code.
> > 
> > diff --git a/net/handshake/request.c b/net/handshake/request.c
> > index 54ba304b5bef..44a097210e59 100644
> > --- a/net/handshake/request.c
> > +++ b/net/handshake/request.c
> > @@ -207,6 +207,7 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
> > 	if (READ_ONCE(hn->hn_pending) >= hn->hn_pending_max)
> > 		goto out_err;
> > 
> > +	req->hr_sk = sk;
> > 	req->hr_odestruct = sk->sk_destruct;
> > 	sk->sk_destruct = handshake_sk_destruct;
> > 	spin_lock(&hn->hn_lock);
> > @@ -230,7 +231,6 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
> > 
> > 	/* Prevent socket release while a handshake request is pending */
> > 	sock_hold(sk);
> > -	req->hr_sk = sk;
> > 
> > 	trace_handshake_submit(net, req, sk);
> > 	return 0;
> > -- 
> > 2.39.2
> > 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] handshake: set hr_sk field earlier in handshake_req_submit

[Chuck Lever III]

> On Mar 28, 2023, at 4:29 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2023-03-28 at 18:12 +0000, Chuck Lever III wrote:
>> 
>>> On Mar 28, 2023, at 2:10 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> Otherwise, the request is always hashed with the socket set to a NULL
>>> pointer.
>> 
>> Sounds plausible. I will need to check the clean-up paths too.
> 
> You're right about the error paths. The sock refcounting will need to be
> fixed up for sure.
> 
> With this but fixed on both ends though, it works for me pretty
> consistently now. I guess I was just unlucky with the hashing!

No, you might be the first person to test this code after I
addressed Paolo's reference counting concerns. It was working
fine before I made those changes.


--
Chuck Lever