[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