* [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
@ 2013-10-28 9:14 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-10-28 9:14 UTC (permalink / raw)
To: linux-sctp
Unlike UDP or TCP, we do not take the pseudo-header into
account in SCTP checksums. So in case port mapping is the
very same, we do not need to recalculate the whole SCTP
checksum in software, which is very expensive.
Also, similarly as in 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.
Thanks for feedback regarding skb->ip_summed checks from
Julian Anastasov; here's a discussion on these checks for
snat and dnat:
* For snat_handler(), we can see CHECKSUM_PARTIAL from
virtual devices, and from LOCAL_OUT, otherwise it
should be CHECKSUM_UNNECESSARY. In general, in snat 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.
* For dnat_handler(), we should not see CHECKSUM_COMPLETE
for SCTP, in fact the small set of drivers that support
SCTP offloading return CHECKSUM_UNNECESSARY on correctly
received SCTP csum. We can see CHECKSUM_PARTIAL from
local stack or received from virtual drivers. 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. So, checks
in the 'if' part will decide whether it is ok to keep
CHECKSUM_PARTIAL for the output. If the packet was with
CHECKSUM_NONE, hence we deal with unknown checksum. As we
recalculate the sum for IP header in all cases, it should
be safe to use CHECKSUM_UNNECESSARY. We can forward wrong
checksum in this case (without cp->app). In case of
CHECKSUM_UNNECESSARY, the csum was valid on receive.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
v1->v2:
- Applied feedback from Julian, thanks!
net/netfilter/ipvs/ip_vs_proto_sctp.c | 37 +++++++++++++++++++++++++++++------
1 file changed, 31 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..ec2a126 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,30 @@ sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
return 0;
if (unlikely(cp->app != NULL)) {
+ int ret;
+
/* Some checks before mangling */
if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
return 0;
/* Call application helper if needed */
- if (!ip_vs_app_pkt_out(cp, skb))
+ if (!(ret = ip_vs_app_pkt_out(cp, skb)))
return 0;
+ /* ret=2: csum update is needed after payload mangling */
+ if (ret = 2)
+ payload_csum = true;
}
sctph = (void *) skb_network_header(skb) + sctphoff;
- sctph->source = cp->vport;
- sctp_nat_csum(skb, sctph, sctphoff);
+ /* Only update csum if we really have to */
+ if (sctph->source != cp->vport || payload_csum ||
+ skb->ip_summed = CHECKSUM_PARTIAL) {
+ sctph->source = cp->vport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ } else {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ }
return 1;
}
@@ -115,6 +127,7 @@ sctp_dnat_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)
@@ -126,19 +139,31 @@ sctp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
return 0;
if (unlikely(cp->app != NULL)) {
+ int ret;
+
/* Some checks before mangling */
if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
return 0;
/* Call application helper if needed */
- if (!ip_vs_app_pkt_in(cp, skb))
+ if (!(ret = ip_vs_app_pkt_in(cp, skb)))
return 0;
+ /* ret=2: csum update is needed after payload mangling */
+ if (ret = 2)
+ payload_csum = true;
}
sctph = (void *) skb_network_header(skb) + sctphoff;
- sctph->dest = cp->dport;
- sctp_nat_csum(skb, sctph, sctphoff);
+ /* Only update csum if we really have to */
+ if (sctph->dest != cp->dport || payload_csum ||
+ (skb->ip_summed = CHECKSUM_PARTIAL &&
+ !(skb_dst(skb)->dev->features & NETIF_F_SCTP_CSUM))) {
+ sctph->dest = cp->dport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ } else if (skb->ip_summed != CHECKSUM_PARTIAL) {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ }
return 1;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
@ 2013-10-28 9:14 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-10-28 9:14 UTC (permalink / raw)
To: horms; +Cc: ja, lvs-devel, linux-sctp
Unlike UDP or TCP, we do not take the pseudo-header into
account in SCTP checksums. So in case port mapping is the
very same, we do not need to recalculate the whole SCTP
checksum in software, which is very expensive.
Also, similarly as in 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.
Thanks for feedback regarding skb->ip_summed checks from
Julian Anastasov; here's a discussion on these checks for
snat and dnat:
* For snat_handler(), we can see CHECKSUM_PARTIAL from
virtual devices, and from LOCAL_OUT, otherwise it
should be CHECKSUM_UNNECESSARY. In general, in snat 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.
* For dnat_handler(), we should not see CHECKSUM_COMPLETE
for SCTP, in fact the small set of drivers that support
SCTP offloading return CHECKSUM_UNNECESSARY on correctly
received SCTP csum. We can see CHECKSUM_PARTIAL from
local stack or received from virtual drivers. 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. So, checks
in the 'if' part will decide whether it is ok to keep
CHECKSUM_PARTIAL for the output. If the packet was with
CHECKSUM_NONE, hence we deal with unknown checksum. As we
recalculate the sum for IP header in all cases, it should
be safe to use CHECKSUM_UNNECESSARY. We can forward wrong
checksum in this case (without cp->app). In case of
CHECKSUM_UNNECESSARY, the csum was valid on receive.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
v1->v2:
- Applied feedback from Julian, thanks!
net/netfilter/ipvs/ip_vs_proto_sctp.c | 37 +++++++++++++++++++++++++++++------
1 file changed, 31 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..ec2a126 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,30 @@ sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
return 0;
if (unlikely(cp->app != NULL)) {
+ int ret;
+
/* Some checks before mangling */
if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
return 0;
/* Call application helper if needed */
- if (!ip_vs_app_pkt_out(cp, skb))
+ if (!(ret = ip_vs_app_pkt_out(cp, skb)))
return 0;
+ /* ret=2: csum update is needed after payload mangling */
+ if (ret == 2)
+ payload_csum = true;
}
sctph = (void *) skb_network_header(skb) + sctphoff;
- sctph->source = cp->vport;
- sctp_nat_csum(skb, sctph, sctphoff);
+ /* Only update csum if we really have to */
+ if (sctph->source != cp->vport || payload_csum ||
+ skb->ip_summed == CHECKSUM_PARTIAL) {
+ sctph->source = cp->vport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ } else {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ }
return 1;
}
@@ -115,6 +127,7 @@ sctp_dnat_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)
@@ -126,19 +139,31 @@ sctp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
return 0;
if (unlikely(cp->app != NULL)) {
+ int ret;
+
/* Some checks before mangling */
if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
return 0;
/* Call application helper if needed */
- if (!ip_vs_app_pkt_in(cp, skb))
+ if (!(ret = ip_vs_app_pkt_in(cp, skb)))
return 0;
+ /* ret=2: csum update is needed after payload mangling */
+ if (ret == 2)
+ payload_csum = true;
}
sctph = (void *) skb_network_header(skb) + sctphoff;
- sctph->dest = cp->dport;
- sctp_nat_csum(skb, sctph, sctphoff);
+ /* Only update csum if we really have to */
+ if (sctph->dest != cp->dport || payload_csum ||
+ (skb->ip_summed == CHECKSUM_PARTIAL &&
+ !(skb_dst(skb)->dev->features & NETIF_F_SCTP_CSUM))) {
+ sctph->dest = cp->dport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ } else if (skb->ip_summed != CHECKSUM_PARTIAL) {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ }
return 1;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
2013-10-28 9:14 ` Daniel Borkmann
@ 2013-10-28 9:44 ` Julian Anastasov
-1 siblings, 0 replies; 6+ messages in thread
From: Julian Anastasov @ 2013-10-28 9:44 UTC (permalink / raw)
To: linux-sctp
Hello,
On Mon, 28 Oct 2013, Daniel Borkmann wrote:
> /* Call application helper if needed */
> - if (!ip_vs_app_pkt_out(cp, skb))
> + if (!(ret = ip_vs_app_pkt_out(cp, skb)))
> return 0;
> - if (!ip_vs_app_pkt_in(cp, skb))
> + if (!(ret = ip_vs_app_pkt_in(cp, skb)))
> return 0;
Sorry, just now noticed the errors from
scripts/checkpatch.pl:
ERROR: do not use assignment in if condition
#78: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:103:
+ if (!(ret = ip_vs_app_pkt_out(cp, skb)))
ERROR: do not use assignment in if condition
#120: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:149:
+ if (!(ret = ip_vs_app_pkt_in(cp, skb)))
total: 2 errors, 0 warnings, 81 lines checked
I know that you copied the TCP logic that was
added by me but now we should split the complex conditions
at both places:
ret = ...
if (!ret)
return 0;
Otherwise patch looks good and I'll ack v3 with
the above changes.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
@ 2013-10-28 9:44 ` Julian Anastasov
0 siblings, 0 replies; 6+ messages in thread
From: Julian Anastasov @ 2013-10-28 9:44 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
Hello,
On Mon, 28 Oct 2013, Daniel Borkmann wrote:
> /* Call application helper if needed */
> - if (!ip_vs_app_pkt_out(cp, skb))
> + if (!(ret = ip_vs_app_pkt_out(cp, skb)))
> return 0;
> - if (!ip_vs_app_pkt_in(cp, skb))
> + if (!(ret = ip_vs_app_pkt_in(cp, skb)))
> return 0;
Sorry, just now noticed the errors from
scripts/checkpatch.pl:
ERROR: do not use assignment in if condition
#78: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:103:
+ if (!(ret = ip_vs_app_pkt_out(cp, skb)))
ERROR: do not use assignment in if condition
#120: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:149:
+ if (!(ret = ip_vs_app_pkt_in(cp, skb)))
total: 2 errors, 0 warnings, 81 lines checked
I know that you copied the TCP logic that was
added by me but now we should split the complex conditions
at both places:
ret = ...
if (!ret)
return 0;
Otherwise patch looks good and I'll ack v3 with
the above changes.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
2013-10-28 9:44 ` Julian Anastasov
@ 2013-10-28 9:47 ` Daniel Borkmann
-1 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-10-28 9:47 UTC (permalink / raw)
To: linux-sctp
On 10/28/2013 10:44 AM, Julian Anastasov wrote:
[...]
> Sorry, just now noticed the errors from
> scripts/checkpatch.pl:
>
> ERROR: do not use assignment in if condition
> #78: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:103:
> + if (!(ret = ip_vs_app_pkt_out(cp, skb)))
>
> ERROR: do not use assignment in if condition
> #120: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:149:
> + if (!(ret = ip_vs_app_pkt_in(cp, skb)))
>
> total: 2 errors, 0 warnings, 81 lines checked
>
> I know that you copied the TCP logic that was
> added by me but now we should split the complex conditions
> at both places:
>
> ret = ...
> if (!ret)
> return 0;
>
> Otherwise patch looks good and I'll ack v3 with
> the above changes.
Ok, sure, I'll update with the cosmetic change.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change
@ 2013-10-28 9:47 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-10-28 9:47 UTC (permalink / raw)
To: Julian Anastasov; +Cc: horms, lvs-devel, linux-sctp
On 10/28/2013 10:44 AM, Julian Anastasov wrote:
[...]
> Sorry, just now noticed the errors from
> scripts/checkpatch.pl:
>
> ERROR: do not use assignment in if condition
> #78: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:103:
> + if (!(ret = ip_vs_app_pkt_out(cp, skb)))
>
> ERROR: do not use assignment in if condition
> #120: FILE: net/netfilter/ipvs/ip_vs_proto_sctp.c:149:
> + if (!(ret = ip_vs_app_pkt_in(cp, skb)))
>
> total: 2 errors, 0 warnings, 81 lines checked
>
> I know that you copied the TCP logic that was
> added by me but now we should split the complex conditions
> at both places:
>
> ret = ...
> if (!ret)
> return 0;
>
> Otherwise patch looks good and I'll ack v3 with
> the above changes.
Ok, sure, I'll update with the cosmetic change.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-28 9:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-28 9:14 [PATCH v2 ipvs-next] net: ipvs: sctp: do not recalc sctp csum when ports didn't change Daniel Borkmann
2013-10-28 9:14 ` Daniel Borkmann
2013-10-28 9:44 ` Julian Anastasov
2013-10-28 9:44 ` Julian Anastasov
2013-10-28 9:47 ` Daniel Borkmann
2013-10-28 9:47 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.