On 11/1/21 9:22 PM, Francesco Ruggeri wrote:
+/* Compute SNE for a specific packet (by seq). */
+static int compute_packet_sne(struct sock *sk, struct tcp_authopt_info *info,
+ u32 seq, bool input, __be32 *sne)
+{
+ u32 rcv_nxt, snd_nxt;
+
+ // We can't use normal SNE computation before reaching TCP_ESTABLISHED
+ // For TCP_SYN_SENT the dst_isn field is initialized only after we
+ // validate the remote SYN/ACK
+ // For TCP_NEW_SYN_RECV there is no tcp_authopt_info at all
+ if (sk->sk_state == TCP_SYN_SENT ||
+ sk->sk_state == TCP_NEW_SYN_RECV ||
+ sk->sk_state == TCP_LISTEN)
+ return 0;
+
In case of TCP_NEW_SYN_RECV, if our SYNACK had sequence number
0xffffffff, we will receive an ACK sequence number of 0, which
should have sne = 1.
In a somewhat similar corner case, when we receive a SYNACK to
our SYN in tcp_rcv_synsent_state_process, if the SYNACK has
sequence number 0xffffffff, we set tp->rcv_nxt to 0, and we
should set sne to 1.
There may be more similar corner cases related to a wraparound
during the handshake.
Since as you pointed out all we need is "recent" valid <sne, seq>
pairs as reference, rather than relying on rcv_sne being paired
with tp->rcv_nxt (and similarly for snd_sne and tp->snd_nxt),
would it be easier to maintain reference <sne, seq> pairs for send
and receive in tcp_authopt_info, appropriately handle the different
handshake cases and initialize the pairs, and only then track them
in tcp_rcv_nxt_update and tcp_rcv_snd_update?
For TCP_NEW_SYN_RECV there is no struct tcp_authopt_info, only a request
minisock. I think those are deliberately kept small save resources on
SYN floods so I'd rather not increase their size.
For all the handshake cases we can just rely on SNE=0 for ISN and we
already need to keep track of ISNs because they're part of the signature.
I'll need to test handshake seq 0xFFFFFFFF deliberately, you're right
that it can fail.
static void tcp_rcv_nxt_update(struct tcp_sock *tp, u32 seq)
{
u32 delta = seq - tp->rcv_nxt;
sock_owned_by_me((struct sock *)tp);
+ tcp_authopt_update_rcv_sne(tp, seq);
tp->bytes_received += delta;
WRITE_ONCE(tp->rcv_nxt, seq);
}
Since rcv_sne and tp->rcv_nxt are not updated atomically, could
there ever be a case where a reader might use the new sne with
the old rcv_nxt?
As far as I understand if all of the read and writes to SNE happen under
the socket lock it should be fine. I don't know why WRITE_ONCE is used
here, maybe somebody else wants to read rcv_nxt outside the socket lock?
That doesn't matter for SNE.
I think the only case would be sending ipv4 RSTs outside the socket.
--
Regards,
Leonard