On 03/08/2019 01:09 PM, Guillaume Nault wrote:
> Commit 7716682cc58e ("tcp/dccp: fix another race at listener
> dismantle") let inet_csk_reqsk_queue_add() fail, and adjusted
> {tcp,dccp}_check_req() accordingly. However, TFO and syncookies
> weren't modified, thus leaking allocated resources on error.
>
> Contrary to tcp_check_req(), in both syncookies and TFO cases,
> we need to drop the request socket. Also, since the child socket is
> created with inet_csk_clone_lock(), we have to unlock it and drop an
> extra reference (->sk_refcount is initially set to 2 and
> inet_csk_reqsk_queue_add() drops only one ref).
>
> For TFO, we also need to revert the work done by tcp_try_fastopen()
> (with reqsk_fastopen_remove()).
>
> Fixes: 7716682cc58e ("tcp/dccp: fix another race at listener dismantle")
> Signed-off-by: Guillaume Nault <gna...@redhat.com>
> ---
>
> Note for stable backports: this patch relies on da8ab57863ed
> ("tcp/dccp: remove reqsk_put() from inet_child_forget()"), to prevent
> inet_child_forget() from dropping a reference from the request socket.
>
> Therefore, for trees older than 4.14, commit da8ab57863ed has to be
> backported before this patch.
>
Thanks for working on this issue (it was on my radar as well)
>
> net/ipv4/syncookies.c | 7 ++++++-
> net/ipv4/tcp_input.c | 8 +++++++-
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 606f868d9f3f..e531344611a0 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -216,7 +216,12 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct
> sk_buff *skb,
> refcount_set(&req->rsk_refcnt, 1);
> tcp_sk(child)->tsoffset = tsoff;
> sock_rps_save_rxhash(child, skb);
> - inet_csk_reqsk_queue_add(sk, req, child);
> + if (!inet_csk_reqsk_queue_add(sk, req, child)) {
> + bh_unlock_sock(child);
> + sock_put(child);
> + child = NULL;
> + reqsk_put(req);
Since we use reqsk_free(req) in the same function, we can use reqsk_free(req)
here as well ?
I suggest the following maybe :
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index
606f868d9f3fde1c3140aa7eecde87d2ec32b5f2..8b28fb66a8fcefba27a2f5e371e9469d4d7e3650
100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -216,11 +216,14 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct
sk_buff *skb,
refcount_set(&req->rsk_refcnt, 1);
tcp_sk(child)->tsoffset = tsoff;
sock_rps_save_rxhash(child, skb);
- inet_csk_reqsk_queue_add(sk, req, child);
- } else {
- reqsk_free(req);
+ if (likely(inet_csk_reqsk_queue_add(sk, req, child)))
+ return child;
+ bh_unlock_sock(child);
+ sock_put(child);
}
- return child;
+
+ reqsk_free(req);
+ return NULL;
}
EXPORT_SYMBOL(tcp_get_cookie_sock);
> + }
> } else {
> reqsk_free(req);
> }
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 4eb0c8ca3c60..5def3c48870e 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6498,7 +6498,13 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> af_ops->send_synack(fastopen_sk, dst, &fl, req,
> &foc, TCP_SYNACK_FASTOPEN);
> /* Add the child socket directly into the accept queue */
> - inet_csk_reqsk_queue_add(sk, req, fastopen_sk);
> + if (!inet_csk_reqsk_queue_add(sk, req, fastopen_sk)) {
> + reqsk_fastopen_remove(fastopen_sk, req, false);
> + bh_unlock_sock(fastopen_sk);
> + sock_put(fastopen_sk);
> + reqsk_put(req);
> + goto drop;
These two lines can be replaced by :
goto drop_and_free;
> + }
> sk->sk_data_ready(sk);
> bh_unlock_sock(fastopen_sk);
> sock_put(fastopen_sk);
>
