| Message ID | c92e068760b45b13cae7f29107acabf8512c5f3f.1599830705.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [net] tcp: fix syn cookied MPTCP request socket leak | expand |
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
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
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 --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;
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