Message ID | 1444335972.14655.388.camel@sakura.staff.proxad.net |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Oct 8, 2015 at 1:26 PM, Maxime Bizon <mbizon@freebox.fr> wrote: > > With this setup: > > * non IPv6 checksumming capable network hardware > * GRO off > * IPv6 SNAT > > I get this when I receive an UDPv6 reply: "<unknown>: hw csum failure" > > Call trace: > > * nf_ip6_checksum() calls __skb_checksum_complete() > * nf_nat_ipv6_csum_update() & nf_nat_ipv6_manip_pkt() > * __udp6_lib_rcv() => udp6_csum_init() > * __skb_checksum_validate_complete() "fastpath" fails because > skb->csum is incorrect. > * udpv6_recvmsg() => skb_copy_and_csum_datagram_msg() > > The last call computes a valid checksum despite CHECKSUM_COMPLETE and > triggers the warning. > > When we perform NAT on IPv4, we also update the IPv4 checksum, so > there is no side effect on skb->csum (since the csum over a valid IPv4 > header area was already zero). > > But IPv6 doesn't have such checksum, so when performing NAT we need to > update skb->csum. > > Signed-off-by: Maxime Bizon <mbizon@freebox.fr> > --- > net/ipv6/netfilter/nf_nat_l3proto_ipv6.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c > index 70fbaed..e44af9c 100644 > --- a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c > +++ b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c > @@ -81,6 +81,8 @@ static bool nf_nat_ipv6_manip_pkt(struct sk_buff *skb, > enum nf_nat_manip_type maniptype) > { > struct ipv6hdr *ipv6h; > + const __be32 *to; > + __be32 *from; > __be16 frag_off; > int hdroff; > u8 nexthdr; > @@ -100,11 +102,24 @@ static bool nf_nat_ipv6_manip_pkt(struct sk_buff *skb, > target, maniptype)) > return false; > manip_addr: > - if (maniptype == NF_NAT_MANIP_SRC) > - ipv6h->saddr = target->src.u3.in6; > - else > - ipv6h->daddr = target->dst.u3.in6; > + if (maniptype == NF_NAT_MANIP_SRC) { > + from = ipv6h->saddr.s6_addr32; > + to = target->src.u3.in6.s6_addr32; > + } else { > + from = ipv6h->daddr.s6_addr32; > + to = target->dst.u3.in6.s6_addr32; > + } > + > + if (skb->ip_summed == CHECKSUM_COMPLETE) { > + __be32 diff[] = { > + ~from[0], ~from[1], ~from[2], ~from[3], > + to[0], to[1], to[2], to[3], > + }; > + > + skb->csum = ~csum_partial(diff, sizeof(diff), ~skb->csum); > + } > I think inet_proto_csum_replace16 should be called here. > + memcpy(from, to, sizeof (struct in6_addr)); > return true; > } > > -- > 1.9.1 > > > -- > Maxime > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-10-08 at 14:09 -0700, Tom Herbert wrote:
> I think inet_proto_csum_replace16 should be called here.
inet_proto_csum_replace16() wants a non NULL checksum pointer to update,
and there is no such thing here.
I could pass a dummy value, but inet_proto_csum_replace16() will do
twice more work for nothing
or I could modify inet_proto_csum_replace16() to allow a NULL sum
argument.
On Thu, Oct 8, 2015 at 2:26 PM, Maxime Bizon <mbizon@freebox.fr> wrote: > > On Thu, 2015-10-08 at 14:09 -0700, Tom Herbert wrote: > >> I think inet_proto_csum_replace16 should be called here. > > inet_proto_csum_replace16() wants a non NULL checksum pointer to update, > and there is no such thing here. > > I could pass a dummy value, but inet_proto_csum_replace16() will do > twice more work for nothing > > or I could modify inet_proto_csum_replace16() to allow a NULL sum > argument. > If I am reading the code correctly, it looks like inet_proto_csum_replace16 is called from {tcp,dccp,tcp.udp,udplite}_manip_pkt via l3proto->csum_update which is a call to nf_nat_ipv6_csum_update for IPv6. So for these protocols when the transport checksum is updated skb->csum is also correctly updated. For other protocols or when UDP checksum is zero or for fragments-- I don't see where skb->csum is being properly updated. This potentially seems to be a major bug. I would suggest: 1) Modify the inet_proto_csum_replace* routines to check for NULL check pointer and and only write it when non-NULL 2) Call l3proto->csum_update from unknown_manip_pkt with check == NULL 3) In udp_manip_pkt call l3proto->csum_update with check = NULL when UDP checksum is zero 4) For IPv6 fragments call l3proto->csum_update with check = NULL Tom > -- > Maxime > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c index 70fbaed..e44af9c 100644 --- a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c +++ b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c @@ -81,6 +81,8 @@ static bool nf_nat_ipv6_manip_pkt(struct sk_buff *skb, enum nf_nat_manip_type maniptype) { struct ipv6hdr *ipv6h; + const __be32 *to; + __be32 *from; __be16 frag_off; int hdroff; u8 nexthdr; @@ -100,11 +102,24 @@ static bool nf_nat_ipv6_manip_pkt(struct sk_buff *skb, target, maniptype)) return false; manip_addr: - if (maniptype == NF_NAT_MANIP_SRC) - ipv6h->saddr = target->src.u3.in6; - else - ipv6h->daddr = target->dst.u3.in6; + if (maniptype == NF_NAT_MANIP_SRC) { + from = ipv6h->saddr.s6_addr32; + to = target->src.u3.in6.s6_addr32; + } else { + from = ipv6h->daddr.s6_addr32; + to = target->dst.u3.in6.s6_addr32; + } + + if (skb->ip_summed == CHECKSUM_COMPLETE) { + __be32 diff[] = { + ~from[0], ~from[1], ~from[2], ~from[3], + to[0], to[1], to[2], to[3], + }; + + skb->csum = ~csum_partial(diff, sizeof(diff), ~skb->csum); + } + memcpy(from, to, sizeof (struct in6_addr)); return true; }
With this setup: * non IPv6 checksumming capable network hardware * GRO off * IPv6 SNAT I get this when I receive an UDPv6 reply: "<unknown>: hw csum failure" Call trace: * nf_ip6_checksum() calls __skb_checksum_complete() * nf_nat_ipv6_csum_update() & nf_nat_ipv6_manip_pkt() * __udp6_lib_rcv() => udp6_csum_init() * __skb_checksum_validate_complete() "fastpath" fails because skb->csum is incorrect. * udpv6_recvmsg() => skb_copy_and_csum_datagram_msg() The last call computes a valid checksum despite CHECKSUM_COMPLETE and triggers the warning. When we perform NAT on IPv4, we also update the IPv4 checksum, so there is no side effect on skb->csum (since the csum over a valid IPv4 header area was already zero). But IPv6 doesn't have such checksum, so when performing NAT we need to update skb->csum. Signed-off-by: Maxime Bizon <mbizon@freebox.fr> --- net/ipv6/netfilter/nf_nat_l3proto_ipv6.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)