tcp: implement throttling of challenge ACKs

[tuexen]

Summary

Implement ACK throttling of challenge ACKs as described in RFC 5961.

[cc]

sys/netinet/tcp_subr.c

2186

typo: per TCP connections ==> per TCP connection

2194–2196

If ACK war protection is disabled, are we going to send challenge ACK or not?

I thought if ACK war protection is disabled, we should send unlimited challenge ACKs.

reference:
ctf_ack_war_checks()

[tuexen]

Address issues raised by cc@.

sys/netinet/tcp_subr.c

2186

Fixed.

2194–2196

Fixed.

[cc]

sys/netinet/tcp_subr.c

2194

A little bit less than perfect, I think. For example, tcp_ack_war_time_window requires a zero check within the epoch check. It can be like this:

	if (tcp_ack_war_time_window > 0 && tcp_ack_war_cnt > 0) {
		// check rate limit
		// if under limit, pass through; else, just return
	} 
	// send challenge ACK

[tuexen]

sys/netinet/tcp_subr.c

2194

I am using the negation of your condition. So were is the difference?

[cc]

sys/netinet/tcp_subr.c

2194

I am using the negation of your condition. So were is the difference?

On a second thought, you are right.

[cc]

sys/netinet/tcp_subr.c

2194

Still, I recommend the cleaner approach, which saves the local variable send_challenge_ack and the else branch.

[cc]

Just saw https://reviews.freebsd.org/D46068. If this patch has not been committed, we can still further revise it, so that D46068 can be cleaner on function re-use.

I think the logic here can be split into two functions, such that the refined logic 1 on checking if we should send ACK can be re-used across multiple places.

logic 1:

bool
is_ack_unlimited(struct tcpcb *tp)
{
    /* focus on checking the epoch and
     * if should send ACK, return true; else return false
     */ 
}

logic 2:

void
tcp_send_challenge_ack(struct tcpcb *tp, struct tcphdr *th, struct mbuf *m)
{
    if (is_ack_unlimited(tp)) {.
       tcp_respond();
    ....
    }
}

[tuexen]

In D46066#1050919, @cc wrote:

Just saw https://reviews.freebsd.org/D46068. If this patch has not been committed, we can still further revise it, so that D46068 can be cleaner on function re-use.

I think the logic here can be split into two functions, such that the refined logic 1 on checking if we should send ACK can be re-used across multiple places.

logic 1:

bool
is_ack_unlimited(struct tcpcb *tp)
{
    /* focus on checking the epoch and
     * if should send ACK, return true; else return false
     */ 
}

logic 2:

void
tcp_send_challenge_ack(struct tcpcb *tp, struct tcphdr *th, struct mbuf *m)
{
    if (is_ack_unlimited(tp)) {.
       tcp_respond();
    ....
    }
}

I don't get the point. Eventually, all places should use tcp_send_challenge_ack(), I think. But that is multiple commits away.

[cc]

In D46066#1051183, @tuexen wrote:

In D46066#1050919, @cc wrote:

Just saw https://reviews.freebsd.org/D46068. If this patch has not been committed, we can still further revise it, so that D46068 can be cleaner on function re-use.

I think the logic here can be split into two functions, such that the refined logic 1 on checking if we should send ACK can be re-used across multiple places.

logic 1:

bool
is_ack_unlimited(struct tcpcb *tp)
{
    /* focus on checking the epoch and
     * if should send ACK, return true; else return false
     */ 
}

logic 2:

void
tcp_send_challenge_ack(struct tcpcb *tp, struct tcphdr *th, struct mbuf *m)
{
    if (is_ack_unlimited(tp)) {.
       tcp_respond();
    ....
    }
}

I don't get the point. Eventually, all places should use tcp_send_challenge_ack(), I think. But that is multiple commits away.

For example, ctf_ack_war_checks() can be simply refined as this:

ctf_ack_war_checks()
{
    if (is_ack_unlimited(tp)) {
        tp->t_flags |= TF_ACKNOW;
    } else {
        tp->t_flags &= ~TF_ACKNOW;
    }
}