[PATCH net-next 0/2] tcp: address two poll() flakes

[PATCH net-next 0/2] tcp: address two poll() flakes

[Eric Dumazet]

Some packetdrill tests are failing when host kernel is using ASAN
or other debugging infrastructure.

I was able to fix the flakes by making sure we were not
sending wakeup events too soon.

Eric Dumazet (2):
  tcp: remove poll() flakes when receiving RST
  tcp: remove poll() flakes with FastOpen

 net/ipv4/tcp_input.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.12.2.762.g0e3151a226-goog

[PATCH net-next 1/2] tcp: remove poll() flakes when receiving RST

[Eric Dumazet]

When a RST packet is processed, we send two wakeup events to interested
polling users.

First one by a sk->sk_error_report(sk) from tcp_reset(),
followed by a sk->sk_state_change(sk) from tcp_done().

Depending on machine load and luck, poll() can either return POLLERR,
or POLLIN|POLLOUT|POLLERR|POLLHUP (this happens on 99 % of the cases)

This is probably fine, but we can avoid the confusion by reordering
things so that we have more TCP fields updated before the first wakeup.

This might even allow us to remove some barriers we added in the past.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a5838858c362cd3270296001ceaae341e9e9bf01..37e2aa925f62395cfb48145cd3a76b6afebb64b1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4008,10 +4008,10 @@ void tcp_reset(struct sock *sk)
 	/* This barrier is coupled with smp_rmb() in tcp_poll() */
 	smp_wmb();
 
