Re: [PATCH net-next v7 3/7] sch_cake: Add optional ACK filter

[kbuild test robot <lkp@intel.com>]

Hi Toke,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/sched-Add-Common-Applications-Kept-Enhanced-cake-qdisc/20180503-073002


coccinelle warnings: (new ones prefixed by >>)

>> net/sched/sch_cake.c:1047:6-13: ERROR: PTR_ERR applied after initialization to constant on line 822

vim +1047 net/sched/sch_cake.c

   812	
   813	static struct sk_buff *cake_ack_filter(struct cake_sched_data *q,
   814					       struct cake_flow *flow)
   815	{
   816		bool thisconn_redundant_seen = false, thisconn_seen_last = false;
   817		bool aggressive = q->ack_filter == CAKE_ACK_AGGRESSIVE;
   818		bool otherconn_ack_seen = false;
   819		struct sk_buff *skb_check, *skb_check_prev;
   820		struct sk_buff *otherconn_checked_to = NULL;
   821		struct sk_buff *thisconn_checked_to = NULL;
   822		struct sk_buff *thisconn_ack = NULL;
   823		const struct ipv6hdr *ipv6h, *ipv6h_check;
   824		const struct tcphdr *tcph, *tcph_check;
   825		const struct iphdr *iph, *iph_check;
   826		const struct sk_buff *skb;
   827		struct ipv6hdr _iph, _iph_check;
   828		struct tcphdr _tcph_check;
   829		unsigned char _tcph[64]; /* need to hold maximum hdr size */
   830		int seglen;
   831	
   832		/* no other possible ACKs to filter */
   833		if (flow->head == flow->tail)
   834			return NULL;
   835	
   836		skb = flow->tail;
   837		tcph = cake_get_tcphdr(skb, _tcph, sizeof(_tcph));
   838		iph = cake_get_iphdr(skb, &_iph);
   839		if (!tcph)
   840			return NULL;
   841	
   842		/* the 'triggering' packet need only have the ACK flag set.
   843		 * also check that SYN is not set, as there won't be any previous ACKs.
   844		 */
   845		if ((tcp_flag_word(tcph) &
   846		     (TCP_FLAG_ACK | TCP_FLAG_SYN)) != TCP_FLAG_ACK)
   847			return NULL;
   848	
   849		/* the 'triggering' ACK is at the end of the queue,
   850		 * we have already returned if it is the only packet in the flow.
   851		 * stop before last packet in queue, don't compare trigger ACK to itself
   852		 * start where we finished last time if recorded in ->ackcheck
   853		 * otherwise start from the the head of the flow queue.
   854		 */
   855		skb_check_prev = flow->ackcheck;
   856		skb_check = flow->ackcheck ?: flow->head;
   857	
   858		while (skb_check->next) {
   859			bool pure_ack, thisconn;
   860	
   861			/* don't increment if at head of flow queue (_prev == NULL) */
   862			if (skb_check_prev) {
   863				skb_check_prev = skb_check;
   864				skb_check = skb_check->next;
   865				if (!skb_check->next)
   866					break;
   867			} else {
   868				skb_check_prev = ERR_PTR(-1);
   869			}
   870	
   871			iph_check = cake_get_iphdr(skb_check, &_iph_check);
   872			tcph_check = cake_get_tcphdr(skb_check, &_tcph_check,
   873						     sizeof(_tcph_check));
   874	
   875			if (!tcph_check || iph->version != iph_check->version)
   876				continue;
   877	
   878			if (iph->version == 4) {
   879				seglen = ntohs(iph_check->tot_len) -
   880					       (4 * iph_check->ihl);
   881	
   882				thisconn = (iph_check->saddr == iph->saddr &&
   883					    iph_check->daddr == iph->daddr);
   884			} else if (iph->version == 6) {
   885				ipv6h = (struct ipv6hdr *)iph;
   886				ipv6h_check = (struct ipv6hdr *)iph_check;
   887				seglen = ntohs(ipv6h_check->payload_len);
   888	
   889				thisconn = (!ipv6_addr_cmp(&ipv6h_check->saddr,
   890							   &ipv6h->saddr) &&
   891					    !ipv6_addr_cmp(&ipv6h_check->daddr,
   892							   &ipv6h->daddr));
   893			} else {
   894				WARN_ON(1);  /* shouldn't happen */
   895				continue;
   896			}
   897	
   898			/* stricter criteria apply to ACKs that we may filter
   899			 * 3 reserved flags must be unset to avoid future breakage
   900			 * ECE/CWR/NS can be safely ignored
   901			 * ACK must be set
   902			 * All other flags URG/PSH/RST/SYN/FIN must be unset
   903			 * 0x0FFF0000 = all TCP flags (confirm ACK=1, others zero)
   904			 * 0x01C00000 = NS/CWR/ECE (safe to ignore)
   905			 * 0x0E3F0000 = 0x0FFF0000 & ~0x01C00000
   906			 * must be 'pure' ACK, contain zero bytes of segment data
   907			 * options are ignored
   908			 */
   909			if ((tcp_flag_word(tcph_check) &
   910			     (TCP_FLAG_ACK | TCP_FLAG_SYN)) != TCP_FLAG_ACK)
   911				continue;
   912	
   913			else if (((tcp_flag_word(tcph_check) &
   914				   cpu_to_be32(0x0E3F0000)) != TCP_FLAG_ACK) ||
   915				 ((seglen - __tcp_hdrlen(tcph_check)) != 0))
   916				pure_ack = false;
   917	
   918			else
   919				pure_ack = true;
   920	
   921			/* if we find an ACK belonging to a different connection
   922			 * continue checking for other ACKs this round however
   923			 * restart checking from the other connection next time.
   924			 */
   925			if (thisconn &&	(tcph_check->source != tcph->source ||
   926					 tcph_check->dest != tcph->dest))
   927				thisconn = false;
   928	
   929			/* new ack sequence must be greater
   930			 */
   931			if (thisconn &&
   932			    ((int32_t)(ntohl(tcph_check->ack_seq) -
   933				       ntohl(tcph->ack_seq)) > 0))
   934				continue;
   935	
   936			/* DupACKs with an equal sequence number shouldn't be filtered,
   937			 * but we can filter if the triggering packet is a SACK
   938			 */
   939			if (thisconn &&
   940			    (ntohl(tcph_check->ack_seq) == ntohl(tcph->ack_seq))) {
   941				/* inspired by tcp_parse_options in tcp_input.c */
   942				bool sack = false;
   943				int length = __tcp_hdrlen(tcph) - sizeof(struct tcphdr);
   944				const u8 *ptr = (const u8 *)(tcph + 1);
   945	
   946				while (length > 0) {
   947					int opcode = *ptr++;
   948					int opsize;
   949	
   950					if (opcode == TCPOPT_EOL)
   951						break;
   952					if (opcode == TCPOPT_NOP) {
   953						length--;
   954						continue;
   955					}
   956					opsize = *ptr++;
   957					if (opsize < 2 || opsize > length)
   958						break;
   959					if (opcode == TCPOPT_SACK) {
   960						sack = true;
   961						break;
   962					}
   963					ptr += opsize - 2;
   964					length -= opsize;
   965				}
   966				if (!sack)
   967					continue;
   968			}
   969	
   970			/* somewhat complicated control flow for 'conservative'
   971			 * ACK filtering that aims to be more polite to slow-start and
   972			 * in the presence of packet loss.
   973			 * does not filter if there is one 'redundant' ACK in the queue.
   974			 * 'data' ACKs won't be filtered but do count as redundant ACKs.
   975			 */
   976			if (thisconn) {
   977				thisconn_seen_last = true;
   978				/* if aggressive and this is a data ack we can skip
   979				 * checking it next time.
   980				 */
   981				thisconn_checked_to = (aggressive && !pure_ack) ?
   982					skb_check : skb_check_prev;
   983				/* the first pure ack for this connection.
   984				 * record where it is, but only break if aggressive
   985				 * or already seen data ack from the same connection
   986				 */
   987				if (pure_ack && !thisconn_ack) {
   988					thisconn_ack = skb_check_prev;
   989					if (aggressive || thisconn_redundant_seen)
   990						break;
   991				/* data ack or subsequent pure ack */
   992				} else {
   993					thisconn_redundant_seen = true;
   994					/* this is the second ack for this connection
   995					 * break to filter the first pure ack
   996					 */
   997					if (thisconn_ack)
   998						break;
   999				}
  1000			/* track packets from non-matching tcp connections that will
  1001			 * need evaluation on the next run.
  1002			 * if there are packets from both the matching connection and
  1003			 * others that requre checking next run, track which was updated
  1004			 * last and return the older of the two to ensure full coverage.
  1005			 * if a non-matching pure ack has been seen, cannot skip any
  1006			 * further on the next run so don't update.
  1007			 */
  1008			} else if (!otherconn_ack_seen) {
  1009				thisconn_seen_last = false;
  1010				if (pure_ack) {
  1011					otherconn_ack_seen = true;
  1012					/* if aggressive we don't care about old data,
  1013					 * start from the pure ack.
  1014					 * otherwise if there is a previous data ack,
  1015					 * start checking from it next time.
  1016					 */
  1017					if (aggressive || !otherconn_checked_to)
  1018						otherconn_checked_to = skb_check_prev;
  1019				} else {
  1020					otherconn_checked_to = aggressive ?
  1021						skb_check : skb_check_prev;
  1022				}
  1023			}
  1024		}
  1025	
  1026		/* skb_check is reused at this point
  1027		 * it is the pure ACK to be filtered (if any)
  1028		 */
  1029		skb_check = NULL;
  1030	
  1031		/* next time start checking from the older/nearest to head of unfiltered
  1032		 * but important tcp packets from this connection and other connections.
  1033		 * if none seen, start after the last packet evaluated in the loop.
  1034		 */
  1035		if (thisconn_checked_to && otherconn_checked_to)
  1036			flow->ackcheck = thisconn_seen_last ?
  1037				otherconn_checked_to : thisconn_checked_to;
  1038		else if (thisconn_checked_to)
  1039			flow->ackcheck = thisconn_checked_to;
  1040		else if (otherconn_checked_to)
  1041			flow->ackcheck = otherconn_checked_to;
  1042		else
  1043			flow->ackcheck = skb_check_prev;
  1044	
  1045		/* if filtering, remove the pure ACK from the flow queue */
  1046		if (thisconn_ack && (aggressive || thisconn_redundant_seen)) {
> 1047			if (PTR_ERR(thisconn_ack) == -1) {
  1048				skb_check = flow->head;
  1049				flow->head = flow->head->next;
  1050			} else {
  1051				skb_check = thisconn_ack->next;
  1052				thisconn_ack->next = thisconn_ack->next->next;
  1053			}
  1054		}
  1055	
  1056		/* we just filtered that ack, fix up the list */
  1057		if (flow->ackcheck == skb_check)
  1058			flow->ackcheck = thisconn_ack;
  1059		/* check the entire flow queue next time */
  1060		if (PTR_ERR(flow->ackcheck) == -1)
  1061			flow->ackcheck = NULL;
  1062	
  1063		return skb_check;
  1064	}
  1065	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation