| Message ID | d0fac98f696008ec10ec2d8ca5f82a4b32f5e6d7.1553007150.git.gnault@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] tcp: free request sock directly upon TFO or syncookies error | expand |
On 03/19/2019 08:05 AM, Guillaume Nault wrote: > Since the request socket is created locally, it'd make more sense to > use reqsk_free() instead of reqsk_put() in TFO and syncookies' error > path. > > However, tcp_get_cookie_sock() may set ->rsk_refcnt before freeing the > socket; tcp_conn_request() may also have non-null ->rsk_refcnt because > of tcp_try_fastopen(). In both cases 'req' hasn't been exposed > to the outside world and is safe to free immediately, but that'd > trigger the WARN_ON_ONCE in reqsk_free(). > > Define __reqsk_free() for these situations where we know nobody's > referencing the socket, even though ->rsk_refcnt might be non-null. > Now we can consolidate the error path of tcp_get_cookie_sock() and > tcp_conn_request(). > > Signed-off-by: Guillaume Nault <gnault@redhat.com> SGTM thanks Signed-off-by: Eric Dumazet <edumazet@google.com>
From: Guillaume Nault <gnault@redhat.com> Date: Tue, 19 Mar 2019 16:05:44 +0100 > Since the request socket is created locally, it'd make more sense to > use reqsk_free() instead of reqsk_put() in TFO and syncookies' error > path. > > However, tcp_get_cookie_sock() may set ->rsk_refcnt before freeing the > socket; tcp_conn_request() may also have non-null ->rsk_refcnt because > of tcp_try_fastopen(). In both cases 'req' hasn't been exposed > to the outside world and is safe to free immediately, but that'd > trigger the WARN_ON_ONCE in reqsk_free(). > > Define __reqsk_free() for these situations where we know nobody's > referencing the socket, even though ->rsk_refcnt might be non-null. > Now we can consolidate the error path of tcp_get_cookie_sock() and > tcp_conn_request(). > > Signed-off-by: Guillaume Nault <gnault@redhat.com> Applied.
diff --git a/include/net/request_sock.h b/include/net/request_sock.h index 21a5243fecd1..9dfd7960d90a 100644 --- a/include/net/request_sock.h +++ b/include/net/request_sock.h @@ -106,10 +106,8 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener, return req; } -static inline void reqsk_free(struct request_sock *req) +static inline void __reqsk_free(struct request_sock *req) { - WARN_ON_ONCE(refcount_read(&req->rsk_refcnt) != 0); - req->rsk_ops->destructor(req); if (req->rsk_listener) sock_put(req->rsk_listener); @@ -117,6 +115,12 @@ static inline void reqsk_free(struct request_sock *req) kmem_cache_free(req->rsk_ops->slab, req); } +static inline void reqsk_free(struct request_sock *req) +{ + WARN_ON_ONCE(refcount_read(&req->rsk_refcnt) != 0); + __reqsk_free(req); +} + static inline void reqsk_put(struct request_sock *req) { if (refcount_dec_and_test(&req->rsk_refcnt)) diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index e531344611a0..008545f63667 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -216,16 +216,15 @@ 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); - if (!inet_csk_reqsk_queue_add(sk, req, child)) { - bh_unlock_sock(child); - sock_put(child); - child = NULL; - reqsk_put(req); - } - } else { - reqsk_free(req); + if (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); diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 5def3c48870e..5dfbc333e79a 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -6502,8 +6502,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, reqsk_fastopen_remove(fastopen_sk, req, false); bh_unlock_sock(fastopen_sk); sock_put(fastopen_sk); - reqsk_put(req); - goto drop; + goto drop_and_free; } sk->sk_data_ready(sk); bh_unlock_sock(fastopen_sk); @@ -6527,7 +6526,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, drop_and_release: dst_release(dst); drop_and_free: - reqsk_free(req); + __reqsk_free(req); drop: tcp_listendrop(sk); return 0;
Since the request socket is created locally, it'd make more sense to use reqsk_free() instead of reqsk_put() in TFO and syncookies' error path. However, tcp_get_cookie_sock() may set ->rsk_refcnt before freeing the socket; tcp_conn_request() may also have non-null ->rsk_refcnt because of tcp_try_fastopen(). In both cases 'req' hasn't been exposed to the outside world and is safe to free immediately, but that'd trigger the WARN_ON_ONCE in reqsk_free(). Define __reqsk_free() for these situations where we know nobody's referencing the socket, even though ->rsk_refcnt might be non-null. Now we can consolidate the error path of tcp_get_cookie_sock() and tcp_conn_request(). Signed-off-by: Guillaume Nault <gnault@redhat.com> --- include/net/request_sock.h | 10 +++++++--- net/ipv4/syncookies.c | 17 ++++++++--------- net/ipv4/tcp_input.c | 5 ++--- 3 files changed, 17 insertions(+), 15 deletions(-)