[ovs-dev] [PATCH] Conntrack: Fix L4 Checksums in kernel <4.6 when using NAT and helpers

Jarno Rajahalme jarno at ovn.org
Wed Jan 4 00:53:01 UTC 2017


> On Dec 28, 2016, at 3:05 PM, John Hurley <john.hurley at netronome.com> wrote:
> 
> sorry, updated patch….

This patch is still whitespace damaged. Maybe use git format-patch and git send-email to send it?

> 
> --------------------------------
> 
> Fix for a bug when sending a NATed packet to helper function in kernels
> <4.6.
> 
> Setting CHECKSUM_PARTIAL flag can lead to L4 checksum corruption.
> 
> Corruption can occur in datapath.c/queue_userspace_packet().
> 

You mean this code:

	/* Complete checksum if needed */
	if (skb->ip_summed == CHECKSUM_PARTIAL &&
	    (err = skb_checksum_help(skb)))
		goto out;

Would you care detailing why the corruption happens? Does it always happen if ip_summed is CHECKSUM_PARTIAL?

> Giving the packet an skb_dst allows the kernel to correct the checksum.
> 
> Verified with packets mangled by Conntrack/NAT helpers.
> 

It would also be helpful to add the reference to the patch that this fixes (“Fixes:”)

> Signed-off-by: John Hurley <john.hurley at netronome.com>
> ---
> 
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index d942884..fef67ba 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -314,6 +314,10 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
> proto)
>  u8 nexthdr;
>  int err;
> 
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
> + struct rtable *rt = NULL;
> +#endif
> +
>  ct = nf_ct_get(skb, &ctinfo);
>  if (!ct || ctinfo == IP_CT_RELATED_REPLY)
>  return NF_ACCEPT;
> @@ -352,43 +356,28 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
> proto)
> #if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
>  /* Linux 4.5 and older depend on skb_dst being set when recalculating
>  * checksums after NAT helper has mangled TCP or UDP packet payload.
> - * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP
> - * has no checksum.
>  *
> - * The dependency is not triggered when the main NAT code updates
> - * checksums after translating the IP header (address, port), so this
> - * fix only needs to be executed on packets that are both being NATted
> - * and that have a helper assigned.
> + * skb_dst is cast to a rtable struct and the flags examined.
> + * Forcing these flags to have RTCF_LOCAL set allows checksum mod
> + * to be carried out in the same way as kernel versions > 4.5

Are kernels > 4.5 treating all these packets as RTCF_LOCAL, or are some of them possibly CHECKSUM_PARTIAL?

>  */
>  if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) {
> - u8 ipproto = (proto == NFPROTO_IPV4)
> - ? ip_hdr(skb)->protocol : nexthdr;
> - u16 offset = 0;
> -
> - switch (ipproto) {
> - case IPPROTO_TCP:
> - offset = offsetof(struct tcphdr, check);
> - break;
> - case IPPROTO_UDP:
> - /* Skip if no csum. */
> - if (udp_hdr(skb)->check)
> - offset = offsetof(struct udphdr, check);
> - break;
> - }
> - if (offset) {
> - if (unlikely(!pskb_may_pull(skb, protoff + offset + 2)))
> - return NF_DROP;
> -
> - skb->csum_start = skb_headroom(skb) + protoff;
> - skb->csum_offset = offset;
> - skb->ip_summed = CHECKSUM_PARTIAL;
> - }
> + rt = kmalloc(sizeof(struct rtable), GFP_KERNEL);

Could the struct rtable be allocated from stack instead? Or could it be a static variable in the file scope? In either case we would avoid dynamic memory allocation/free. I recall setting ip_summed to CHECKSUM_PARTIAL was deemed a simpler way to backport as it (also) avoids the dynamic memory allocations.
 
> + rt->rt_flags = RTCF_LOCAL;
> + skb_dst_set(skb, (struct dst_entry *)rt);
>  }
> #endif
>  err = helper->help(skb, protoff, ct, ctinfo);
>  if (err != NF_ACCEPT)
>  return err;
> 
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
> + if (rt) {
> + skb_dst_set(skb, NULL);
> + kfree(rt);
> + }
> +#endif
> +
>  /* Adjust seqs after helper.  This is needed due to some helpers (e.g.,
>  * FTP with NAT) adusting the TCP payload size when mangling IP
>  * addresses and/or port numbers in the text-based control connection.
> 
> --
> 
> On Wed, Dec 28, 2016 at 10:08 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
>> On Wed, Dec 28, 2016 at 09:37:30PM +0000, John Hurley wrote:
>>> Fix for a bug when sending a NATed packet to helper function in kernels
>>> <4.6.
>>> 
>>> Setting CHECKSUM_PARTIAL flag means packets could have L4 checksum
>>> corrupted in
>>> 
>>> datapath.c/queue_userspace_packet().
>>> 
>>> Giving the packet an skb_dst allows the kernel to correct the checksum if
>>> packet
>>> mangling happens in Conntrack/NAT helpers.
>>> 
>>> Signed-off-by: John Hurley <john.hurley at netronome.com>
>> 
>> I'm not the right person to review this but I did notice that the patch
>> is wordwrapped and otherwise space-damaged.
>> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list