From: Neal Cardwell <ncardwell@google.com> To: Phi Nguyen <phind.uet@gmail.com> Cc: Eric Dumazet <edumazet@google.com>, David Miller <davem@davemloft.net>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, David Ahern <dsahern@kernel.org>, Jakub Kicinski <kuba@kernel.org>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>, John Fastabend <john.fastabend@gmail.com>, kpsingh@kernel.org, netdev <netdev@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com, Yuchung Cheng <ycheng@google.com> Subject: Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer. Date: Thu, 1 Jul 2021 10:23:13 -0400 [thread overview] Message-ID: <CADVnQyn_965EHdQke_S7FUySiamyyRx-3b8o+cm+=4jYxG_GOw@mail.gmail.com> (raw) In-Reply-To: <30527e25-dd66-da7a-7344-494b4539abf7@gmail.com> On Wed, Jun 30, 2021 at 2:25 PM Phi Nguyen <phind.uet@gmail.com> wrote: > > On 6/29/2021 11:59 PM, Neal Cardwell wrote: > > On Tue, Jun 29, 2021 at 8:58 AM Eric Dumazet <edumazet@google.com> wrote: > > From my perspective, the bug was introduced when that 8919a9b31eb4 > > commit introduced icsk_ca_initialized and set icsk_ca_initialized to 0 > > in tcp_init_transfer(), missing the possibility that 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(). The 8919a9b31eb4 commit 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!). > > > >> Although I am not sure what happens at accept() time when the listener > >> socket is cloned. > > > > It seems that for listener sockets, they cannot initialize their CC > > module state, because there is no way for them to reach > > tcp_init_congestion_control(), since: > > > > (a) tcp_set_congestion_control() -> tcp_reinit_congestion_control() > > will not call tcp_init_congestion_control() on a socket in CLOSE or > > LISTEN > > > > (b) tcp_init_transfer() -> tcp_init_congestion_control() can only > > happen for established sockets and successful TFO SYN_RECV sockets > Is this what was mentioned in this commit ce69e563b325(tcp: make sure > listeners don't initialize congestion-control state) Yes, exactly. > > -- > > [PATCH] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized > > > > 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 8919a9b31eb4 commit 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(). The 8919a9b31eb4 commit 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). > > Could I use your commit message when I resubmit patch? Yes, feel free to use that commit message verbatim or modified. thanks, neal
WARNING: multiple messages have this Message-ID (diff)
From: Neal Cardwell via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org> To: Phi Nguyen <phind.uet@gmail.com> Cc: Song Liu <songliubraving@fb.com>, Martin KaFai Lau <kafai@fb.com>, syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com, Daniel Borkmann <daniel@iogearbox.net>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, netdev <netdev@vger.kernel.org>, David Ahern <dsahern@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, Eric Dumazet <edumazet@google.com>, Yuchung Cheng <ycheng@google.com>, kpsingh@kernel.org, Jakub Kicinski <kuba@kernel.org>, bpf <bpf@vger.kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, David Miller <davem@davemloft.net>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer. Date: Thu, 1 Jul 2021 10:23:13 -0400 [thread overview] Message-ID: <CADVnQyn_965EHdQke_S7FUySiamyyRx-3b8o+cm+=4jYxG_GOw@mail.gmail.com> (raw) In-Reply-To: <30527e25-dd66-da7a-7344-494b4539abf7@gmail.com> On Wed, Jun 30, 2021 at 2:25 PM Phi Nguyen <phind.uet@gmail.com> wrote: > > On 6/29/2021 11:59 PM, Neal Cardwell wrote: > > On Tue, Jun 29, 2021 at 8:58 AM Eric Dumazet <edumazet@google.com> wrote: > > From my perspective, the bug was introduced when that 8919a9b31eb4 > > commit introduced icsk_ca_initialized and set icsk_ca_initialized to 0 > > in tcp_init_transfer(), missing the possibility that 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(). The 8919a9b31eb4 commit 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!). > > > >> Although I am not sure what happens at accept() time when the listener > >> socket is cloned. > > > > It seems that for listener sockets, they cannot initialize their CC > > module state, because there is no way for them to reach > > tcp_init_congestion_control(), since: > > > > (a) tcp_set_congestion_control() -> tcp_reinit_congestion_control() > > will not call tcp_init_congestion_control() on a socket in CLOSE or > > LISTEN > > > > (b) tcp_init_transfer() -> tcp_init_congestion_control() can only > > happen for established sockets and successful TFO SYN_RECV sockets > Is this what was mentioned in this commit ce69e563b325(tcp: make sure > listeners don't initialize congestion-control state) Yes, exactly. > > -- > > [PATCH] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized > > > > 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 8919a9b31eb4 commit 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(). The 8919a9b31eb4 commit 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). > > Could I use your commit message when I resubmit patch? Yes, feel free to use that commit message verbatim or modified. thanks, neal _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2021-07-01 14:23 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-28 14:49 [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer Nguyen Dinh Phi 2021-06-28 14:49 ` Nguyen Dinh Phi 2021-06-28 14:52 ` Eric Dumazet 2021-06-28 14:52 ` Eric Dumazet via Linux-kernel-mentees 2021-06-28 16:18 ` Phi Nguyen 2021-06-28 16:18 ` Phi Nguyen 2021-06-28 16:24 ` Neal Cardwell 2021-06-28 16:24 ` Neal Cardwell via Linux-kernel-mentees 2021-06-28 17:15 ` Phi Nguyen 2021-06-28 17:15 ` Phi Nguyen 2021-06-28 17:20 ` Neal Cardwell 2021-06-28 17:20 ` Neal Cardwell via Linux-kernel-mentees 2021-06-28 23:41 ` Martin KaFai Lau 2021-06-28 23:41 ` Martin KaFai Lau via Linux-kernel-mentees 2021-06-29 6:03 ` Phi Nguyen Dinh 2021-06-29 7:17 ` Nguyen Dinh Phi 2021-06-29 7:17 ` Nguyen Dinh Phi 2021-06-29 8:21 ` Eric Dumazet 2021-06-29 8:21 ` Eric Dumazet via Linux-kernel-mentees 2021-06-29 12:28 ` Nguyen Dinh Phi 2021-06-29 12:28 ` Nguyen Dinh Phi 2021-06-29 12:58 ` Eric Dumazet 2021-06-29 12:58 ` Eric Dumazet via Linux-kernel-mentees 2021-06-29 15:59 ` Neal Cardwell 2021-06-29 15:59 ` Neal Cardwell via Linux-kernel-mentees 2021-06-30 18:25 ` Phi Nguyen 2021-06-30 18:25 ` Phi Nguyen 2021-07-01 14:23 ` Neal Cardwell [this message] 2021-07-01 14:23 ` Neal Cardwell via Linux-kernel-mentees 2021-06-28 17:09 ` Neal Cardwell 2021-06-28 17:09 ` Neal Cardwell via Linux-kernel-mentees
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CADVnQyn_965EHdQke_S7FUySiamyyRx-3b8o+cm+=4jYxG_GOw@mail.gmail.com' \ --to=ncardwell@google.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=dsahern@kernel.org \ --cc=edumazet@google.com \ --cc=john.fastabend@gmail.com \ --cc=kafai@fb.com \ --cc=kpsingh@kernel.org \ --cc=kuba@kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=phind.uet@gmail.com \ --cc=songliubraving@fb.com \ --cc=syzbot+f1e24a0594d4e3a895d3@syzkaller.appspotmail.com \ --cc=ycheng@google.com \ --cc=yoshfuji@linux-ipv6.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.