* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-28 17:20 ` Neal Cardwell via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Neal Cardwell via Linux-kernel-mentees @ 2021-06-28 17:20 UTC (permalink / raw)
To: Phi Nguyen
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Eric Dumazet, Yuchung Cheng, kpsingh, Jakub Kicinski, bpf,
linux-kernel-mentees, David Miller, LKML
)
On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
>
> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
>
> > Thanks.
> >
> > Can you also please provide a summary of the event sequence that
> > triggers the bug? Based on your Reported-by tag, I guess this is based
> > on the syzbot reproducer:
> >
> > https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> >
> > but perhaps you can give a summary of the event sequence that causes
> > the bug? Is it that the call:
> >
> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> > &(0x7f0000000000)='cdg\x00', 0x4)
> >
> > initializes the CC and happens before the connection is established,
> > and then when the connection is established, the line that sets:
> > icsk->icsk_ca_initialized = 0;
> > is incorrect, causing the CC to be initialized again without first
> > calling the cleanup code that deallocates the CDG-allocated memory?
> >
> > thanks,
> > neal
> >
>
> Hi Neal,
>
> The gdb stack trace that lead to init_transfer_input() is as bellow, the
> current sock state is TCP_SYN_RECV.
Thanks. That makes sense as a snapshot of time for
tcp_init_transfer(), but I think what would be more useful would be a
description of the sequence of events, including when the CC was
initialized previous to that point (as noted above, was it that the
setsockopt(TCP_CONGESTION) completed before that point?).
thanks,
neal
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-28 17:20 ` Neal Cardwell via Linux-kernel-mentees
@ 2021-06-28 23:41 ` Martin KaFai Lau via Linux-kernel-mentees
-1 siblings, 0 replies; 31+ messages in thread
From: Martin KaFai Lau @ 2021-06-28 23:41 UTC (permalink / raw)
To: Phi Nguyen
Cc: Neal Cardwell, Eric Dumazet, David Miller, Hideaki YOSHIFUJI,
David Ahern, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, John Fastabend, kpsingh, netdev, LKML,
bpf, linux-kernel-mentees, syzbot+f1e24a0594d4e3a895d3,
Yuchung Cheng
On Mon, Jun 28, 2021 at 01:20:19PM -0400, Neal Cardwell wrote:
> )
>
> On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
> >
> > On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >
> > > Thanks.
> > >
> > > Can you also please provide a summary of the event sequence that
> > > triggers the bug? Based on your Reported-by tag, I guess this is based
> > > on the syzbot reproducer:
> > >
> > > https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> > >
> > > but perhaps you can give a summary of the event sequence that causes
> > > the bug? Is it that the call:
> > >
> > > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> > > &(0x7f0000000000)='cdg\x00', 0x4)
> > >
> > > initializes the CC and happens before the connection is established,
> > > and then when the connection is established, the line that sets:
> > > icsk->icsk_ca_initialized = 0;
> > > is incorrect, causing the CC to be initialized again without first
> > > calling the cleanup code that deallocates the CDG-allocated memory?
> > >
> > > thanks,
> > > neal
> > >
> >
> > Hi Neal,
> >
> > The gdb stack trace that lead to init_transfer_input() is as bellow, the
> > current sock state is TCP_SYN_RECV.
>
> Thanks. That makes sense as a snapshot of time for
> tcp_init_transfer(), but I think what would be more useful would be a
> description of the sequence of events, including when the CC was
> initialized previous to that point (as noted above, was it that the
> setsockopt(TCP_CONGESTION) completed before that point?).
+1. It needs to first explain when was the very first CC initialized.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-28 23:41 ` Martin KaFai Lau via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Martin KaFai Lau via Linux-kernel-mentees @ 2021-06-28 23:41 UTC (permalink / raw)
To: Phi Nguyen
Cc: Song Liu, syzbot+f1e24a0594d4e3a895d3, Daniel Borkmann,
Hideaki YOSHIFUJI, netdev, David Ahern, John Fastabend,
Alexei Starovoitov, Andrii Nakryiko, Eric Dumazet, Yuchung Cheng,
kpsingh, Jakub Kicinski, bpf, Neal Cardwell, David Miller, LKML,
linux-kernel-mentees
On Mon, Jun 28, 2021 at 01:20:19PM -0400, Neal Cardwell wrote:
> )
>
> On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
> >
> > On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >
> > > Thanks.
> > >
> > > Can you also please provide a summary of the event sequence that
> > > triggers the bug? Based on your Reported-by tag, I guess this is based
> > > on the syzbot reproducer:
> > >
> > > https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> > >
> > > but perhaps you can give a summary of the event sequence that causes
> > > the bug? Is it that the call:
> > >
> > > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> > > &(0x7f0000000000)='cdg\x00', 0x4)
> > >
> > > initializes the CC and happens before the connection is established,
> > > and then when the connection is established, the line that sets:
> > > icsk->icsk_ca_initialized = 0;
> > > is incorrect, causing the CC to be initialized again without first
> > > calling the cleanup code that deallocates the CDG-allocated memory?
> > >
> > > thanks,
> > > neal
> > >
> >
> > Hi Neal,
> >
> > The gdb stack trace that lead to init_transfer_input() is as bellow, the
> > current sock state is TCP_SYN_RECV.
>
> Thanks. That makes sense as a snapshot of time for
> tcp_init_transfer(), but I think what would be more useful would be a
> description of the sequence of events, including when the CC was
> initialized previous to that point (as noted above, was it that the
> setsockopt(TCP_CONGESTION) completed before that point?).
+1. It needs to first explain when was the very first CC initialized.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-28 17:20 ` Neal Cardwell via Linux-kernel-mentees
(?)
(?)
@ 2021-06-29 6:03 ` Phi Nguyen Dinh
-1 siblings, 0 replies; 31+ messages in thread
From: Phi Nguyen Dinh @ 2021-06-29 6:03 UTC (permalink / raw)
To: Neal Cardwell
Cc: Song Liu, Martin KaFai Lau, syzbot, Daniel Borkmann,
Hideaki YOSHIFUJI, netdev, David Ahern, John Fastabend,
Alexei Starovoitov, Andrii Nakryiko, Eric Dumazet, Yuchung Cheng,
kpsingh, Jakub Kicinski, bpf, linux-kernel-mentees, David Miller,
LKML
[-- Attachment #1.1: Type: text/plain, Size: 1744 bytes --]
Yes, the CC had been initialized by setsockopt, after that, it was
intialized again in function tcp_init_transfer() because of
setting icsk_ca_initialized to 0.
Regards,
On Tue, Jun 29, 2021, 01:20 Neal Cardwell <ncardwell@google.com> wrote:
> )
>
> On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
> >
> > On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >
> > > Thanks.
> > >
> > > Can you also please provide a summary of the event sequence that
> > > triggers the bug? Based on your Reported-by tag, I guess this is based
> > > on the syzbot reproducer:
> > >
> > >
> https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> > >
> > > but perhaps you can give a summary of the event sequence that causes
> > > the bug? Is it that the call:
> > >
> > > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> > > &(0x7f0000000000)='cdg\x00', 0x4)
> > >
> > > initializes the CC and happens before the connection is established,
> > > and then when the connection is established, the line that sets:
> > > icsk->icsk_ca_initialized = 0;
> > > is incorrect, causing the CC to be initialized again without first
> > > calling the cleanup code that deallocates the CDG-allocated memory?
> > >
> > > thanks,
> > > neal
> > >
> >
> > Hi Neal,
> >
> > The gdb stack trace that lead to init_transfer_input() is as bellow, the
> > current sock state is TCP_SYN_RECV.
>
> Thanks. That makes sense as a snapshot of time for
> tcp_init_transfer(), but I think what would be more useful would be a
> description of the sequence of events, including when the CC was
> initialized previous to that point (as noted above, was it that the
> setsockopt(TCP_CONGESTION) completed before that point?).
>
> thanks,
> neal
>
[-- Attachment #1.2: Type: text/html, Size: 2802 bytes --]
[-- Attachment #2: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-28 17:20 ` Neal Cardwell via Linux-kernel-mentees
@ 2021-06-29 7:17 ` Nguyen Dinh Phi
-1 siblings, 0 replies; 31+ messages in thread
From: Nguyen Dinh Phi @ 2021-06-29 7:17 UTC (permalink / raw)
To: Neal Cardwell
Cc: Eric Dumazet, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell <ncardwell@google.com> wrote:
>)
>
>On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
>>
>> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
>>
>> > Thanks.
>> >
>> > Can you also please provide a summary of the event sequence that
>> > triggers the bug? Based on your Reported-by tag, I guess this is
>based
>> > on the syzbot reproducer:
>> >
>> >
>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
>> >
>> > but perhaps you can give a summary of the event sequence that
>causes
>> > the bug? Is it that the call:
>> >
>> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
>> > &(0x7f0000000000)='cdg\x00', 0x4)
>> >
>> > initializes the CC and happens before the connection is
>established,
>> > and then when the connection is established, the line that sets:
>> > icsk->icsk_ca_initialized = 0;
>> > is incorrect, causing the CC to be initialized again without first
>> > calling the cleanup code that deallocates the CDG-allocated memory?
>> >
>> > thanks,
>> > neal
>> >
>>
>> Hi Neal,
>>
>> The gdb stack trace that lead to init_transfer_input() is as bellow,
>the
>> current sock state is TCP_SYN_RECV.
>
>Thanks. That makes sense as a snapshot of time for
>tcp_init_transfer(), but I think what would be more useful would be a
>description of the sequence of events, including when the CC was
>initialized previous to that point (as noted above, was it that the
>setsockopt(TCP_CONGESTION) completed before that point?).
>
>thanks,
>neal
I resend my message because I accidently used html format in last one. I am very sorry for the inconvenience caused.
---
Yes, the CC had been initialized by the setsockopt, after that, it was initialized again in function tcp_init_transfer() because of setting isck_ca_initialized to 0.
Regards,
Phi.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-29 7:17 ` Nguyen Dinh Phi
0 siblings, 0 replies; 31+ messages in thread
From: Nguyen Dinh Phi @ 2021-06-29 7:17 UTC (permalink / raw)
To: Neal Cardwell
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Eric Dumazet, Yuchung Cheng, kpsingh, Jakub Kicinski, bpf,
linux-kernel-mentees, David Miller, LKML
On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell <ncardwell@google.com> wrote:
>)
>
>On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
>>
>> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
>>
>> > Thanks.
>> >
>> > Can you also please provide a summary of the event sequence that
>> > triggers the bug? Based on your Reported-by tag, I guess this is
>based
>> > on the syzbot reproducer:
>> >
>> >
>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
>> >
>> > but perhaps you can give a summary of the event sequence that
>causes
>> > the bug? Is it that the call:
>> >
>> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
>> > &(0x7f0000000000)='cdg\x00', 0x4)
>> >
>> > initializes the CC and happens before the connection is
>established,
>> > and then when the connection is established, the line that sets:
>> > icsk->icsk_ca_initialized = 0;
>> > is incorrect, causing the CC to be initialized again without first
>> > calling the cleanup code that deallocates the CDG-allocated memory?
>> >
>> > thanks,
>> > neal
>> >
>>
>> Hi Neal,
>>
>> The gdb stack trace that lead to init_transfer_input() is as bellow,
>the
>> current sock state is TCP_SYN_RECV.
>
>Thanks. That makes sense as a snapshot of time for
>tcp_init_transfer(), but I think what would be more useful would be a
>description of the sequence of events, including when the CC was
>initialized previous to that point (as noted above, was it that the
>setsockopt(TCP_CONGESTION) completed before that point?).
>
>thanks,
>neal
I resend my message because I accidently used html format in last one. I am very sorry for the inconvenience caused.
---
Yes, the CC had been initialized by the setsockopt, after that, it was initialized again in function tcp_init_transfer() because of setting isck_ca_initialized to 0.
Regards,
Phi.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-29 7:17 ` Nguyen Dinh Phi
@ 2021-06-29 8:21 ` Eric Dumazet via Linux-kernel-mentees
-1 siblings, 0 replies; 31+ messages in thread
From: Eric Dumazet @ 2021-06-29 8:21 UTC (permalink / raw)
To: Nguyen Dinh Phi
Cc: Neal Cardwell, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell <ncardwell@google.com> wrote:
> >)
> >
> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
> >>
> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >>
> >> > Thanks.
> >> >
> >> > Can you also please provide a summary of the event sequence that
> >> > triggers the bug? Based on your Reported-by tag, I guess this is
> >based
> >> > on the syzbot reproducer:
> >> >
> >> >
> >https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> >> >
> >> > but perhaps you can give a summary of the event sequence that
> >causes
> >> > the bug? Is it that the call:
> >> >
> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> >> > &(0x7f0000000000)='cdg\x00', 0x4)
> >> >
> >> > initializes the CC and happens before the connection is
> >established,
> >> > and then when the connection is established, the line that sets:
> >> > icsk->icsk_ca_initialized = 0;
> >> > is incorrect, causing the CC to be initialized again without first
> >> > calling the cleanup code that deallocates the CDG-allocated memory?
> >> >
> >> > thanks,
> >> > neal
> >> >
> >>
> >> Hi Neal,
> >>
> >> The gdb stack trace that lead to init_transfer_input() is as bellow,
> >the
> >> current sock state is TCP_SYN_RECV.
> >
> >Thanks. That makes sense as a snapshot of time for
> >tcp_init_transfer(), but I think what would be more useful would be a
> >description of the sequence of events, including when the CC was
> >initialized previous to that point (as noted above, was it that the
> >setsockopt(TCP_CONGESTION) completed before that point?).
> >
> >thanks,
> >neal
>
> I resend my message because I accidently used html format in last one. I am very sorry for the inconvenience caused.
> ---
> Yes, the CC had been initialized by the setsockopt, after that, it was initialized again in function tcp_init_transfer() because of setting isck_ca_initialized to 0.
"the setsockopt" is rather vague, sorry.
The hard part is that all scenarios have to be considered.
TCP flows can either be passive and active.
CC can be set :
1) Before the connect() or accept()
2) After the connect() or accept()
3) after the connect() but before 3WHS is completed.
So we need to make sure all cases will still work with any combination
of CDG CC (before/after) in the picture.
Note that a memory leak for a restricted CC (CDG can only be used by
CAP_NET_ADMIN privileged user)
is a small problem compared to more serious bug that could be added
by an incomplete fix.
I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
tcp_cdg would no longer need a dynamic memory allocation.
Thank you.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-29 8:21 ` Eric Dumazet via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Eric Dumazet via Linux-kernel-mentees @ 2021-06-29 8:21 UTC (permalink / raw)
To: Nguyen Dinh Phi
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Yuchung Cheng, kpsingh, Jakub Kicinski, bpf, Neal Cardwell,
David Miller, LKML, linux-kernel-mentees
On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell <ncardwell@google.com> wrote:
> >)
> >
> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com> wrote:
> >>
> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >>
> >> > Thanks.
> >> >
> >> > Can you also please provide a summary of the event sequence that
> >> > triggers the bug? Based on your Reported-by tag, I guess this is
> >based
> >> > on the syzbot reproducer:
> >> >
> >> >
> >https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> >> >
> >> > but perhaps you can give a summary of the event sequence that
> >causes
> >> > the bug? Is it that the call:
> >> >
> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> >> > &(0x7f0000000000)='cdg\x00', 0x4)
> >> >
> >> > initializes the CC and happens before the connection is
> >established,
> >> > and then when the connection is established, the line that sets:
> >> > icsk->icsk_ca_initialized = 0;
> >> > is incorrect, causing the CC to be initialized again without first
> >> > calling the cleanup code that deallocates the CDG-allocated memory?
> >> >
> >> > thanks,
> >> > neal
> >> >
> >>
> >> Hi Neal,
> >>
> >> The gdb stack trace that lead to init_transfer_input() is as bellow,
> >the
> >> current sock state is TCP_SYN_RECV.
> >
> >Thanks. That makes sense as a snapshot of time for
> >tcp_init_transfer(), but I think what would be more useful would be a
> >description of the sequence of events, including when the CC was
> >initialized previous to that point (as noted above, was it that the
> >setsockopt(TCP_CONGESTION) completed before that point?).
> >
> >thanks,
> >neal
>
> I resend my message because I accidently used html format in last one. I am very sorry for the inconvenience caused.
> ---
> Yes, the CC had been initialized by the setsockopt, after that, it was initialized again in function tcp_init_transfer() because of setting isck_ca_initialized to 0.
"the setsockopt" is rather vague, sorry.
The hard part is that all scenarios have to be considered.
TCP flows can either be passive and active.
CC can be set :
1) Before the connect() or accept()
2) After the connect() or accept()
3) after the connect() but before 3WHS is completed.
So we need to make sure all cases will still work with any combination
of CDG CC (before/after) in the picture.
Note that a memory leak for a restricted CC (CDG can only be used by
CAP_NET_ADMIN privileged user)
is a small problem compared to more serious bug that could be added
by an incomplete fix.
I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
tcp_cdg would no longer need a dynamic memory allocation.
Thank you.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-29 8:21 ` Eric Dumazet via Linux-kernel-mentees
@ 2021-06-29 12:28 ` Nguyen Dinh Phi
-1 siblings, 0 replies; 31+ messages in thread
From: Nguyen Dinh Phi @ 2021-06-29 12:28 UTC (permalink / raw)
To: Eric Dumazet
Cc: Neal Cardwell, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
On June 29, 2021 4:21:59 PM GMT+08:00, Eric Dumazet <edumazet@google.com> wrote:
>On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com>
>wrote:
>>
>> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell
><ncardwell@google.com> wrote:
>> >)
>> >
>> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com>
>wrote:
>> >>
>> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
>> >>
>> >> > Thanks.
>> >> >
>> >> > Can you also please provide a summary of the event sequence that
>> >> > triggers the bug? Based on your Reported-by tag, I guess this is
>> >based
>> >> > on the syzbot reproducer:
>> >> >
>> >> >
>>
>>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
>> >> >
>> >> > but perhaps you can give a summary of the event sequence that
>> >causes
>> >> > the bug? Is it that the call:
>> >> >
>> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
>> >> > &(0x7f0000000000)='cdg\x00', 0x4)
>> >> >
>> >> > initializes the CC and happens before the connection is
>> >established,
>> >> > and then when the connection is established, the line that sets:
>> >> > icsk->icsk_ca_initialized = 0;
>> >> > is incorrect, causing the CC to be initialized again without
>first
>> >> > calling the cleanup code that deallocates the CDG-allocated
>memory?
>> >> >
>> >> > thanks,
>> >> > neal
>> >> >
>> >>
>> >> Hi Neal,
>> >>
>> >> The gdb stack trace that lead to init_transfer_input() is as
>bellow,
>> >the
>> >> current sock state is TCP_SYN_RECV.
>> >
>> >Thanks. That makes sense as a snapshot of time for
>> >tcp_init_transfer(), but I think what would be more useful would be
>a
>> >description of the sequence of events, including when the CC was
>> >initialized previous to that point (as noted above, was it that the
>> >setsockopt(TCP_CONGESTION) completed before that point?).
>> >
>> >thanks,
>> >neal
>>
>> I resend my message because I accidently used html format in last
>one. I am very sorry for the inconvenience caused.
>> ---
>> Yes, the CC had been initialized by the setsockopt, after that, it
>was initialized again in function tcp_init_transfer() because of
>setting isck_ca_initialized to 0.
>
>"the setsockopt" is rather vague, sorry.
>
>
>The hard part is that all scenarios have to be considered.
>
>TCP flows can either be passive and active.
>
>CC can be set :
>
>1) Before the connect() or accept()
>2) After the connect() or accept()
>3) after the connect() but before 3WHS is completed.
>
>So we need to make sure all cases will still work with any combination
>of CDG CC (before/after) in the picture.
>
>Note that a memory leak for a restricted CC (CDG can only be used by
>CAP_NET_ADMIN privileged user)
> is a small problem compared to more serious bug that could be added
>by an incomplete fix.
>
>I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
>tcp_cdg would no longer need a dynamic memory allocation.
>
>Thank you.
Hi,
I will try to see whether I am able to get the full sequence. I am also affraid of making a change that could affect big part of the kernel.
About CDG, how we can get rid of dynamic allocation by increasing icsk_priv_data to 120? because I see that the window size is a module parameter, so I guess it is not a fixed value.
Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
Thank you.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-29 12:28 ` Nguyen Dinh Phi
0 siblings, 0 replies; 31+ messages in thread
From: Nguyen Dinh Phi @ 2021-06-29 12:28 UTC (permalink / raw)
To: Eric Dumazet
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Yuchung Cheng, kpsingh, Jakub Kicinski, bpf, Neal Cardwell,
David Miller, LKML, linux-kernel-mentees
On June 29, 2021 4:21:59 PM GMT+08:00, Eric Dumazet <edumazet@google.com> wrote:
>On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com>
>wrote:
>>
>> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell
><ncardwell@google.com> wrote:
>> >)
>> >
>> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com>
>wrote:
>> >>
>> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
>> >>
>> >> > Thanks.
>> >> >
>> >> > Can you also please provide a summary of the event sequence that
>> >> > triggers the bug? Based on your Reported-by tag, I guess this is
>> >based
>> >> > on the syzbot reproducer:
>> >> >
>> >> >
>>
>>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
>> >> >
>> >> > but perhaps you can give a summary of the event sequence that
>> >causes
>> >> > the bug? Is it that the call:
>> >> >
>> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
>> >> > &(0x7f0000000000)='cdg\x00', 0x4)
>> >> >
>> >> > initializes the CC and happens before the connection is
>> >established,
>> >> > and then when the connection is established, the line that sets:
>> >> > icsk->icsk_ca_initialized = 0;
>> >> > is incorrect, causing the CC to be initialized again without
>first
>> >> > calling the cleanup code that deallocates the CDG-allocated
>memory?
>> >> >
>> >> > thanks,
>> >> > neal
>> >> >
>> >>
>> >> Hi Neal,
>> >>
>> >> The gdb stack trace that lead to init_transfer_input() is as
>bellow,
>> >the
>> >> current sock state is TCP_SYN_RECV.
>> >
>> >Thanks. That makes sense as a snapshot of time for
>> >tcp_init_transfer(), but I think what would be more useful would be
>a
>> >description of the sequence of events, including when the CC was
>> >initialized previous to that point (as noted above, was it that the
>> >setsockopt(TCP_CONGESTION) completed before that point?).
>> >
>> >thanks,
>> >neal
>>
>> I resend my message because I accidently used html format in last
>one. I am very sorry for the inconvenience caused.
>> ---
>> Yes, the CC had been initialized by the setsockopt, after that, it
>was initialized again in function tcp_init_transfer() because of
>setting isck_ca_initialized to 0.
>
>"the setsockopt" is rather vague, sorry.
>
>
>The hard part is that all scenarios have to be considered.
>
>TCP flows can either be passive and active.
>
>CC can be set :
>
>1) Before the connect() or accept()
>2) After the connect() or accept()
>3) after the connect() but before 3WHS is completed.
>
>So we need to make sure all cases will still work with any combination
>of CDG CC (before/after) in the picture.
>
>Note that a memory leak for a restricted CC (CDG can only be used by
>CAP_NET_ADMIN privileged user)
> is a small problem compared to more serious bug that could be added
>by an incomplete fix.
>
>I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
>tcp_cdg would no longer need a dynamic memory allocation.
>
>Thank you.
Hi,
I will try to see whether I am able to get the full sequence. I am also affraid of making a change that could affect big part of the kernel.
About CDG, how we can get rid of dynamic allocation by increasing icsk_priv_data to 120? because I see that the window size is a module parameter, so I guess it is not a fixed value.
Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
Thank you.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-29 12:28 ` Nguyen Dinh Phi
@ 2021-06-29 12:58 ` Eric Dumazet via Linux-kernel-mentees
-1 siblings, 0 replies; 31+ messages in thread
From: Eric Dumazet @ 2021-06-29 12:58 UTC (permalink / raw)
To: Nguyen Dinh Phi
Cc: Neal Cardwell, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
On Tue, Jun 29, 2021 at 2:28 PM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> On June 29, 2021 4:21:59 PM GMT+08:00, Eric Dumazet <edumazet@google.com> wrote:
> >On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com>
> >wrote:
> >>
> >> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell
> ><ncardwell@google.com> wrote:
> >> >)
> >> >
> >> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com>
> >wrote:
> >> >>
> >> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >> >>
> >> >> > Thanks.
> >> >> >
> >> >> > Can you also please provide a summary of the event sequence that
> >> >> > triggers the bug? Based on your Reported-by tag, I guess this is
> >> >based
> >> >> > on the syzbot reproducer:
> >> >> >
> >> >> >
> >>
> >>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> >> >> >
> >> >> > but perhaps you can give a summary of the event sequence that
> >> >causes
> >> >> > the bug? Is it that the call:
> >> >> >
> >> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> >> >> > &(0x7f0000000000)='cdg\x00', 0x4)
> >> >> >
> >> >> > initializes the CC and happens before the connection is
> >> >established,
> >> >> > and then when the connection is established, the line that sets:
> >> >> > icsk->icsk_ca_initialized = 0;
> >> >> > is incorrect, causing the CC to be initialized again without
> >first
> >> >> > calling the cleanup code that deallocates the CDG-allocated
> >memory?
> >> >> >
> >> >> > thanks,
> >> >> > neal
> >> >> >
> >> >>
> >> >> Hi Neal,
> >> >>
> >> >> The gdb stack trace that lead to init_transfer_input() is as
> >bellow,
> >> >the
> >> >> current sock state is TCP_SYN_RECV.
> >> >
> >> >Thanks. That makes sense as a snapshot of time for
> >> >tcp_init_transfer(), but I think what would be more useful would be
> >a
> >> >description of the sequence of events, including when the CC was
> >> >initialized previous to that point (as noted above, was it that the
> >> >setsockopt(TCP_CONGESTION) completed before that point?).
> >> >
> >> >thanks,
> >> >neal
> >>
> >> I resend my message because I accidently used html format in last
> >one. I am very sorry for the inconvenience caused.
> >> ---
> >> Yes, the CC had been initialized by the setsockopt, after that, it
> >was initialized again in function tcp_init_transfer() because of
> >setting isck_ca_initialized to 0.
> >
> >"the setsockopt" is rather vague, sorry.
> >
> >
> >The hard part is that all scenarios have to be considered.
> >
> >TCP flows can either be passive and active.
> >
> >CC can be set :
> >
> >1) Before the connect() or accept()
> >2) After the connect() or accept()
> >3) after the connect() but before 3WHS is completed.
> >
> >So we need to make sure all cases will still work with any combination
> >of CDG CC (before/after) in the picture.
> >
> >Note that a memory leak for a restricted CC (CDG can only be used by
> >CAP_NET_ADMIN privileged user)
> > is a small problem compared to more serious bug that could be added
> >by an incomplete fix.
> >
> >I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
> >tcp_cdg would no longer need a dynamic memory allocation.
> >
> >Thank you.
>
> Hi,
> I will try to see whether I am able to get the full sequence. I am also affraid of making a change that could affect big part of the kernel.
> About CDG, how we can get rid of dynamic allocation by increasing icsk_priv_data to 120? because I see that the window size is a module parameter, so I guess it is not a fixed value.
Given this module parameter is constant, I doubt anyone really uses a
bigger window.
If researchers want to experiment bigger window, they could adjust a
macro and recompile (#define TCP_CDG_WINDOW 8 -> X)
> Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
I think icsk_ca_initialized lost its strong meaning when CDG was
introduced (since this is the only CC allocating memory)
The bug really is that before clearing icsk_ca_initialized we should
call cc->release()
Maybe we missed this cleanup in commit
8919a9b31eb4fb4c0a93e5fb350a626924302aa6 ("tcp: Only init congestion
control if not initialized already")
Although I am not sure what happens at accept() time when the listener
socket is cloned.
If we make any hypothesis, we need to check all CC modules to make
sure they respect it.
>
> Thank you.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-29 12:58 ` Eric Dumazet via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Eric Dumazet via Linux-kernel-mentees @ 2021-06-29 12:58 UTC (permalink / raw)
To: Nguyen Dinh Phi
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Yuchung Cheng, kpsingh, Jakub Kicinski, bpf, Neal Cardwell,
David Miller, LKML, linux-kernel-mentees
On Tue, Jun 29, 2021 at 2:28 PM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> On June 29, 2021 4:21:59 PM GMT+08:00, Eric Dumazet <edumazet@google.com> wrote:
> >On Tue, Jun 29, 2021 at 9:17 AM Nguyen Dinh Phi <phind.uet@gmail.com>
> >wrote:
> >>
> >> On June 29, 2021 1:20:19 AM GMT+08:00, Neal Cardwell
> ><ncardwell@google.com> wrote:
> >> >)
> >> >
> >> >On Mon, Jun 28, 2021 at 1:15 PM Phi Nguyen <phind.uet@gmail.com>
> >wrote:
> >> >>
> >> >> On 6/29/2021 12:24 AM, Neal Cardwell wrote:
> >> >>
> >> >> > Thanks.
> >> >> >
> >> >> > Can you also please provide a summary of the event sequence that
> >> >> > triggers the bug? Based on your Reported-by tag, I guess this is
> >> >based
> >> >> > on the syzbot reproducer:
> >> >> >
> >> >> >
> >>
> >>https://groups.google.com/g/syzkaller-bugs/c/VbHoSsBz0hk/m/cOxOoTgPCAAJ
> >> >> >
> >> >> > but perhaps you can give a summary of the event sequence that
> >> >causes
> >> >> > the bug? Is it that the call:
> >> >> >
> >> >> > setsockopt$inet_tcp_TCP_CONGESTION(r0, 0x6, 0xd,
> >> >> > &(0x7f0000000000)='cdg\x00', 0x4)
> >> >> >
> >> >> > initializes the CC and happens before the connection is
> >> >established,
> >> >> > and then when the connection is established, the line that sets:
> >> >> > icsk->icsk_ca_initialized = 0;
> >> >> > is incorrect, causing the CC to be initialized again without
> >first
> >> >> > calling the cleanup code that deallocates the CDG-allocated
> >memory?
> >> >> >
> >> >> > thanks,
> >> >> > neal
> >> >> >
> >> >>
> >> >> Hi Neal,
> >> >>
> >> >> The gdb stack trace that lead to init_transfer_input() is as
> >bellow,
> >> >the
> >> >> current sock state is TCP_SYN_RECV.
> >> >
> >> >Thanks. That makes sense as a snapshot of time for
> >> >tcp_init_transfer(), but I think what would be more useful would be
> >a
> >> >description of the sequence of events, including when the CC was
> >> >initialized previous to that point (as noted above, was it that the
> >> >setsockopt(TCP_CONGESTION) completed before that point?).
> >> >
> >> >thanks,
> >> >neal
> >>
> >> I resend my message because I accidently used html format in last
> >one. I am very sorry for the inconvenience caused.
> >> ---
> >> Yes, the CC had been initialized by the setsockopt, after that, it
> >was initialized again in function tcp_init_transfer() because of
> >setting isck_ca_initialized to 0.
> >
> >"the setsockopt" is rather vague, sorry.
> >
> >
> >The hard part is that all scenarios have to be considered.
> >
> >TCP flows can either be passive and active.
> >
> >CC can be set :
> >
> >1) Before the connect() or accept()
> >2) After the connect() or accept()
> >3) after the connect() but before 3WHS is completed.
> >
> >So we need to make sure all cases will still work with any combination
> >of CDG CC (before/after) in the picture.
> >
> >Note that a memory leak for a restricted CC (CDG can only be used by
> >CAP_NET_ADMIN privileged user)
> > is a small problem compared to more serious bug that could be added
> >by an incomplete fix.
> >
> >I also note that if icsk_ca_priv] was increased from 104 to 120 bytes,
> >tcp_cdg would no longer need a dynamic memory allocation.
> >
> >Thank you.
>
> Hi,
> I will try to see whether I am able to get the full sequence. I am also affraid of making a change that could affect big part of the kernel.
> About CDG, how we can get rid of dynamic allocation by increasing icsk_priv_data to 120? because I see that the window size is a module parameter, so I guess it is not a fixed value.
Given this module parameter is constant, I doubt anyone really uses a
bigger window.
If researchers want to experiment bigger window, they could adjust a
macro and recompile (#define TCP_CDG_WINDOW 8 -> X)
> Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
I think icsk_ca_initialized lost its strong meaning when CDG was
introduced (since this is the only CC allocating memory)
The bug really is that before clearing icsk_ca_initialized we should
call cc->release()
Maybe we missed this cleanup in commit
8919a9b31eb4fb4c0a93e5fb350a626924302aa6 ("tcp: Only init congestion
control if not initialized already")
Although I am not sure what happens at accept() time when the listener
socket is cloned.
If we make any hypothesis, we need to check all CC modules to make
sure they respect it.
>
> Thank you.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-29 12:58 ` Eric Dumazet via Linux-kernel-mentees
@ 2021-06-29 15:59 ` Neal Cardwell via Linux-kernel-mentees
-1 siblings, 0 replies; 31+ messages in thread
From: Neal Cardwell @ 2021-06-29 15:59 UTC (permalink / raw)
To: Eric Dumazet
Cc: Nguyen Dinh Phi, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
On Tue, Jun 29, 2021 at 8:58 AM Eric Dumazet <edumazet@google.com> wrote:
> > Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
>
> I think icsk_ca_initialized lost its strong meaning when CDG was
> introduced (since this is the only CC allocating memory)
>
> The bug really is that before clearing icsk_ca_initialized we should
> call cc->release()
>
> Maybe we missed this cleanup in commit
> 8919a9b31eb4fb4c0a93e5fb350a626924302aa6 ("tcp: Only init congestion
> control if not initialized already")
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
So it seems my previously suggested change (yesterday in this thread)
to add icsk_ca_initialized=0 in tcp_ca_openreq_child() is not needed.
> If we make any hypothesis, we need to check all CC modules to make
> sure they respect it.
AFAICT the fix is correct; it just needs a Fixes: tag and a more clear
description in the commit message.
I have cherry-picked the patch into our kernel and verified it passes
all of our internal packetdrill tests.
So the diff seems OK, but I would suggest a commit message something
like the following:
--
[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).
Fixes: 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>
--
neal
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-29 15:59 ` Neal Cardwell via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Neal Cardwell via Linux-kernel-mentees @ 2021-06-29 15:59 UTC (permalink / raw)
To: Eric Dumazet
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Yuchung Cheng, kpsingh, Jakub Kicinski, bpf,
linux-kernel-mentees, David Miller, LKML
On Tue, Jun 29, 2021 at 8:58 AM Eric Dumazet <edumazet@google.com> wrote:
> > Because the problem only happens with CDG, is adding check in its tcp_cdg_init() function Ok? And about icsk_ca_initialized, Could I expect it to be 0 in CC's init functions?
>
> I think icsk_ca_initialized lost its strong meaning when CDG was
> introduced (since this is the only CC allocating memory)
>
> The bug really is that before clearing icsk_ca_initialized we should
> call cc->release()
>
> Maybe we missed this cleanup in commit
> 8919a9b31eb4fb4c0a93e5fb350a626924302aa6 ("tcp: Only init congestion
> control if not initialized already")
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
So it seems my previously suggested change (yesterday in this thread)
to add icsk_ca_initialized=0 in tcp_ca_openreq_child() is not needed.
> If we make any hypothesis, we need to check all CC modules to make
> sure they respect it.
AFAICT the fix is correct; it just needs a Fixes: tag and a more clear
description in the commit message.
I have cherry-picked the patch into our kernel and verified it passes
all of our internal packetdrill tests.
So the diff seems OK, but I would suggest a commit message something
like the following:
--
[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).
Fixes: 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>
--
neal
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-29 15:59 ` Neal Cardwell via Linux-kernel-mentees
@ 2021-06-30 18:25 ` Phi Nguyen
-1 siblings, 0 replies; 31+ messages in thread
From: Phi Nguyen @ 2021-06-30 18:25 UTC (permalink / raw)
To: Neal Cardwell, Eric Dumazet
Cc: David Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, John Fastabend, kpsingh, netdev,
LKML, bpf, linux-kernel-mentees, syzbot+f1e24a0594d4e3a895d3,
Yuchung Cheng
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)
> --
> [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?
Thank you.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-06-30 18:25 ` Phi Nguyen
0 siblings, 0 replies; 31+ messages in thread
From: Phi Nguyen @ 2021-06-30 18:25 UTC (permalink / raw)
To: Neal Cardwell, Eric Dumazet
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Yuchung Cheng, kpsingh, Jakub Kicinski, bpf,
linux-kernel-mentees, David Miller, LKML
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)
> --
> [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?
Thank you.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
2021-06-30 18:25 ` Phi Nguyen
@ 2021-07-01 14:23 ` Neal Cardwell via Linux-kernel-mentees
-1 siblings, 0 replies; 31+ messages in thread
From: Neal Cardwell @ 2021-07-01 14:23 UTC (permalink / raw)
To: Phi Nguyen
Cc: Eric Dumazet, David Miller, Hideaki YOSHIFUJI, David Ahern,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
kpsingh, netdev, LKML, bpf, linux-kernel-mentees,
syzbot+f1e24a0594d4e3a895d3, Yuchung Cheng
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] tcp: Do not reset the icsk_ca_initialized in tcp_init_transfer.
@ 2021-07-01 14:23 ` Neal Cardwell via Linux-kernel-mentees
0 siblings, 0 replies; 31+ messages in thread
From: Neal Cardwell via Linux-kernel-mentees @ 2021-07-01 14:23 UTC (permalink / raw)
To: Phi Nguyen
Cc: Song Liu, Martin KaFai Lau, syzbot+f1e24a0594d4e3a895d3,
Daniel Borkmann, Hideaki YOSHIFUJI, netdev, David Ahern,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Eric Dumazet, Yuchung Cheng, kpsingh, Jakub Kicinski, bpf,
linux-kernel-mentees, David Miller, LKML
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
^ permalink raw reply [flat|nested] 31+ messages in thread