From 219e962832367963b92cc27e7c8687db4048cfb7 Mon Sep 17 00:00:00 2001 From: Simon Sundberg Date: Fri, 12 Feb 2021 18:31:30 +0100 Subject: [PATCH] pping: Avoid timestamping pure TCP ACKs Add a parsing_context struct to keep track data, data_end and currently parsed position, as well as handling the difference between data_end for XDP and TC through data_end_end pointer. Use parsing_context struct to detect pure TCP ACKs, and avoid creating identifier for them on egress (to avoid creating timestamp entries). This solves issue of calculating RTTs in inproper contexts. Signed-off-by: Simon Sundberg --- pping/pping_helpers.h | 41 +++++++++++++++++++++++++++++------------ pping/pping_kern_tc.c | 9 ++++++--- pping/pping_kern_xdp.c | 9 ++++++--- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/pping/pping_helpers.h b/pping/pping_helpers.h index 5dc7971..5e54857 100644 --- a/pping/pping_helpers.h +++ b/pping/pping_helpers.h @@ -18,6 +18,21 @@ #define AF_INET6 10 #define MAX_TCP_OPTIONS 10 +/* + * This struct keeps track of the data and data_end pointers from the xdp_md or + * __skb_buff contexts, as well as a currently parsed to position kept in nh. + * The member data_end_end pointer which has the adress that would correspond + * to the logical end of the packet (for XDP, this is the same as data_end, + * but for TC this is skb->data + skb->len), which is useful for determining + * how much data a certain header encloses. + */ +struct parsing_context { + void *data; //Start of eth hdr + void *data_end; //End of safe acessible area + void *data_end_end; //Logical end of packet + struct hdr_cursor nh; //Position to parse next +}; + /* * Maps and IPv4 address into an IPv6 address according to RFC 4291 sec 2.5.5.2 */ @@ -85,18 +100,21 @@ static int parse_tcp_ts(struct tcphdr *tcph, void *data_end, __u32 *tsval, * and dest, respectively, and 0 will be returned. On failure, -1 will be * returned. */ -// Has 6 parameters (one too many) -static int parse_tcp_identifier(struct hdr_cursor *nh, void *data_end, - bool is_egress, struct flow_address *saddr, +static int parse_tcp_identifier(struct parsing_context *ctx, bool is_egress, + struct flow_address *saddr, struct flow_address *daddr, __u32 *identifier) { __u32 tsval, tsecr; struct tcphdr *tcph; - if (parse_tcphdr(nh, data_end, &tcph) < 0) + if (parse_tcphdr(&ctx->nh, ctx->data_end, &tcph) < 0) return -1; - if (parse_tcp_ts(tcph, data_end, &tsval, &tsecr) < 0) + // Do not timestamp pure ACKs + if (is_egress && ctx->data_end_end <= ctx->nh.pos && !tcph->syn) + return -1; + + if (parse_tcp_ts(tcph, ctx->data_end, &tsval, &tsecr) < 0) return -1; //Possible TODO, fall back on seq/ack instead saddr->port = tcph->source; @@ -116,11 +134,10 @@ static int parse_tcp_identifier(struct hdr_cursor *nh, void *data_end, * destination and source of packet, respectively), and identifier will be * set to the identifier of a response. */ -static int parse_packet_identifier(void *data, void *data_end, bool is_egress, +static int parse_packet_identifier(struct parsing_context *ctx, bool is_egress, struct packet_id *p_id) { int proto, err; - struct hdr_cursor nh = { .pos = data }; struct ethhdr *eth; struct iphdr *iph; struct ipv6hdr *ip6h; @@ -135,23 +152,23 @@ static int parse_packet_identifier(void *data, void *data_end, bool is_egress, daddr = &p_id->flow.saddr; } - proto = parse_ethhdr(&nh, data_end, ð); + proto = parse_ethhdr(&ctx->nh, ctx->data_end, ð); // Parse IPv4/6 header if (proto == bpf_htons(ETH_P_IP)) { p_id->flow.ipv = AF_INET; - proto = parse_iphdr(&nh, data_end, &iph); + proto = parse_iphdr(&ctx->nh, ctx->data_end, &iph); } else if (proto == bpf_htons(ETH_P_IPV6)) { p_id->flow.ipv = AF_INET6; - proto = parse_ip6hdr(&nh, data_end, &ip6h); + proto = parse_ip6hdr(&ctx->nh, ctx->data_end, &ip6h); } else { return -1; } // Add new protocols here if (proto == IPPROTO_TCP) { - err = parse_tcp_identifier(&nh, data_end, is_egress, saddr, - daddr, &p_id->identifier); + err = parse_tcp_identifier(ctx, is_egress, saddr, daddr, + &p_id->identifier); if (err) return -1; } else { diff --git a/pping/pping_kern_tc.c b/pping/pping_kern_tc.c index 30d94dd..d46094e 100644 --- a/pping/pping_kern_tc.c +++ b/pping/pping_kern_tc.c @@ -31,13 +31,16 @@ struct bpf_elf_map SEC("maps") ts_start = { SEC(TCBPF_PROG_SEC) int tc_bpf_prog_egress(struct __sk_buff *skb) { + struct parsing_context pctx; struct packet_id p_id = { 0 }; struct packet_timestamp p_ts = { 0 }; - void *data = (void *)(long)skb->data; - void *data_end = (void *)(long)skb->data_end; + pctx.data = (void *)(long)skb->data; + pctx.data_end = (void *)(long)skb->data_end; + pctx.data_end_end = pctx.data + skb->len; + pctx.nh.pos = pctx.data; - if (parse_packet_identifier(data, data_end, true, &p_id) < 0) + if (parse_packet_identifier(&pctx, true, &p_id) < 0) goto end; p_ts.timestamp = bpf_ktime_get_ns(); // or bpf_ktime_get_boot_ns diff --git a/pping/pping_kern_xdp.c b/pping/pping_kern_xdp.c index dff40ac..e135dd2 100644 --- a/pping/pping_kern_xdp.c +++ b/pping/pping_kern_xdp.c @@ -25,14 +25,17 @@ struct { SEC(XDP_PROG_SEC) int xdp_prog_ingress(struct xdp_md *ctx) { + struct parsing_context pctx; struct packet_id p_id = { 0 }; struct packet_timestamp *p_ts; struct rtt_event event = { 0 }; - void *data = (void *)(long)ctx->data; - void *data_end = (void *)(long)ctx->data_end; + pctx.data = (void *)(long)ctx->data; + pctx.data_end = (void *)(long)ctx->data_end; + pctx.data_end_end = pctx.data_end; + pctx.nh.pos = pctx.data; - if (parse_packet_identifier(data, data_end, false, &p_id) < 0) + if (parse_packet_identifier(&pctx, false, &p_id) < 0) goto end; p_ts = bpf_map_lookup_elem(&ts_start, &p_id);