linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized
@ 2021-07-03  9:34 Nguyen Dinh Phi
  2021-07-03 21:49 ` David Miller
  0 siblings, 1 reply; 4+ messages in thread
From: Nguyen Dinh Phi @ 2021-07-03  9:34 UTC (permalink / raw)
  To: yhs, edumazet, davem, yoshfuji, dsahern, kuba, ast, daniel,
	andrii, kafai, songliubraving, john.fastabend, kpsingh, ycheng,
	ncardwell, yyd
  Cc: Nguyen Dinh Phi, netdev, linux-kernel, bpf, linux-kernel-mentees,
	syzbot+f1e24a0594d4e3a895d3

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: 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.
V3:	- Fix netdev/verify_fixes format error.
V4:	- Add blamed authors to receiver list.

 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;
 	bpf_skops_established(sk, bpf_op, skb);
 	if (!icsk->icsk_ca_initialized)
 		tcp_init_congestion_control(sk);
--
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized
  2021-07-03  9:34 [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized Nguyen Dinh Phi
@ 2021-07-03 21:49 ` David Miller
  2021-07-04 14:03   ` Neal Cardwell
       [not found]   ` <CADVnQynxFKthexWRFRGN_9enRt7cmgrNo7mpNOMpNVm_jJpt4w@mail.gmail.com>
  0 siblings, 2 replies; 4+ messages in thread
From: David Miller @ 2021-07-03 21:49 UTC (permalink / raw)
  To: phind.uet
  Cc: yhs, edumazet, yoshfuji, dsahern, kuba, ast, daniel, andrii,
	kafai, songliubraving, john.fastabend, kpsingh, ycheng,
	ncardwell, yyd, netdev, linux-kernel, bpf, linux-kernel-mentees,
	syzbot+f1e24a0594d4e3a895d3

From: Nguyen Dinh Phi <phind.uet@gmail.com>
Date: Sat,  3 Jul 2021 17:34:17 +0800

> 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);

Don't you have to make the tcp_init_congestion_control() call unconditional now?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized
  2021-07-03 21:49 ` David Miller
@ 2021-07-04 14:03   ` Neal Cardwell
       [not found]   ` <CADVnQynxFKthexWRFRGN_9enRt7cmgrNo7mpNOMpNVm_jJpt4w@mail.gmail.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Neal Cardwell @ 2021-07-04 14:03 UTC (permalink / raw)
  To: David Miller
  Cc: phind.uet, yhs, edumazet, yoshfuji, dsahern, kuba, ast, daniel,
	andrii, kafai, songliubraving, john.fastabend, kpsingh, ycheng,
	yyd, netdev, linux-kernel, bpf, linux-kernel-mentees,
	syzbot+f1e24a0594d4e3a895d3

On Sat, Jul 3, 2021 at 5:49 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nguyen Dinh Phi <phind.uet@gmail.com>
> Date: Sat,  3 Jul 2021 17:34:17 +0800
>
> > 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);
>
> Don't you have to make the tcp_init_congestion_control() call unconditional now?

I think we want to keep it conditional, to avoid double-initialization
if the BPF code sets the congestion control algorithm and initializes
it. But that's relatively new and subtle, so it might be nice for this
patch to add a comment about that, since it's touching this part of
the code anyway:

-       icsk->icsk_ca_initialized = 0;
        bpf_skops_established(sk, bpf_op, skb);
+       /* Initialize congestion control unless BPF initialized it already: */
        if (!icsk->icsk_ca_initialized)
                tcp_init_congestion_control(sk);

neal

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized
       [not found]   ` <CADVnQynxFKthexWRFRGN_9enRt7cmgrNo7mpNOMpNVm_jJpt4w@mail.gmail.com>
@ 2021-07-05 16:34     ` Phi Nguyen
  0 siblings, 0 replies; 4+ messages in thread
From: Phi Nguyen @ 2021-07-05 16:34 UTC (permalink / raw)
  To: Neal Cardwell, David Miller
  Cc: yhs, edumazet, yoshfuji, dsahern, kuba, ast, daniel, andrii,
	kafai, songliubraving, john.fastabend, kpsingh, ycheng, yyd,
	netdev, linux-kernel, bpf, linux-kernel-mentees,
	syzbot+f1e24a0594d4e3a895d3

On 7/4/2021 9:52 PM, Neal Cardwell wrote:
> 
> 
> On Sat, Jul 3, 2021 at 5:49 PM David Miller <davem@davemloft.net 
> <mailto:davem@davemloft.net>> wrote:
>  >
>  > From: Nguyen Dinh Phi <phind.uet@gmail.com <mailto:phind.uet@gmail.com>>
>  > Date: Sat,  3 Jul 2021 17:34:17 +0800
>  >
>  > > 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);
>  >
>  > Don't you have to make the tcp_init_congestion_control() call 
> unconditional now?
> 
> I think we want to keep it conditional, to avoid double-initialization 
> if the BPF code sets the congestion control algorithm and initializes 
> it. But that's relatively new and subtle, so it might be nice for this 
> patch to add a comment about that, since it's touching this part of the 
> code anyway:
> 
> -       icsk->icsk_ca_initialized = 0;
>          bpf_skops_established(sk, bpf_op, skb);
> +       /* Initialize congestion control unless a BPF op initialized it 
> already: */
>          if (!icsk->icsk_ca_initialized)
>                  tcp_init_congestion_control(sk);
> 
> neal
> 
I will make a new version with your comment.
Thank you very much for helping.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-05 16:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-03  9:34 [PATCH v4] tcp: fix tcp_init_transfer() to not reset icsk_ca_initialized Nguyen Dinh Phi
2021-07-03 21:49 ` David Miller
2021-07-04 14:03   ` Neal Cardwell
     [not found]   ` <CADVnQynxFKthexWRFRGN_9enRt7cmgrNo7mpNOMpNVm_jJpt4w@mail.gmail.com>
2021-07-05 16:34     ` Phi Nguyen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).