Lorenzo Bianconi lorenzo@kernel.org wrote:
- path->type = DEV_PATH_IPENCAP;
- path->dev = ctx->dev;
- path->encap.proto = htons(ETH_P_IP);
- path->encap.id = jhash_3words(ntohl(tiph->saddr), ntohl(tiph->daddr),
IPPROTO_IPIP, 0);I think it would be better to have a helper. Else I think this needs a comment that explains it must be kept in sync with nf_flow_tuple_encap().
Or use __ipv4_addr_hash(tiph->saddr, (__force __u32)tiph->daddr). (loses IPPROTO_IPIP though).
ack, I will fix it in v5.
@@ -165,6 +166,19 @@ static void nf_flow_tuple_encap(struct sk_buff *skb, tuple->encap[i].id = ntohs(phdr->sid); tuple->encap[i].proto = skb->protocol; break;
- case htons(ETH_P_IP):
if (!pskb_may_pull(skb, sizeof(*iph)))break;Is this needed? Caller does:
if (!pskb_may_pull(skb, thoff + ctx->hdrsize)) return -1;and then populates the inner header: iph = (struct iphdr *)(skb_network_header(skb) + ctx->offset); tuple->src_v4.s_addr = iph->saddr;
.... so I think this can rely on the outer header being available via skb_network_header().
I agree, I will fix it in v5.
tuple->encap[i].proto = htons(ETH_P_IP);tuple->encap[i].id = jhash_3words(ntohl(iph->daddr),ntohl(iph->saddr),IPPROTO_IPIP, 0);See above, I think this desevers a helper or a comment, or both.
+static bool nf_flow_ip4_encap_proto(struct sk_buff *skb, u16 *size) +{
- struct iphdr *iph;
- if (!pskb_may_pull(skb, sizeof(*iph)))
return false;Nit: I think this could be 2 * sizeof() and a comment that we will also need the inner ip header later, might save one reallocation.
nf_flow_ip4_encap_proto() is used even for plain IP traffic but I guess we can assume the IP payload is at least 20B, right?
- iph = (struct iphdr *)skb_network_header(skb);
- *size = iph->ihl << 2;
I think this should be sanity tested vs. sizeof(iph).
I guess this is already done in ip_has_options(), agree?
static bool nf_flow_skb_encap_protocol(struct sk_buff *skb, __be16 proto, u32 *offset) { struct vlan_ethhdr *veth; __be16 inner_proto;
- u16 size;
switch (skb->protocol) {
- case htons(ETH_P_IP):
if (nf_flow_ip4_encap_proto(skb, &size))*offset += size;Nit: return nf_flow_ip4_encap_proto(skb, &offset) ?
ack, I will fix it in v5.
Regards, Lorenzo