+	tcp_done(sk);
+
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_error_report(sk);
-
-	tcp_done(sk);
 }
 
 /*
-- 
2.12.2.762.g0e3151a226-goog

Re: [PATCH net-next 1/2] tcp: remove poll() flakes when receiving RST

[Soheil Hassas Yeganeh]

On Tue, Apr 18, 2017 at 12:45 PM, Eric Dumazet <edumazet@google.com> wrote:
> When a RST packet is processed, we send two wakeup events to interested
> polling users.
>
> First one by a sk->sk_error_report(sk) from tcp_reset(),
> followed by a sk->sk_state_change(sk) from tcp_done().
>
> Depending on machine load and luck, poll() can either return POLLERR,
> or POLLIN|POLLOUT|POLLERR|POLLHUP (this happens on 99 % of the cases)
>
> This is probably fine, but we can avoid the confusion by reordering
> things so that we have more TCP fields updated before the first wakeup.
>
> This might even allow us to remove some barriers we added in the past.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

> ---
>  net/ipv4/tcp_input.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index a5838858c362cd3270296001ceaae341e9e9bf01..37e2aa925f62395cfb48145cd3a76b6afebb64b1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4008,10 +4008,10 @@ void tcp_reset(struct sock *sk)
>         /* This barrier is coupled with smp_rmb() in tcp_poll() */
>         smp_wmb();
>
> +       tcp_done(sk);
> +
>         if (!sock_flag(sk, SOCK_DEAD))
>                 sk->sk_error_report(sk);
> -
> -       tcp_done(sk);
>  }
>
>  /*
> --
> 2.12.2.762.g0e3151a226-goog
>

Thanks, Eric. Nice improvement!

[PATCH net-next 2/2] tcp: remove poll() flakes with FastOpen

[Eric Dumazet]

When using TCP FastOpen for an active session, we send one wakeup event
from tcp_finish_connect(), right before the data eventually contained in
the received SYNACK is queued to sk->sk_receive_queue.

This means that depending on machine load or luck, poll() users
might receive POLLOUT events instead of POLLIN|POLLOUT

To fix this, we need to move the call to sk->sk_state_change()
after the (optional) call to tcp_rcv_fastopen_synack()

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_input.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 37e2aa925f62395cfb48145cd3a76b6afebb64b1..341f021f02a2931cd75b2e1e71af9729fc4c7895 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5580,10 +5580,6 @@ void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
 	else
 		tp->pred_flags = 0;
 
-	if (!sock_flag(sk, SOCK_DEAD)) {
-		sk->sk_state_change(sk);
-		sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
-	}
 }
 
 static bool tcp_rcv_fastopen_synack(struct sock *sk, struct sk_buff *synack,
@@ -5652,6 +5648,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_fastopen_cookie foc = { .len = -1 };
 	int saved_clamp = tp->rx_opt.mss_clamp;
+	bool fastopen_fail;
 
 	tcp_parse_options(skb, &tp->rx_opt, 0, &foc);
 	if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr)
@@ -5755,10 +5752,15 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 
 		tcp_finish_connect(sk, skb);
 
-		if ((tp->syn_fastopen || tp->syn_data) &&
-		    tcp_rcv_fastopen_synack(sk, skb, &foc))
-			return -1;
+		fastopen_fail = (tp->syn_fastopen || tp->syn_data) &&
+				tcp_rcv_fastopen_synack(sk, skb, &foc);
 
+		if (!sock_flag(sk, SOCK_DEAD)) {
+			sk->sk_state_change(sk);
+			sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
+		}
+		if (fastopen_fail)
+			return -1;
 		if (sk->sk_write_pending ||
 		    icsk->icsk_accept_queue.rskq_defer_accept ||
 		    icsk->icsk_ack.pingpong) {
-- 
2.12.2.762.g0e3151a226-goog

Re: [PATCH net-next 2/2] tcp: remove poll() flakes with FastOpen

[Yuchung Cheng]

On Tue, Apr 18, 2017 at 9:45 AM, Eric Dumazet <edumazet@google.com> wrote:
>
> When using TCP FastOpen for an active session, we send one wakeup event
> from tcp_finish_connect(), right before the data eventually contained in
> the received SYNACK is queued to sk->sk_receive_queue.
>
> This means that depending on machine load or luck, poll() users
> might receive POLLOUT events instead of POLLIN|POLLOUT
>
> To fix this, we need to move the call to sk->sk_state_change()
> after the (optional) call to tcp_rcv_fastopen_synack()
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>

Thanks for the fix!

> ---
>  net/ipv4/tcp_input.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 37e2aa925f62395cfb48145cd3a76b6afebb64b1..341f021f02a2931cd75b2e1e71af9729fc4c7895 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5580,10 +5580,6 @@ void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
>         else
>                 tp->pred_flags = 0;
>
> -       if (!sock_flag(sk, SOCK_DEAD)) {
> -               sk->sk_state_change(sk);
> -               sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
> -       }
>  }
>
>  static bool tcp_rcv_fastopen_synack(struct sock *sk, struct sk_buff *synack,
> @@ -5652,6 +5648,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
>         struct tcp_sock *tp = tcp_sk(sk);
>         struct tcp_fastopen_cookie foc = { .len = -1 };
>         int saved_clamp = tp->rx_opt.mss_clamp;
> +       bool fastopen_fail;
>
>         tcp_parse_options(skb, &tp->rx_opt, 0, &foc);
>         if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr)
> @@ -5755,10 +5752,15 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
>
>                 tcp_finish_connect(sk, skb);
>
> -               if ((tp->syn_fastopen || tp->syn_data) &&
> -                   tcp_rcv_fastopen_synack(sk, skb, &foc))
> -                       return -1;
> +               fastopen_fail = (tp->syn_fastopen || tp->syn_data) &&
> +                               tcp_rcv_fastopen_synack(sk, skb, &foc);
>
> +               if (!sock_flag(sk, SOCK_DEAD)) {
> +                       sk->sk_state_change(sk);
> +                       sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
> +               }
> +               if (fastopen_fail)
> +                       return -1;
>                 if (sk->sk_write_pending ||
>                     icsk->icsk_accept_queue.rskq_defer_accept ||
>                     icsk->icsk_ack.pingpong) {
> --
> 2.12.2.762.g0e3151a226-goog
>

Re: [PATCH net-next 0/2] tcp: address two poll() flakes

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 18 Apr 2017 09:45:50 -0700

> Some packetdrill tests are failing when host kernel is using ASAN
> or other debugging infrastructure.
> 
> I was able to fix the flakes by making sure we were not
> sending wakeup events too soon.

Series applied, thanks Eric.