* [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-25 9:05 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-25 9:05 UTC (permalink / raw)
To: horms; +Cc: lvs-devel, linux-sctp
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@redhat.com>
---
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,
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) {
+ sctph->source = cp->vport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ }
return 1;
}
@@ -115,6 +124,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 +136,27 @@ 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) {
+ sctph->dest = cp->dport;
+ sctp_nat_csum(skb, sctph, sctphoff);
+ }
return 1;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 9:48 ` Jesper Dangaard Brouer
-1 siblings, 0 replies; 34+ messages in thread
From: Jesper Dangaard Brouer @ 2013-10-25 9:48 UTC (permalink / raw)
To: linux-sctp
On Fri, 25 Oct 2013 11:05:05 +0200
Daniel Borkmann <dborkman@redhat.com> 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@redhat.com>
> ---
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Thanks Daniel, this will hopefully increase SCTP performance as we
avoid recalculating the checksum when not necessary :-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-25 9:48 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 34+ messages in thread
From: Jesper Dangaard Brouer @ 2013-10-25 9:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
On Fri, 25 Oct 2013 11:05:05 +0200
Daniel Borkmann <dborkman@redhat.com> 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@redhat.com>
> ---
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Thanks Daniel, this will hopefully increase SCTP performance as we
avoid recalculating the checksum when not necessary :-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 13:01 ` Neil Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2013-10-25 13:01 UTC (permalink / raw)
To: linux-sctp
On Fri, Oct 25, 2013 at 11:05:05AM +0200, 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@redhat.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---
> 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,
> 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) {
> + sctph->source = cp->vport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
>
> return 1;
> }
> @@ -115,6 +124,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 +136,27 @@ 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) {
> + sctph->dest = cp->dport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
>
> return 1;
> }
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-25 13:01 ` Neil Horman
0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2013-10-25 13:01 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
On Fri, Oct 25, 2013 at 11:05:05AM +0200, 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@redhat.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---
> 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,
> 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) {
> + sctph->source = cp->vport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
>
> return 1;
> }
> @@ -115,6 +124,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 +136,27 @@ 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) {
> + sctph->dest = cp->dport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
>
> return 1;
> }
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 21:00 ` Julian Anastasov
-1 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-25 21:00 UTC (permalink / raw)
To: linux-sctp
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@redhat.com>
> ---
> 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.
> + 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@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-25 21:00 ` Julian Anastasov
0 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-25 21:00 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
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@redhat.com>
> ---
> 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.
> + 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@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-25 21:00 ` Julian Anastasov
@ 2013-10-26 18:55 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-26 18:55 UTC (permalink / raw)
To: linux-sctp
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@redhat.com>
>> ---
>> 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@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-26 18:55 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-26 18:55 UTC (permalink / raw)
To: Julian Anastasov; +Cc: horms, lvs-devel, linux-sctp
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@redhat.com>
>> ---
>> 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@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-26 18:55 ` Daniel Borkmann
@ 2013-10-28 1:39 ` Simon Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 1:39 UTC (permalink / raw)
To: linux-sctp
On Sat, Oct 26, 2013 at 08:55:40PM +0200, Daniel Borkmann wrote:
> 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@redhat.com>
> >>---
> >> 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.
Hi Daniel,
I am happy to take the 1st patch of the series as-is.
Let me know if that works for you.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-28 1:39 ` Simon Horman
0 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 1:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Julian Anastasov, lvs-devel, linux-sctp
On Sat, Oct 26, 2013 at 08:55:40PM +0200, Daniel Borkmann wrote:
> 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@redhat.com>
> >>---
> >> 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.
Hi Daniel,
I am happy to take the 1st patch of the series as-is.
Let me know if that works for you.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-28 1:39 ` Simon Horman
@ 2013-10-28 8:11 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-28 8:11 UTC (permalink / raw)
To: linux-sctp
On 10/28/2013 02:39 AM, Simon Horman wrote:
> I am happy to take the 1st patch of the series as-is.
> Let me know if that works for you.
Yes, sure. Then, I'll only resend the 2nd one today.
Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-28 8:11 ` Julian Anastasov
-1 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-28 8:11 UTC (permalink / raw)
To: linux-sctp
Hello,
On Fri, 25 Oct 2013, Daniel Borkmann wrote:
> + /* Only update csum if we really have to */
> + if (sctph->source != cp->vport || payload_csum) {
> + sctph->source = cp->vport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
The 'if' check for snat_handler should be:
if (sctph->source != cp->vport || payload_csum ||
skb->ip_summed = CHECKSUM_PARTIAL) {
Because we can see CHECKSUM_PARTIAL from virtual devices,
not only from LOCAL_OUT. That is why I removed the '!skb->dev'
check.
The 'else' part should be:
} else {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
> + /* Only update csum if we really have to */
> + if (sctph->dest != cp->dport || payload_csum) {
> + sctph->dest = cp->dport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
The 'else' part for dnat_handler should be:
} else if (skb->ip_summed != CHECKSUM_PARTIAL) {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
Because:
- CHECKSUM_COMPLETE: we should not see it for SCTP, in fact
the small set of drivers that support SCTP offloading return
CHECKSUM_UNNECESSARY on correctly received SCTP csum
- CHECKSUM_PARTIAL: we can see it from local stack or
received from virtual drivers. Checks in the 'if' part
will decide whether it is ok to keep CHECKSUM_PARTIAL for
the output
- CHECKSUM_NONE: packet was 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).
- CHECKSUM_UNNECESSARY: csum was valid on receive
The 'if' part for dnat_handler can be:
if (sctph->dest != cp->dport || payload_csum ||
(skb->ip_summed = CHECKSUM_PARTIAL &&
!(skb_dst(skb)->dev->features & NETIF_F_SCTP_CSUM))) {
I.e. we keep CHECKSUM_PARTIAL only if the new device
supports offloading.
One thing to note: 'lo' does not seem to include
NETIF_F_SCTP_CSUM, so full checksum is calculated.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-28 8:11 ` Julian Anastasov
0 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-28 8:11 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
Hello,
On Fri, 25 Oct 2013, Daniel Borkmann wrote:
> + /* Only update csum if we really have to */
> + if (sctph->source != cp->vport || payload_csum) {
> + sctph->source = cp->vport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
The 'if' check for snat_handler should be:
if (sctph->source != cp->vport || payload_csum ||
skb->ip_summed == CHECKSUM_PARTIAL) {
Because we can see CHECKSUM_PARTIAL from virtual devices,
not only from LOCAL_OUT. That is why I removed the '!skb->dev'
check.
The 'else' part should be:
} else {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
> + /* Only update csum if we really have to */
> + if (sctph->dest != cp->dport || payload_csum) {
> + sctph->dest = cp->dport;
> + sctp_nat_csum(skb, sctph, sctphoff);
> + }
The 'else' part for dnat_handler should be:
} else if (skb->ip_summed != CHECKSUM_PARTIAL) {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
Because:
- CHECKSUM_COMPLETE: we should not see it for SCTP, in fact
the small set of drivers that support SCTP offloading return
CHECKSUM_UNNECESSARY on correctly received SCTP csum
- CHECKSUM_PARTIAL: we can see it from local stack or
received from virtual drivers. Checks in the 'if' part
will decide whether it is ok to keep CHECKSUM_PARTIAL for
the output
- CHECKSUM_NONE: packet was 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).
- CHECKSUM_UNNECESSARY: csum was valid on receive
The 'if' part for dnat_handler can be:
if (sctph->dest != cp->dport || payload_csum ||
(skb->ip_summed == CHECKSUM_PARTIAL &&
!(skb_dst(skb)->dev->features & NETIF_F_SCTP_CSUM))) {
I.e. we keep CHECKSUM_PARTIAL only if the new device
supports offloading.
One thing to note: 'lo' does not seem to include
NETIF_F_SCTP_CSUM, so full checksum is calculated.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-28 8:11 ` Daniel Borkmann
@ 2013-10-28 8:50 ` Simon Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 8:50 UTC (permalink / raw)
To: linux-sctp
On Mon, Oct 28, 2013 at 09:11:41AM +0100, Daniel Borkmann wrote:
> On 10/28/2013 02:39 AM, Simon Horman wrote:
>
> >I am happy to take the 1st patch of the series as-is.
> >Let me know if that works for you.
>
> Yes, sure. Then, I'll only resend the 2nd one today.
Thanks, I have pushed it to ipvs-next.
I will hold off on sending a pull-request to Pablo until you send
the 2nd patch.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-28 8:50 ` Simon Horman
0 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 8:50 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Julian Anastasov, lvs-devel, linux-sctp
On Mon, Oct 28, 2013 at 09:11:41AM +0100, Daniel Borkmann wrote:
> On 10/28/2013 02:39 AM, Simon Horman wrote:
>
> >I am happy to take the 1st patch of the series as-is.
> >Let me know if that works for you.
>
> Yes, sure. Then, I'll only resend the 2nd one today.
Thanks, I have pushed it to ipvs-next.
I will hold off on sending a pull-request to Pablo until you send
the 2nd patch.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-28 8:50 ` Simon Horman
@ 2013-10-28 9:00 ` Julian Anastasov
-1 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-28 9:00 UTC (permalink / raw)
To: linux-sctp
Hello,
On Mon, 28 Oct 2013, Simon Horman wrote:
> Thanks, I have pushed it to ipvs-next.
>
> I will hold off on sending a pull-request to Pablo until you send
> the 2nd patch.
First patch is a bugfix, it should go to stable
kernels. The second patch looks like an optimization.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-28 9:00 ` Julian Anastasov
0 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-28 9:00 UTC (permalink / raw)
To: Simon Horman; +Cc: Daniel Borkmann, lvs-devel, linux-sctp
Hello,
On Mon, 28 Oct 2013, Simon Horman wrote:
> Thanks, I have pushed it to ipvs-next.
>
> I will hold off on sending a pull-request to Pablo until you send
> the 2nd patch.
First patch is a bugfix, it should go to stable
kernels. The second patch looks like an optimization.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
2013-10-28 9:00 ` Julian Anastasov
@ 2013-10-28 9:38 ` Simon Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 9:38 UTC (permalink / raw)
To: linux-sctp
[ Cc: Pablo ]
On Mon, Oct 28, 2013 at 11:00:37AM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 28 Oct 2013, Simon Horman wrote:
>
> > Thanks, I have pushed it to ipvs-next.
> >
> > I will hold off on sending a pull-request to Pablo until you send
> > the 2nd patch.
>
> First patch is a bugfix, it should go to stable
> kernels. The second patch looks like an optimization.
Good point.
Pablo, what is the best way to handle this?
Should I send the fix as a pull-request for nf or nf-next?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed
@ 2013-10-28 9:38 ` Simon Horman
0 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-28 9:38 UTC (permalink / raw)
To: Julian Anastasov
Cc: Daniel Borkmann, lvs-devel, linux-sctp, Pablo Neira Ayuso
[ Cc: Pablo ]
On Mon, Oct 28, 2013 at 11:00:37AM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 28 Oct 2013, Simon Horman wrote:
>
> > Thanks, I have pushed it to ipvs-next.
> >
> > I will hold off on sending a pull-request to Pablo until you send
> > the 2nd patch.
>
> First patch is a bugfix, it should go to stable
> kernels. The second patch looks like an optimization.
Good point.
Pablo, what is the best way to handle this?
Should I send the fix as a pull-request for nf or nf-next?
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 9:05 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-25 9:05 UTC (permalink / raw)
To: horms; +Cc: lvs-devel, linux-sctp
If skb_header_pointer() fails, we need to assign a verdict, that is
NF_DROP in this case, otherwise, we would leave the verdict from
conn_schedule() uninitialized when returning.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/netfilter/ipvs/ip_vs_proto_sctp.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 23e596e..9ca7aa0 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -20,13 +20,18 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
sctp_sctphdr_t *sh, _sctph;
sh = skb_header_pointer(skb, iph->len, sizeof(_sctph), &_sctph);
- if (sh == NULL)
+ if (sh == NULL) {
+ *verdict = NF_DROP;
return 0;
+ }
sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
sizeof(_schunkh), &_schunkh);
- if (sch == NULL)
+ if (sch == NULL) {
+ *verdict = NF_DROP;
return 0;
+ }
+
net = skb_net(skb);
ipvs = net_ipvs(net);
rcu_read_lock();
--
1.7.11.7
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 9:39 ` Jesper Dangaard Brouer
-1 siblings, 0 replies; 34+ messages in thread
From: Jesper Dangaard Brouer @ 2013-10-25 9:39 UTC (permalink / raw)
To: linux-sctp
On Fri, 25 Oct 2013 11:05:04 +0200
Daniel Borkmann <dborkman@redhat.com> wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
It looks like a good ide, and resembles how we handle these situations
else were in the IPVS code (e.g. for TCP and UDP).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 9:39 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 34+ messages in thread
From: Jesper Dangaard Brouer @ 2013-10-25 9:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
On Fri, 25 Oct 2013 11:05:04 +0200
Daniel Borkmann <dborkman@redhat.com> wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
It looks like a good ide, and resembles how we handle these situations
else were in the IPVS code (e.g. for TCP and UDP).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
2013-10-25 9:39 ` Jesper Dangaard Brouer
@ 2013-10-25 9:55 ` Simon Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-25 9:55 UTC (permalink / raw)
To: linux-sctp
On Fri, Oct 25, 2013 at 11:39:02AM +0200, Jesper Dangaard Brouer wrote:
> On Fri, 25 Oct 2013 11:05:04 +0200
> Daniel Borkmann <dborkman@redhat.com> wrote:
>
> > If skb_header_pointer() fails, we need to assign a verdict, that is
> > NF_DROP in this case, otherwise, we would leave the verdict from
> > conn_schedule() uninitialized when returning.
> >
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > ---
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> It looks like a good ide, and resembles how we handle these situations
> else were in the IPVS code (e.g. for TCP and UDP).
Likeiwse.
I am wondering if this resolves a but and if so how severe it is.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 9:55 ` Simon Horman
0 siblings, 0 replies; 34+ messages in thread
From: Simon Horman @ 2013-10-25 9:55 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: Daniel Borkmann, lvs-devel, linux-sctp
On Fri, Oct 25, 2013 at 11:39:02AM +0200, Jesper Dangaard Brouer wrote:
> On Fri, 25 Oct 2013 11:05:04 +0200
> Daniel Borkmann <dborkman@redhat.com> wrote:
>
> > If skb_header_pointer() fails, we need to assign a verdict, that is
> > NF_DROP in this case, otherwise, we would leave the verdict from
> > conn_schedule() uninitialized when returning.
> >
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > ---
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> It looks like a good ide, and resembles how we handle these situations
> else were in the IPVS code (e.g. for TCP and UDP).
Likeiwse.
I am wondering if this resolves a but and if so how severe it is.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
2013-10-25 9:55 ` Simon Horman
@ 2013-10-25 10:03 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-25 10:03 UTC (permalink / raw)
To: linux-sctp
On 10/25/2013 11:55 AM, Simon Horman wrote:
> On Fri, Oct 25, 2013 at 11:39:02AM +0200, Jesper Dangaard Brouer wrote:
>> On Fri, 25 Oct 2013 11:05:04 +0200
>> Daniel Borkmann <dborkman@redhat.com> wrote:
>>
>>> If skb_header_pointer() fails, we need to assign a verdict, that is
>>> NF_DROP in this case, otherwise, we would leave the verdict from
>>> conn_schedule() uninitialized when returning.
>>>
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> ---
>>
>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> It looks like a good ide, and resembles how we handle these situations
>> else were in the IPVS code (e.g. for TCP and UDP).
>
> Likeiwse.
>
> I am wondering if this resolves a but and if so how severe it is.
Probably with malformed SCTP INIT packets, but haven't tried so far.
Just found it during code review.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 10:03 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-25 10:03 UTC (permalink / raw)
To: Simon Horman; +Cc: Jesper Dangaard Brouer, lvs-devel, linux-sctp
On 10/25/2013 11:55 AM, Simon Horman wrote:
> On Fri, Oct 25, 2013 at 11:39:02AM +0200, Jesper Dangaard Brouer wrote:
>> On Fri, 25 Oct 2013 11:05:04 +0200
>> Daniel Borkmann <dborkman@redhat.com> wrote:
>>
>>> If skb_header_pointer() fails, we need to assign a verdict, that is
>>> NF_DROP in this case, otherwise, we would leave the verdict from
>>> conn_schedule() uninitialized when returning.
>>>
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> ---
>>
>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> It looks like a good ide, and resembles how we handle these situations
>> else were in the IPVS code (e.g. for TCP and UDP).
>
> Likeiwse.
>
> I am wondering if this resolves a but and if so how severe it is.
Probably with malformed SCTP INIT packets, but haven't tried so far.
Just found it during code review.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 12:59 ` Neil Horman
-1 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2013-10-25 12:59 UTC (permalink / raw)
To: linux-sctp
On Fri, Oct 25, 2013 at 11:05:04AM +0200, Daniel Borkmann wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 23e596e..9ca7aa0 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -20,13 +20,18 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
> sctp_sctphdr_t *sh, _sctph;
>
> sh = skb_header_pointer(skb, iph->len, sizeof(_sctph), &_sctph);
> - if (sh = NULL)
> + if (sh = NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
>
> sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> sizeof(_schunkh), &_schunkh);
> - if (sch = NULL)
> + if (sch = NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
> +
> net = skb_net(skb);
> ipvs = net_ipvs(net);
> rcu_read_lock();
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 12:59 ` Neil Horman
0 siblings, 0 replies; 34+ messages in thread
From: Neil Horman @ 2013-10-25 12:59 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
On Fri, Oct 25, 2013 at 11:05:04AM +0200, Daniel Borkmann wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 23e596e..9ca7aa0 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -20,13 +20,18 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
> sctp_sctphdr_t *sh, _sctph;
>
> sh = skb_header_pointer(skb, iph->len, sizeof(_sctph), &_sctph);
> - if (sh == NULL)
> + if (sh == NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
>
> sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> sizeof(_schunkh), &_schunkh);
> - if (sch == NULL)
> + if (sch == NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
> +
> net = skb_net(skb);
> ipvs = net_ipvs(net);
> rcu_read_lock();
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
2013-10-25 9:05 ` Daniel Borkmann
@ 2013-10-25 21:05 ` Julian Anastasov
-1 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-25 21:05 UTC (permalink / raw)
To: linux-sctp
Hello,
On Fri, 25 Oct 2013, Daniel Borkmann wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Looks good,
Acked-by: Julian Anastasov <ja@ssi.bg>
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 23e596e..9ca7aa0 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -20,13 +20,18 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
> sctp_sctphdr_t *sh, _sctph;
>
> sh = skb_header_pointer(skb, iph->len, sizeof(_sctph), &_sctph);
> - if (sh = NULL)
> + if (sh = NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
>
> sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> sizeof(_schunkh), &_schunkh);
> - if (sch = NULL)
> + if (sch = NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
> +
> net = skb_net(skb);
> ipvs = net_ipvs(net);
> rcu_read_lock();
> --
> 1.7.11.7
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule
@ 2013-10-25 21:05 ` Julian Anastasov
0 siblings, 0 replies; 34+ messages in thread
From: Julian Anastasov @ 2013-10-25 21:05 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: horms, lvs-devel, linux-sctp
Hello,
On Fri, 25 Oct 2013, Daniel Borkmann wrote:
> If skb_header_pointer() fails, we need to assign a verdict, that is
> NF_DROP in this case, otherwise, we would leave the verdict from
> conn_schedule() uninitialized when returning.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Looks good,
Acked-by: Julian Anastasov <ja@ssi.bg>
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 23e596e..9ca7aa0 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -20,13 +20,18 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
> sctp_sctphdr_t *sh, _sctph;
>
> sh = skb_header_pointer(skb, iph->len, sizeof(_sctph), &_sctph);
> - if (sh == NULL)
> + if (sh == NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
>
> sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> sizeof(_schunkh), &_schunkh);
> - if (sch == NULL)
> + if (sch == NULL) {
> + *verdict = NF_DROP;
> return 0;
> + }
> +
> net = skb_net(skb);
> ipvs = net_ipvs(net);
> rcu_read_lock();
> --
> 1.7.11.7
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 34+ messages in thread