Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
flat view
From: Neal Cardwell <ncardwell@google.com>
Date: 2021-06-28 17:10:08
Also in:
bpf, linux-kernel-mentees, lkml
Subsystem:
networking [general], networking [tcp], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Neal Cardwell, Linus Torvalds
On Mon, Jun 28, 2021 at 11:39 AM Nguyen Dinh Phi [off-list ref] wrote:
quoted hunk ↗ jump to hunk
icsk_ca_initialized be always set to zero before we examine it in if block, this makes the congestion control module's initialization be called even if the CC module was initialized already. In case the CC module allocates and setups its dynamically allocated private data in its init() function, e.g, CDG, the memory leak may occur. Reported-by: syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com Signed-off-by: Nguyen Dinh Phi <redacted> --- net/ipv4/tcp_input.c | 1 - 1 file changed, 1 deletion(-)diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 7d5e59f688de..855ada2be25e 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c@@ -5922,7 +5922,6 @@ void tcp_init_transfer(struct sock *sk, int bpf_op, struct sk_buff *skb) tp->snd_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk)); tp->snd_cwnd_stamp = tcp_jiffies32; - icsk->icsk_ca_initialized = 0;
If this patch removes that line, then AFAICT the patch should also insert a corresponding: icsk->icsk_ca_initialized = 0; in tcp_ca_openreq_child(), so that any non-zero icsk_ca_initialized value in a listener socket (on which setsockopt(TCP_CONGESTION) was called) is not erroneously inherited by a child socket due to the tcp_create_openreq_child() -> inet_csk_clone_lock() -> sock_copy() call chain. Something like:
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index c48d8336f26d..4d6a76dfa1c4 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c@@ -446,6 +446,7 @@ void tcp_ca_openreq_child(struct sock *sk, conststruct dst_entry *dst)
}
/* If no valid choice made yet, assign current system default ca. */
+ icsk->icsk_ca_initialized = 0;
if (!ca_got_dst &&
(!icsk->icsk_ca_setsockopt ||
!bpf_try_module_get(icsk->icsk_ca_ops, icsk->icsk_ca_ops->owner)))
thanks,
neal