diff mbox series

[net] tcp: fix syn cookied MPTCP request socket leak

Message ID c92e068760b45b13cae7f29107acabf8512c5f3f.1599830705.git.pabeni@redhat.com
State Changes Requested
Headers show
Series [net] tcp: fix syn cookied MPTCP request socket leak | expand

Commit Message

Paolo Abeni Sept. 11, 2020, 1:38 p.m. UTC
If a syn-cookies request socket don't pass MPTCP-level
validation done in syn_recv_sock(), we need to release
it immediatelly, or it will be leaked.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/89
Fixes: 9466a1ccebbe ("mptcp: enable JOIN requests even if cookies are in use")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/syncookies.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
---
I don't really know what I'm doing here ;) 
But apparently it fixes the leak in my tests

Comments

Geliang Tang Sept. 14, 2020, 9:34 a.m. UTC | #1
Hi Paolo,

Thanks for your patch. I tested it. It fixed the memory leak problem.
But I got this
selftest fail output:

11 single subflow with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
12 multiple subflows with syn cookies   syn[fail] got 1 JOIN[s] syn expected 2
 - synack[fail] got 1 JOIN[s] synack expected 2
 - ack[fail] got 1 JOIN[s] ack expected 2
Server ns stats
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             1                  0.0
MPTcpExtMPJoinAckRx             1                  0.0
Client ns stats
MPTcpExtMPJoinSynAckRx          1                  0.0
13 subflows limited by server w cookies syn[fail] got 1 JOIN[s] syn expected 2
 - synack[fail] got 1 JOIN[s] synack expected 2
 - ack[ ok ]
Server ns stats
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             1                  0.0
MPTcpExtMPJoinAckRx             1                  0.0
Client ns stats
MPTcpExtMPJoinSynAckRx          1                  0.0
14 signal address with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
15 subflow and signal w cookies         syn[ ok ] - synack[ ok ] - ack[ ok ]
16 subflows and signal w. cookies       syn[fail] got 1 JOIN[s] syn expected 3
 - synack[fail] got 1 JOIN[s] synack expected 3
 - ack[fail] got 1 JOIN[s] ack expected 3
Server ns stats
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             1                  0.0
MPTcpExtMPJoinAckRx             1                  0.0
Client ns stats
MPTcpExtMPJoinSynAckRx          1                  0.0

-Geliang

