On (10/17/16 11:15), Alexander Duyck wrote: > I would say you probably only need the first check here for skb->data > and could probably skip the second part. You will be testing for > skb_tail_pointer in all the other tests you added so this check is > redundant anyway. > > Also you might want to go through and wrap these with unlikely() since > most of these are exception cases.
Ok.. v3 will have this. > > /* Currently only IPv4/IPv6 with TCP is supported */ > > switch (hdr.ipv4->version) { > > case IPVERSION: > > /* access ihl as u8 to avoid unaligned access on ia64 */ > > hlen = (hdr.network[0] & 0x0F) << 2; > > + if (skb_tail_pointer(skb) < hdr.network + hlen + > > + sizeof(struct tcphdr)) > > + return; > > l4_proto = hdr.ipv4->protocol; > > break; > > case 6: > > hlen = hdr.network - skb->data; > > + if (skb_tail_pointer(skb) < hdr.network + hlen + > > + sizeof(struct tcphdr)) > > + return; > > l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, > > NULL); > > hlen -= hdr.network - skb->data; > > break; > > I believe one more check is needed after this block to verify the TCP > header fields are present. > > So you probably need to add a check for "skb_tail_pointer(skb) < > (hdr.network + hlen + 20)". But isnt that the same thing as the checks before l4_proto computation above? --Sowmini