LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when n

To: Julian Anastasov <ja@xxxxxx>
Subject: Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
Cc: horms@xxxxxxxxxxxx, lvs-devel@xxxxxxxxxxxxxxx, linux-sctp@xxxxxxxxxxxxxxx
From: Daniel Borkmann <dborkman@xxxxxxxxxx>
Date: Sat, 26 Oct 2013 20:55:40 +0200
On 10/25/2013 11:00 PM, Julian Anastasov wrote:

        Hello,

On Fri, 25 Oct 2013, Daniel Borkmann wrote:

Unlike UDP or TCP, we do not take the pseudo-header into account
in SCTP checksums [1]. So in case port mapping is the very same, we
do not need to recalculate the whole SCTP checksum in software, which
is expensive.

Also, similarly as in IPVS/TCP, take into account when a private
helper mangled the packet. In that case, we also need to recalculate
the checksum even if ports might be same.

  [1] http://tools.ietf.org/html/rfc4960#section-6.8

Signed-off-by: Daniel Borkmann <dborkman@xxxxxxxxxx>
---
  net/netfilter/ipvs/ip_vs_proto_sctp.c | 30 ++++++++++++++++++++++++------
  1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c 
b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 9ca7aa0..e56661e 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -81,6 +81,7 @@ sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol 
*pp,
  {
        sctp_sctphdr_t *sctph;
        unsigned int sctphoff = iph->len;
+       bool payload_csum = false;

  #ifdef CONFIG_IP_VS_IPV6
        if (cp->af == AF_INET6 && iph->fragoffs)
@@ -92,19 +93,27 @@ sctp_snat_handler(struct sk_buff *skb, struct 
ip_vs_protocol *pp,

...

-       sctp_nat_csum(skb, sctph, sctphoff);
+       /* Only update csum if we really have to */
+       if (sctph->source != cp->vport || payload_csum) {

        The above check should be little more complicated
because local SCTP can decide to avoid setting ->checksum,
there is a case when we can see CHECKSUM_PARTIAL for
locally generated packets. And it happens both for
requests (dnat_handler) and replies (snat_handler).

        I mean both places should be fixed because you can
see in ip_vs_ops[] that in NF_INET_LOCAL_OUT we can call both
snat_handler and dnat_handler.

        May be the simplest change can be to add check for
!skb->dev to catch the LOCAL_OUT hook, so that we can
perform full recalculation. We can further optimize this
check for dnat_handler because the dnat_handler can look
at the skb_dst()->dev->features as done by sctp_packet_transmit().
The idea is that SCTP decides to avoid csum calculation
if hardware supports offloading. IPVS can change the
device after rerouting to real server but we can preserve
the CHECKSUM_PARTIAL mode if the new device supports
offloading too. This works because skb dst is changed
before dnat_handler and we see the new device.

        For snat_handler it is more complex. skb contains
the original route and ip_vs_route_me_harder() can change
the route after snat_handler. So, for locally generated
replies from local server we can not preserve the
CHECKSUM_PARTIAL mode. It is an chicken or egg dilemma:
snat_handler needs the device after rerouting (to
check for NETIF_F_SCTP_CSUM), while ip_route_me_harder() wants
the snat_handler() to put the new saddr for proper rerouting.
So, for snat_handler we need just the !skb->dev check,
sort of:

        if (sctph->source != cp->vport || payload_csum ||
            (!skb->dev && skb->ip_summed == CHECKSUM_PARTIAL)) {

        But I have to think more whether we can preserve
the ip_summed value in other cases, see skb_forward_csum()
for reference.

Thanks for all your feedback Julian !

Let me think about this, and come back to you w/ a 2nd version
of the set at latest on Monday.

+               sctph->source = cp->vport;
+               sctp_nat_csum(skb, sctph, sctphoff);
+       }

        return 1;
  }

@@ -126,19 +136,27 @@ sctp_dnat_handler(struct sk_buff *skb, struct 
ip_vs_protocol *pp,

...

-       sctp_nat_csum(skb, sctph, sctphoff);
+       /* Only update csum if we really have to */
+       if (sctph->dest != cp->dport || payload_csum) {

        Here we can can preserve CHECKSUM_PARTIAL after
some checks, eg. when the new device in skb_dst supports
offloading.

+               sctph->dest = cp->dport;
+               sctp_nat_csum(skb, sctph, sctphoff);
+       }

        return 1;
  }
--
1.7.11.7

Regards

--
Julian Anastasov <ja@xxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>