All of lore.kernel.org
 help / color / mirror / Atom feed
* [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: 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
* [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
* [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: 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
* [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 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 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 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 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 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
* 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-28  8:11   ` Daniel Borkmann
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2013-10-28  8:11 UTC (permalink / raw)
  To: Simon Horman; +Cc: Julian Anastasov, lvs-devel, 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
end of thread, other threads:[~2013-10-28  9:38 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-25  9:05 [PATCH ipvs 1/2] net: ipvs: sctp: add missing verdict assignments in sctp_conn_schedule Daniel Borkmann
2013-10-25  9:05 ` Daniel Borkmann
2013-10-25  9:39 ` Jesper Dangaard Brouer
2013-10-25  9:39   ` Jesper Dangaard Brouer
2013-10-25  9:55 ` Simon Horman
2013-10-25  9:55   ` Simon Horman
2013-10-25 10:03 ` Daniel Borkmann
2013-10-25 10:03   ` Daniel Borkmann
2013-10-25 12:59 ` Neil Horman
2013-10-25 12:59   ` Neil Horman
2013-10-25 21:05 ` Julian Anastasov
2013-10-25 21:05   ` Julian Anastasov
2013-10-25  9:05 [PATCH ipvs 2/2] net: ipvs: sctp: do not recalc sctp checksum when not needed Daniel Borkmann
2013-10-25  9:05 ` Daniel Borkmann
2013-10-25  9:48 ` Jesper Dangaard Brouer
2013-10-25  9:48   ` Jesper Dangaard Brouer
2013-10-25 13:01 ` Neil Horman
2013-10-25 13:01   ` Neil Horman
2013-10-25 21:00 ` Julian Anastasov
2013-10-25 21:00   ` Julian Anastasov
2013-10-26 18:55 ` Daniel Borkmann
2013-10-26 18:55   ` Daniel Borkmann
2013-10-28  1:39 ` Simon Horman
2013-10-28  1:39   ` Simon Horman
2013-10-28  8:11 ` Daniel Borkmann
2013-10-28  8:11   ` Daniel Borkmann
2013-10-28  8:11 ` Julian Anastasov
2013-10-28  8:11   ` Julian Anastasov
2013-10-28  8:50 ` Simon Horman
2013-10-28  8:50   ` Simon Horman
2013-10-28  9:00 ` Julian Anastasov
2013-10-28  9:00   ` Julian Anastasov
2013-10-28  9:38 ` Simon Horman
2013-10-28  9:38   ` Simon Horman

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.