On 05/01/2018 12:31 PM, Toke Høiland-Jørgensen wrote:
> Could you comment on specifically what you believe is broken in this, > please, so I can fix it in the same iteration? > Apart from the various pskb_may_pull() this helper should not change skb layout. Ideally, the skb should be const and you would use skb_header_pointer() to make clear you do not ever write this skb. This would make the reviewer job pretty easy, as no side effect can possibly happen. > +static inline struct tcphdr *cake_get_tcphdr(struct sk_buff *skb) > +{ > + struct ipv6hdr *ipv6h; > + struct iphdr *iph; > + > + /* check IPv6 header size immediately, since for IPv4 we need the space > + * for the TCP header anyway > + */ > + if (!pskb_may_pull(skb, skb_network_offset(skb) + > + sizeof(struct ipv6hdr))) > + return NULL; > + > + iph = ip_hdr(skb); > + > + if (iph->version == 4) { > + /* special-case 6in4 tunnelling, as that is a common way to get > + * v6 connectivity in the home > + */ > + if (iph->protocol == IPPROTO_IPV6) { > + if (!pskb_may_pull(skb, (skb_network_offset(skb) + > + ip_hdrlen(skb) + > + sizeof(struct ipv6hdr)))) > + return NULL; > + > + ipv6h = (struct ipv6hdr *)(skb_network_header(skb) + > + ip_hdrlen(skb)); > + > + if (ipv6h->nexthdr != IPPROTO_TCP) > + return NULL; > + > + skb_set_inner_network_header(skb, > + skb_network_offset(skb) + > + ip_hdrlen(skb)); > + skb_set_inner_transport_header(skb, > + skb_inner_network_offset(skb) + > + sizeof(struct ipv6hdr)); This is not allowed for a dissector.