* [PATCH net] tcp: fix wraparound issue in tcp_lp
@ 2017-05-01 22:29 Eric Dumazet
2017-05-01 23:56 ` Stephen Hemminger
2017-05-02 19:07 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2017-05-01 22:29 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
Be careful when comparing tcp_time_stamp to some u32 quantity,
otherwise result can be surprising.
Fixes: 7c106d7e782b ("[TCP]: TCP Low Priority congestion control")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_lp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
index 046fd3910873306d74207615d6997e1c847ea361..d6fb6c067af4641f232b94e7c590c212648e8173 100644
--- a/net/ipv4/tcp_lp.c
+++ b/net/ipv4/tcp_lp.c
@@ -264,13 +264,15 @@ static void tcp_lp_pkts_acked(struct sock *sk, const struct ack_sample *sample)
{
struct tcp_sock *tp = tcp_sk(sk);
struct lp *lp = inet_csk_ca(sk);
+ u32 delta;
if (sample->rtt_us > 0)
tcp_lp_rtt_sample(sk, sample->rtt_us);
/* calc inference */
- if (tcp_time_stamp > tp->rx_opt.rcv_tsecr)
- lp->inference = 3 * (tcp_time_stamp - tp->rx_opt.rcv_tsecr);
+ delta = tcp_time_stamp - tp->rx_opt.rcv_tsecr;
+ if ((s32)delta > 0)
+ lp->inference = 3 * delta;
/* test if within inference */
if (lp->last_drop && (tcp_time_stamp - lp->last_drop < lp->inference))
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] tcp: fix wraparound issue in tcp_lp
2017-05-01 22:29 [PATCH net] tcp: fix wraparound issue in tcp_lp Eric Dumazet
@ 2017-05-01 23:56 ` Stephen Hemminger
2017-05-02 0:31 ` Neal Cardwell
2017-05-02 1:04 ` Eric Dumazet
2017-05-02 19:07 ` David Miller
1 sibling, 2 replies; 6+ messages in thread
From: Stephen Hemminger @ 2017-05-01 23:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Mon, 01 May 2017 15:29:48 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Be careful when comparing tcp_time_stamp to some u32 quantity,
> otherwise result can be surprising.
>
> Fixes: 7c106d7e782b ("[TCP]: TCP Low Priority congestion control")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv4/tcp_lp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
> index 046fd3910873306d74207615d6997e1c847ea361..d6fb6c067af4641f232b94e7c590c212648e8173 100644
> --- a/net/ipv4/tcp_lp.c
> +++ b/net/ipv4/tcp_lp.c
> @@ -264,13 +264,15 @@ static void tcp_lp_pkts_acked(struct sock *sk, const struct ack_sample *sample)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> struct lp *lp = inet_csk_ca(sk);
> + u32 delta;
>
> if (sample->rtt_us > 0)
> tcp_lp_rtt_sample(sk, sample->rtt_us);
>
> /* calc inference */
> - if (tcp_time_stamp > tp->rx_opt.rcv_tsecr)
> - lp->inference = 3 * (tcp_time_stamp - tp->rx_opt.rcv_tsecr);
> + delta = tcp_time_stamp - tp->rx_opt.rcv_tsecr;
> + if ((s32)delta > 0)
> + lp->inference = 3 * delta;
Agreed time wraparound would cause problems.
But why not use existing time_after() macro here?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] tcp: fix wraparound issue in tcp_lp
2017-05-01 23:56 ` Stephen Hemminger
@ 2017-05-02 0:31 ` Neal Cardwell
2017-05-02 1:04 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Neal Cardwell @ 2017-05-02 0:31 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Eric Dumazet, David Miller, netdev
On Mon, May 1, 2017 at 7:56 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Mon, 01 May 2017 15:29:48 -0700
> Agreed time wraparound would cause problems.
> But why not use existing time_after() macro here?
>
I suspect this is because time_after() asserts that it is being used
on unsigned long (64 bits), and tcp_time_stamp is 32 bits.
I suppose for tcp_time_stamp comparisons we could re-use the u32 TCP
sequence macros for before() and after()? Even the comment for
before()/after() is already generic enough to apply to tcp_time_stamp:
"The next routines deal with comparing 32 bit unsigned ints and worry
about wraparound (automatic with unsigned arithmetic)." That might be
nice.
neal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tcp: fix wraparound issue in tcp_lp
2017-05-01 23:56 ` Stephen Hemminger
2017-05-02 0:31 ` Neal Cardwell
@ 2017-05-02 1:04 ` Eric Dumazet
2017-05-02 1:58 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-05-02 1:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
On Mon, 2017-05-01 at 16:56 -0700, Stephen Hemminger wrote:
> On Mon, 01 May 2017 15:29:48 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Be careful when comparing tcp_time_stamp to some u32 quantity,
> > otherwise result can be surprising.
> >
> > Fixes: 7c106d7e782b ("[TCP]: TCP Low Priority congestion control")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> > net/ipv4/tcp_lp.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
> > index 046fd3910873306d74207615d6997e1c847ea361..d6fb6c067af4641f232b94e7c590c212648e8173 100644
> > --- a/net/ipv4/tcp_lp.c
> > +++ b/net/ipv4/tcp_lp.c
> > @@ -264,13 +264,15 @@ static void tcp_lp_pkts_acked(struct sock *sk, const struct ack_sample *sample)
> > {
> > struct tcp_sock *tp = tcp_sk(sk);
> > struct lp *lp = inet_csk_ca(sk);
> > + u32 delta;
> >
> > if (sample->rtt_us > 0)
> > tcp_lp_rtt_sample(sk, sample->rtt_us);
> >
> > /* calc inference */
> > - if (tcp_time_stamp > tp->rx_opt.rcv_tsecr)
> > - lp->inference = 3 * (tcp_time_stamp - tp->rx_opt.rcv_tsecr);
> > + delta = tcp_time_stamp - tp->rx_opt.rcv_tsecr;
> > + if ((s32)delta > 0)
> > + lp->inference = 3 * delta;
>
> Agreed time wraparound would cause problems.
> But why not use existing time_after() macro here?
>
Simply to not perform (tcp_time_stamp - tp->rx_opt.rcv_tsecr) twice.
jiffies being volatile, this can not be optimized by the compiler.
I have a patch series (for linux-4.13) that will switch TCP stack to 1ms
TS options, regardless of CONFIG_HZ value, and when cooking it I found
this bug.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] tcp: fix wraparound issue in tcp_lp
2017-05-02 1:04 ` Eric Dumazet
@ 2017-05-02 1:58 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2017-05-02 1:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
On Mon, 2017-05-01 at 18:04 -0700, Eric Dumazet wrote:
>
> Simply to not perform (tcp_time_stamp - tp->rx_opt.rcv_tsecr) twice.
>
> jiffies being volatile, this can not be optimized by the compiler.
>
> I have a patch series (for linux-4.13) that will switch TCP stack to 1ms
> TS options, regardless of CONFIG_HZ value, and when cooking it I found
> this bug.
I forgot to say that after this upcoming patch series, tcp_time_stamp
will become a more expensive function, no longer a plain (u32)jiffies.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tcp: fix wraparound issue in tcp_lp
2017-05-01 22:29 [PATCH net] tcp: fix wraparound issue in tcp_lp Eric Dumazet
2017-05-01 23:56 ` Stephen Hemminger
@ 2017-05-02 19:07 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2017-05-02 19:07 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 01 May 2017 15:29:48 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> Be careful when comparing tcp_time_stamp to some u32 quantity,
> otherwise result can be surprising.
>
> Fixes: 7c106d7e782b ("[TCP]: TCP Low Priority congestion control")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-02 19:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-01 22:29 [PATCH net] tcp: fix wraparound issue in tcp_lp Eric Dumazet
2017-05-01 23:56 ` Stephen Hemminger
2017-05-02 0:31 ` Neal Cardwell
2017-05-02 1:04 ` Eric Dumazet
2017-05-02 1:58 ` Eric Dumazet
2017-05-02 19:07 ` David Miller
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).