Paolo Abeni <pabeni@redhat.com> 于2020年9月11日周五 下午9:38写道:
>
> If a syn-cookies request socket don't pass MPTCP-level
> validation done in syn_recv_sock(), we need to release
> it immediatelly, or it will be leaked.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/89
> Fixes: 9466a1ccebbe ("mptcp: enable JOIN requests even if cookies are in use")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/ipv4/syncookies.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> ---
> I don't really know what I'm doing here ;)
> But apparently it fixes the leak in my tests
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index c375c126f436..5c8390876cf8 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -209,15 +209,15 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
>         child = icsk->icsk_af_ops->syn_recv_sock(sk, skb, req, dst,
>                                                  NULL, &own_req);
>         if (child) {
> -               refcount_set(&req->rsk_refcnt, 1);
> -               tcp_sk(child)->tsoffset = tsoff;
> -               sock_rps_save_rxhash(child, skb);
> -
>                 if (rsk_drop_req(req)) {
> -                       refcount_set(&req->rsk_refcnt, 2);
> +                       reqsk_free(req);
>                         return child;
>                 }
>
> +               refcount_set(&req->rsk_refcnt, 1);
> +               tcp_sk(child)->tsoffset = tsoff;
> +               sock_rps_save_rxhash(child, skb);
> +
>                 if (inet_csk_reqsk_queue_add(sk, req, child))
>                         return child;
>
> --
> 2.26.2
> _______________________________________________
> mptcp mailing list -- mptcp@lists.01.org
> To unsubscribe send an email to mptcp-leave@lists.01.org
Paolo Abeni Sept. 14, 2020, 1:54 p.m. UTC | #2
On Mon, 2020-09-14 at 17:34 +0800, Geliang Tang wrote:
> Hi Paolo,
> 
> Thanks for your patch. I tested it. It fixed the memory leak problem.
> But I got this
> selftest fail output:
> 
> 11 single subflow with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
> 12 multiple subflows with syn cookies   syn[fail] got 1 JOIN[s] syn expected 2
>  - synack[fail] got 1 JOIN[s] synack expected 2
>  - ack[fail] got 1 JOIN[s] ack expected 2
> Server ns stats
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             1                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> Client ns stats
> MPTcpExtMPJoinSynAckRx          1                  0.0
> 13 subflows limited by server w cookies syn[fail] got 1 JOIN[s] syn expected 2
>  - synack[fail] got 1 JOIN[s] synack expected 2
>  - ack[ ok ]
> Server ns stats
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             1                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> Client ns stats
> MPTcpExtMPJoinSynAckRx          1                  0.0
> 14 signal address with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
> 15 subflow and signal w cookies         syn[ ok ] - synack[ ok ] - ack[ ok ]
> 16 subflows and signal w. cookies       syn[fail] got 1 JOIN[s] syn expected 3
>  - synack[fail] got 1 JOIN[s] synack expected 3
>  - ack[fail] got 1 JOIN[s] ack expected 3
> Server ns stats
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             1                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> Client ns stats
> MPTcpExtMPJoinSynAckRx          1                  0.0

I sometime see similar errors, but they are quite unfrequent, and looks
tied to the self-test fragility: we use some hard-coded sleep to allow
for MPJ/ADD_ADDR handshake. If the VM running the test is my change
slowed down a bit, self-tests will fail.

Is the above failure reproducible for you?

Thanks,

Paolo
Geliang Tang Sept. 15, 2020, 3:03 a.m. UTC | #3
Hi Paolo,

Paolo Abeni <pabeni@redhat.com> 于2020年9月14日周一 下午9:54写道:
>
> On Mon, 2020-09-14 at 17:34 +0800, Geliang Tang wrote:
> > Hi Paolo,
> >
> > Thanks for your patch. I tested it. It fixed the memory leak problem.
> > But I got this
> > selftest fail output:
> >
> > 11 single subflow with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
> > 12 multiple subflows with syn cookies   syn[fail] got 1 JOIN[s] syn expected 2
> >  - synack[fail] got 1 JOIN[s] synack expected 2
> >  - ack[fail] got 1 JOIN[s] ack expected 2
> > Server ns stats
> > MPTcpExtMPCapableSYNRX          1                  0.0
> > MPTcpExtMPCapableACKRX          1                  0.0
> > MPTcpExtMPJoinSynRx             1                  0.0
> > MPTcpExtMPJoinAckRx             1                  0.0
> > Client ns stats
> > MPTcpExtMPJoinSynAckRx          1                  0.0
> > 13 subflows limited by server w cookies syn[fail] got 1 JOIN[s] syn expected 2
> >  - synack[fail] got 1 JOIN[s] synack expected 2
> >  - ack[ ok ]
> > Server ns stats
> > MPTcpExtMPCapableSYNRX          1                  0.0
> > MPTcpExtMPCapableACKRX          1                  0.0
> > MPTcpExtMPJoinSynRx             1                  0.0
> > MPTcpExtMPJoinAckRx             1                  0.0
> > Client ns stats
> > MPTcpExtMPJoinSynAckRx          1                  0.0
> > 14 signal address with syn cookies      syn[ ok ] - synack[ ok ] - ack[ ok ]
> > 15 subflow and signal w cookies         syn[ ok ] - synack[ ok ] - ack[ ok ]
> > 16 subflows and signal w. cookies       syn[fail] got 1 JOIN[s] syn expected 3
> >  - synack[fail] got 1 JOIN[s] synack expected 3
> >  - ack[fail] got 1 JOIN[s] ack expected 3
> > Server ns stats
> > MPTcpExtMPCapableSYNRX          1                  0.0
> > MPTcpExtMPCapableACKRX          1                  0.0
> > MPTcpExtMPJoinSynRx             1                  0.0
> > MPTcpExtMPJoinAckRx             1                  0.0
> > Client ns stats
> > MPTcpExtMPJoinSynAckRx          1                  0.0
>
> I sometime see similar errors, but they are quite unfrequent, and looks
> tied to the self-test fragility: we use some hard-coded sleep to allow
> for MPJ/ADD_ADDR handshake. If the VM running the test is my change
> slowed down a bit, self-tests will fail.
>
> Is the above failure reproducible for you?

Yes, I got this self-test failure every time both on -net and linux-next.

-Geliang

>
> Thanks,
>
> Paolo
>
diff mbox series

Patch

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index c375c126f436..5c8390876cf8 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -209,15 +209,15 @@  struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 	child = icsk->icsk_af_ops->syn_recv_sock(sk, skb, req, dst,
 						 NULL, &own_req);
 	if (child) {
-		refcount_set(&req->rsk_refcnt, 1);
-		tcp_sk(child)->tsoffset = tsoff;
-		sock_rps_save_rxhash(child, skb);
-
 		if (rsk_drop_req(req)) {
-			refcount_set(&req->rsk_refcnt, 2);
+			reqsk_free(req);
 			return child;
 		}
 
+		refcount_set(&req->rsk_refcnt, 1);
+		tcp_sk(child)->tsoffset = tsoff;
+		sock_rps_save_rxhash(child, skb);
+
 		if (inet_csk_reqsk_queue_add(sk, req, child))
 			return child;