Message ID | 1495578286.6465.90.camel@edumazet-glaptop3.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 23 May 2017 15:24:46 -0700 > Add a FLAG_NO_CHALLENGE_ACK so that tcp_rcv_state_process() > can choose to send a challenge ACK and discard the packet instead > of wrongly change socket state. Applied, but the tests end up being double-negatives so it might have been easier to understand if the flag was a positive rather than a negative value.
On Thu, 2017-05-25 at 12:48 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 23 May 2017 15:24:46 -0700 > > > Add a FLAG_NO_CHALLENGE_ACK so that tcp_rcv_state_process() > > can choose to send a challenge ACK and discard the packet instead > > of wrongly change socket state. > > Applied, but the tests end up being double-negatives so it might > have been easier to understand if the flag was a positive rather > than a negative value. I thought of this (and was in fact one of the patch I sent for internal review at Google), but this was changing all tcp_ack() calls instead of a single one ? Or maybe I am missing some easier way ? Thanks
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 25 May 2017 09:50:51 -0700 > On Thu, 2017-05-25 at 12:48 -0400, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Tue, 23 May 2017 15:24:46 -0700 >> >> > Add a FLAG_NO_CHALLENGE_ACK so that tcp_rcv_state_process() >> > can choose to send a challenge ACK and discard the packet instead >> > of wrongly change socket state. >> >> Applied, but the tests end up being double-negatives so it might >> have been easier to understand if the flag was a positive rather >> than a negative value. > > I thought of this (and was in fact one of the patch I sent for internal > review at Google), but this was changing all tcp_ack() calls instead of > a single one ? > > Or maybe I am missing some easier way ? Indeed, it is a bit of churn to adjust all callers in order to make one test easier to read. I'm not so sure it's better or worth it...
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2fa55f57ac06584bfd9b555799ceb3bbfb7e1b4e..c3bdcbcf544793ba410c618130586bf7d3963da6 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -112,6 +112,7 @@ int sysctl_tcp_invalid_ratelimit __read_mostly = HZ/2; #define FLAG_DSACKING_ACK 0x800 /* SACK blocks contained D-SACK info */ #define FLAG_SACK_RENEGING 0x2000 /* snd_una advanced to a sacked seq */ #define FLAG_UPDATE_TS_RECENT 0x4000 /* tcp_replace_ts_recent() */ +#define FLAG_NO_CHALLENGE_ACK 0x8000 /* do not call tcp_send_challenge_ack() */ #define FLAG_ACKED (FLAG_DATA_ACKED|FLAG_SYN_ACKED) #define FLAG_NOT_DUP (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED) @@ -3568,7 +3569,8 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) if (before(ack, prior_snd_una)) { /* RFC 5961 5.2 [Blind Data Injection Attack].[Mitigation] */ if (before(ack, prior_snd_una - tp->max_window)) { - tcp_send_challenge_ack(sk, skb); + if (!(flag & FLAG_NO_CHALLENGE_ACK)) + tcp_send_challenge_ack(sk, skb); return -1; } goto old_ack; @@ -5951,13 +5953,17 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) /* step 5: check the ACK field */ acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH | - FLAG_UPDATE_TS_RECENT) > 0; + FLAG_UPDATE_TS_RECENT | + FLAG_NO_CHALLENGE_ACK) > 0; + if (!acceptable) { + if (sk->sk_state == TCP_SYN_RECV) + return 1; /* send one RST */ + tcp_send_challenge_ack(sk, skb); + goto discard; + } switch (sk->sk_state) { case TCP_SYN_RECV: - if (!acceptable) - return 1; - if (!tp->srtt_us) tcp_synack_rtt_meas(sk, req); @@ -6026,14 +6032,6 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) * our SYNACK so stop the SYNACK timer. */ if (req) { - /* Return RST if ack_seq is invalid. - * Note that RFC793 only says to generate a - * DUPACK for it but for TCP Fast Open it seems - * better to treat this case like TCP_SYN_RECV - * above. - */ - if (!acceptable) - return 1; /* We no longer need the request sock. */ reqsk_fastopen_remove(sk, req, false); tcp_rearm_rto(sk);