| Message ID | 20210702194033.1370634-1-phind.uet@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: yhs@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
On Fri, Jul 2, 2021 at 3:41 PM Nguyen Dinh Phi <phind.uet@gmail.com> wrote: > > This commit fixes a bug (found by syzkaller) that could cause spurious > double-initializations for congestion control modules, which could cause > memory leaks orother problems for congestion control modules (like CDG) > that allocate memory in their init functions. > > The buggy scenario constructed by syzkaller was something like: > > (1) create a TCP socket > (2) initiate a TFO connect via sendto() > (3) while socket is in TCP_SYN_SENT, call setsockopt(TCP_CONGESTION), > which calls: > tcp_set_congestion_control() -> > tcp_reinit_congestion_control() -> > tcp_init_congestion_control() > (4) receive ACK, connection is established, call tcp_init_transfer(), > set icsk_ca_initialized=0 (without first calling cc->release()), > call tcp_init_congestion_control() again. > > Note that in this sequence tcp_init_congestion_control() is called > twice without a cc->release() call in between. Thus, for CC modules > that allocate memory in their init() function, e.g, CDG, a memory leak > may occur. The syzkaller tool managed to find a reproducer that > triggered such a leak in CDG. > > The bug was introduced when that commit 8919a9b31eb4 ("tcp: Only init > congestion control if not initialized already") > introduced icsk_ca_initialized and set icsk_ca_initialized to 0 in > tcp_init_transfer(), missing the possibility for a sequence like the > one above, where a process could call setsockopt(TCP_CONGESTION) in > state TCP_SYN_SENT (i.e. after the connect() or TFO open sendmsg()), > which would call tcp_init_congestion_control(). It did not intend to > reset any initialization that the user had already explicitly made; > it just missed the possibility of that particular sequence (which > syzkaller managed to find). > > Fixes: commit 8919a9b31eb4 ("tcp: Only init congestion control if not > initialized already") Please note that the patchwork tools have found a style/formatting issue with your Fixes tag: You can find them at: https://patchwork.kernel.org/project/netdevbpf/list/ -> https://patchwork.kernel.org/project/netdevbpf/patch/20210702194033.1370634-1-phind.uet@gmail.com/ -> https://patchwork.hopto.org/static/nipa/510221/12356435/verify_fixes/stdout The error is: --- Fixes tag: Fixes: commit 8919a9b31eb4 ("tcp: Only init congestion control if not Has these problem(s): - leading word 'commit' unexpected - Subject has leading but no trailing parentheses - Subject has leading but no trailing quotes --- Basically, please omit the "commit" and don't wrap the text (it's OK if it's longer than 80 or 100 characters). thanks, neal
On 7/3/2021 4:31 AM, Neal Cardwell wrote: > Please note that the patchwork tools have found a style/formatting > issue with your Fixes tag: > > You can find them at: > https://patchwork.kernel.org/project/netdevbpf/list/ > -> > https://patchwork.kernel.org/project/netdevbpf/patch/20210702194033.1370634-1-phind.uet@gmail.com/ > -> > https://patchwork.hopto.org/static/nipa/510221/12356435/verify_fixes/stdout > > The error is: > --- > Fixes tag: Fixes: commit 8919a9b31eb4 ("tcp: Only init congestion control if not > Has these problem(s): > - leading word 'commit' unexpected > - Subject has leading but no trailing parentheses > - Subject has leading but no trailing quotes > --- > > Basically, please omit the "commit" and don't wrap the text (it's OK > if it's longer than 80 or 100 characters). > > thanks, > neal > Hi, I didn't even know there are patchwork tools for checking formatting. Thank you for pointing out, I have just submitted a new version. Best regards, Phi.
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; bpf_skops_established(sk, bpf_op, skb); if (!icsk->icsk_ca_initialized) tcp_init_congestion_control(sk);
This commit fixes a bug (found by syzkaller) that could cause spurious double-initializations for congestion control modules, which could cause memory leaks orother problems for congestion control modules (like CDG) that allocate memory in their init functions. The buggy scenario constructed by syzkaller was something like: (1) create a TCP socket (2) initiate a TFO connect via sendto() (3) while socket is in TCP_SYN_SENT, call setsockopt(TCP_CONGESTION), which calls: tcp_set_congestion_control() -> tcp_reinit_congestion_control() -> tcp_init_congestion_control() (4) receive ACK, connection is established, call tcp_init_transfer(), set icsk_ca_initialized=0 (without first calling cc->release()), call tcp_init_congestion_control() again. Note that in this sequence tcp_init_congestion_control() is called twice without a cc->release() call in between. Thus, for CC modules that allocate memory in their init() function, e.g, CDG, a memory leak may occur. The syzkaller tool managed to find a reproducer that triggered such a leak in CDG. The bug was introduced when that commit 8919a9b31eb4 ("tcp: Only init congestion control if not initialized already") introduced icsk_ca_initialized and set icsk_ca_initialized to 0 in tcp_init_transfer(), missing the possibility for a sequence like the one above, where a process could call setsockopt(TCP_CONGESTION) in state TCP_SYN_SENT (i.e. after the connect() or TFO open sendmsg()), which would call tcp_init_congestion_control(). It did not intend to reset any initialization that the user had already explicitly made; it just missed the possibility of that particular sequence (which syzkaller managed to find). Fixes: commit 8919a9b31eb4 ("tcp: Only init congestion control if not initialized already") Reported-by: syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com> --- V2: - Modify the Subject line. - Adjust the commit message. - Add Fixes: tag. net/ipv4/tcp_input.c | 1 - 1 file changed, 1 deletion(-) -- 2.25.1