On Tue, May 1, 2018 at 9:12 PM, David Ahern <dsah...@gmail.com> wrote: > On 5/1/18 12:32 PM, Dave Taht wrote: >> ack_recognize can shift pure tcp acks into another flowid. >> --- >> examples/bpf/ack_recognize.c | 98 >> ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 examples/bpf/ack_recognize.c >> >> diff --git a/examples/bpf/ack_recognize.c b/examples/bpf/ack_recognize.c >> new file mode 100644 >> index 0000000..5da620c >> --- /dev/null >> +++ b/examples/bpf/ack_recognize.c >> @@ -0,0 +1,98 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright 2017 Google Inc. > > 2017?? it's 2018. Did you write this last year and just now posting?
November, 2017? Shortly prior to you taking iproute2-next off of steven's hands. It was the first stage of a proof of concept showing (eventually) that a simple ack decimator/filter (like "drop every other ack", or simple "drop head" in the supplied example) had a ton of problems, and that to filter out acks correctly to any extent, it needed to peer back into the queue. (see the sch_cake ack-filter controversy on-going on this list) While it's better than what is in wondershaper, I wouldn't recommend anyone use it. It was, however, useful in acquiring a gut feel for what several other broken ack filters might be doing in the field. I submitted it as a possibly useful example for showing off tc-bpf and to add fuel to the ack-filter fire under cake. I can clean it up, SPF it, etc, if you want it. Otherwise, sorry for the noise. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> + * 02110-1301, USA. >> + */ >> + >> +/* >> + * Author: dave.t...@gmail.com (Dave Taht) >> + * >> + * ack_recognizer: An eBPF program that correctly recognizes modern TCP >> ACKs, >> + * with tcp option fields like SACK and timestamps, and no additional data. >> + * >> + * ack_match call: Recognize "pure acks" with no data payload >> + * >> + */ >> + >> +#include "bpf_api.h" >> +#include "linux/if_ether.h" >> +#include "linux/ip.h" >> +#include "linux/in.h" >> +#include "linux/ipv6.h" >> +#include "linux/tcp.h" >> + >> +/* >> + * A pure ack contains the ip header, the tcp header + options, flags with >> the >> + * ack field set, and no additional payload. That last bit is what every >> prior >> + * ack filter gets wrong, they typically assume an obsolete 64 bytes, and >> don't >> + * calculate the options (like sack or timestamps) to subtract from the >> payload. >> + */ >> + >> +__section_cls_entry >> +int ack_match(struct __sk_buff *skb) >> +{ >> + void *data = (void *)(long)skb->data; >> + void *data_end = (void *)(long)skb->data_end; >> + struct ethhdr *eth = data; >> + struct iphdr *iph = data + sizeof(*eth); >> + struct tcphdr *tcp; >> + >> + if (data + sizeof(*eth) + sizeof(*iph) + sizeof(*tcp) > data_end) >> + return 0; >> + >> + if (eth->h_proto == htons(ETH_P_IP) && >> + iph->version == 4) { > > Why not make the version == 4 under the proto check? > if (eth->h_proto == htons(ETH_P_IP) { > if (iph->version != 4) > return 0; > if the eth proto is ETH_P_IP, and version != 4 something is really wrong > >> + if(iph->protocol == IPPROTO_TCP && >> + iph->ihl == 5 && >> + data + sizeof(*eth) + 20 + sizeof(*tcp) <= data_end) { >> + tcp = data + sizeof(*eth) + 20; >> + if (tcp->ack && >> + htons(iph->tot_len) == 20 + tcp->doff*4) >> + return -1; > > And then here why the magic '20' and limits on ihl = 5? both are trivial > to accommodate programmatically. > >> + } >> + } else if (eth->h_proto == htons(ETH_P_IPV6) && >> + iph->version == 6) { >> + struct ipv6hdr *iph6 = (struct ipv6hdr *) iph; >> + if(iph6->nexthdr == IPPROTO_TCP && >> + data + sizeof(*eth) + 40 + sizeof(*tcp) <= data_end ) { >> + tcp = data + sizeof(*eth) + 40; >> + if (tcp->ack && >> + tcp->doff*4 == htons(iph6->payload_len)) >> + return -1; > > ditto here. > >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* Example: Move acks into a priority queue: >> + >> +IFACE=eth0 >> +tc qdisc del dev $IFACE root 2> /dev/null >> +tc qdisc add dev $IFACE root handle 1: prio bands 3 \ >> + priomap 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 >> +tc qdisc add dev $IFACE parent 1:1 handle 10:1 sfq headdrop # acks only >> +tc qdisc add dev $IFACE parent 1:2 handle 20:1 fq_codel # all other traffic >> +tc qdisc add dev $IFACE parent 1:3 handle 30:1 fq_codel # unused >> +tc filter add dev $IFACE parent 1: prio 1 bpf \ >> + object-file ack_recognize.o flowid 1:1 >> + >> +Please note that a strict priority queue is not a good idea (drr would be >> +better), nor is doing any level of prioritization on acks at all.... >> +*/ >> + >> +BPF_LICENSE("GPL"); >> > -- Dave Täht CEO, TekLibre, LLC http://www.teklibre.com Tel: 1-669-226-2619