On Fri, Jun 17, 2016 at 6:44 PM, Jarno Rajahalme <[email protected]> wrote:
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 4b3b78e..40e9843 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -337,6 +338,38 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
> return NF_DROP;
> }
>
> +#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 contents.
> + * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP
> + * has no checksum.
> + */
> + if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) {
I have a hard time verifying that this check is in the right place. It
looks to me like it only gets executed in the case that we have an ALG
and it also seems like it is running after the main NAT code. The
benefit of putting the checksum update here (as opposed to the compat
code where you had it earlier) is obviously that it can reuse the IPv6
extension header scanning, however, it seems like clearly correct, at
least to me.
> + 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:
> + case IPPROTO_UDPLITE:
I think UDP Lite doesn't need to be here - the checksum offload code
in the core kernel doesn't have any special logic for it and so will
compute a full packet checksum, the same as regular UDP.
I verified that UDP Lite has it's own code path separate from UDP and
won't call into the checksum update function that causes a problem. It
might be worth adding a comment explaining why only TCP/UDP over
IPv4/v6 can cause an issue.
> + if (skb->len < protoff + offset + 2)
> + return NF_DROP;
It might be safer to use pskb_may_pull() here to verify the checksum
data is in the linear data area. The checksum code asserts this and
we've had some problems in the past with that.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
