[GIT PULL nf] IPVS fixes for v3.10

[GIT PULL nf] IPVS fixes for v3.10

[Simon Horman]

Hi Pablo,

The following changes since commit 4f36ea6eed2081340c7a7aa98c73187ecfccebff:

  netfilter: ipt_ULOG: fix non-null terminated string in the nf_log path (2013-05-23 14:25:40 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git tags/ipvs-fixes-for-v3.10

for you to fetch changes up to 1fb4f80a593231770eae3192e4a52fa7631bd897:

  ipvs: Fix reuse connection if real server is dead (2013-05-26 08:10:47 +0900)

----------------------------------------------------------------
IPVS fixes for v3.10

Correct connection reuse for TCP and SCTP

----------------------------------------------------------------
Grzegorz Lyczba (1):
      ipvs: Fix reuse connection if real server is dead

 net/netfilter/ipvs/ip_vs_core.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

[PATCH] ipvs: Fix reuse connection if real server is dead

[Simon Horman]

From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>

Expire cached connection for new TCP/SCTP connection if real server is down

Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
Acked-by: Hans Schillstrom <hans@schillstrom.com>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 net/netfilter/ipvs/ip_vs_core.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 085b588..05565d2 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1001,6 +1001,32 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
 	return th->rst;
 }
 
+static inline bool is_new_conn(const struct sk_buff *skb,
+			       struct ip_vs_iphdr *iph)
+{
+	switch (iph->protocol) {
+	case IPPROTO_TCP: {
+		struct tcphdr _tcph, *th;
+
+		th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
+		if (th == NULL)
+			return false;
+		return th->syn;
+	}
+	case IPPROTO_SCTP: {
+		sctp_chunkhdr_t *sch, schunk;
+
+		sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
+					 sizeof(schunk), &schunk);
+		if (sch == NULL)
+			return false;
+		return sch->type == SCTP_CID_INIT;
+	}
+	default:
+		return false;
+	}
+}
+
 /* Handle response packets: rewrite addresses and send away...
  */
 static unsigned int
@@ -1612,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
 	 * Check if the packet belongs to an existing connection entry
 	 */
 	cp = pp->conn_in_get(af, skb, &iph, 0);
+
+	if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
+	    unlikely(!atomic_read(&cp->dest->weight)) && !iph.fragoffs &&
+	    is_new_conn(skb, &iph)) {
+		ip_vs_conn_expire_now(cp);
+		__ip_vs_conn_put(cp);
+		cp = NULL;
+	}
+
 	if (unlikely(!cp) && !iph.fragoffs) {
 		/* No (second) fragments need to enter here, as nf_defrag_ipv6
 		 * replayed fragment zero will already have created the cp
-- 
1.7.10.4

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Pablo Neira Ayuso]

On Sun, May 26, 2013 at 08:16:39AM +0900, Simon Horman wrote:
> From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> 
> Expire cached connection for new TCP/SCTP connection if real server is down

Applied, thanks Simon.

I have mangled the description to insist on the fact that newly
(reused) connections break as they will be directed to the dead
server.

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Simon Horman]

On Mon, May 27, 2013 at 01:49:11PM +0200, Pablo Neira Ayuso wrote:
> On Sun, May 26, 2013 at 08:16:39AM +0900, Simon Horman wrote:
> > From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> > 
> > Expire cached connection for new TCP/SCTP connection if real server is down
> 
> Applied, thanks Simon.
> 
> I have mangled the description to insist on the fact that newly
> (reused) connections break as they will be directed to the dead
> server.

Thanks.

[PATCH] ipvs: Fix reuse connection if real server is dead

[Grzegorz Lyczba]

Expire cached connection for new TCP connection if real server is down

Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
---
 net/netfilter/ipvs/ip_vs_core.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 47edf5a..7c5f083 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
 	return th->rst;
 }
 
+static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
+{
+    struct tcphdr _tcph, *th;
+
+    th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
+    if (th == NULL)
+        return 0;
+    return th->syn;
+}
+
+
 /* Handle response packets: rewrite addresses and send away...
  */
 static unsigned int
@@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
 	 * Check if the packet belongs to an existing connection entry
 	 */
 	cp = pp->conn_in_get(af, skb, &iph, 0);
+
+	if (likely(cp) && likely(cp->dest) &&
+	    unlikely(!atomic_read(&cp->dest->weight)) &&
+	    iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
+		ip_vs_conn_expire_now(cp);
+		__ip_vs_conn_put(cp);
+		cp = NULL;
+	}
+
 	if (unlikely(!cp) && !iph.fragoffs) {
 		/* No (second) fragments need to enter here, as nf_defrag_ipv6
 		 * replayed fragment zero will already have created the cp
-- 
1.7.5.4

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

Hello
On Tue, 2013-05-07 at 23:54 +0200, Grzegorz Lyczba wrote:
> Expire cached connection for new TCP connection if real server is down

This patch will break some "common" use cases,
on the other hand it also solves some issues...
 
I would prefer to have a sysctl to turn it on.

Should we have something similar for sctp?


Regards
Hans

> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_core.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..7c5f083 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
>  	return th->rst;
>  }
>  
> +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> +{
> +    struct tcphdr _tcph, *th;
> +
> +    th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
> +    if (th == NULL)
> +        return 0;
> +    return th->syn;
> +}
> +
> +
>  /* Handle response packets: rewrite addresses and send away...
>   */
>  static unsigned int
> @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
>  	 * Check if the packet belongs to an existing connection entry
>  	 */
>  	cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> +	if (likely(cp) && likely(cp->dest) &&
> +	    unlikely(!atomic_read(&cp->dest->weight)) &&
> +	    iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> +		ip_vs_conn_expire_now(cp);
> +		__ip_vs_conn_put(cp);
> +		cp = NULL;
> +	}
> +
>  	if (unlikely(!cp) && !iph.fragoffs) {
>  		/* No (second) fragments need to enter here, as nf_defrag_ipv6
>  		 * replayed fragment zero will already have created the cp


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Julian Anastasov]

	Hello,

On Tue, 7 May 2013, Grzegorz Lyczba wrote:

> Expire cached connection for new TCP connection if real server is down
> 
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_core.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..7c5f083 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
>  	return th->rst;
>  }
>  
> +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> +{
> +    struct tcphdr _tcph, *th;
> +
> +    th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);

	As Hans notes, it is a good idea to support SCTP by
using the iph address as second argument. May be the func can
be named is_new_conn:

static inline bool is_new_conn(const struct sk_buff *skb,
			       struct ip_vs_iphdr *iph)
...

> +    if (th == NULL)
> +        return 0;
> +    return th->syn;
> +}
> +
> +
>  /* Handle response packets: rewrite addresses and send away...
>   */
>  static unsigned int
> @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
>  	 * Check if the packet belongs to an existing connection entry
>  	 */
>  	cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> +	if (likely(cp) && likely(cp->dest) &&

	I hope it is appropriate to use the expire_nodest_conn flag:

	if (unlikely(sysctl_expire_nodest_conn(ipvs) && cp && cp->dest &&
...

> +	    unlikely(!atomic_read(&cp->dest->weight)) &&

	We should add '!iph.fragoffs &&' here.

> +	    iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> +		ip_vs_conn_expire_now(cp);
> +		__ip_vs_conn_put(cp);
> +		cp = NULL;
> +	}
> +
>  	if (unlikely(!cp) && !iph.fragoffs) {
>  		/* No (second) fragments need to enter here, as nf_defrag_ipv6
>  		 * replayed fragment zero will already have created the cp

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]



On Wed, 2013-05-08 at 10:35 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 7 May 2013, Grzegorz Lyczba wrote:
> 
> > Expire cached connection for new TCP connection if real server is down
> > 
> > Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> > ---
> >  net/netfilter/ipvs/ip_vs_core.c |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index 47edf5a..7c5f083 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> >  	return th->rst;
> >  }
> >  
> > +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> > +{
> > +    struct tcphdr _tcph, *th;
> > +
> > +    th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
> 
> 	As Hans notes, it is a good idea to support SCTP by
> using the iph address as second argument. May be the func can
> be named is_new_conn:
> 
> static inline bool is_new_conn(const struct sk_buff *skb,
> 			       struct ip_vs_iphdr *iph)
> ...
> 
> > +    if (th == NULL)
> > +        return 0;
> > +    return th->syn;
> > +}
> > +
> > +
> >  /* Handle response packets: rewrite addresses and send away...
> >   */
> >  static unsigned int
> > @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> >  	 * Check if the packet belongs to an existing connection entry
> >  	 */
> >  	cp = pp->conn_in_get(af, skb, &iph, 0);
> > +
> > +	if (likely(cp) && likely(cp->dest) &&
> 
> 	I hope it is appropriate to use the expire_nodest_conn flag:
> 
> 	if (unlikely(sysctl_expire_nodest_conn(ipvs) && cp && cp->dest &&
> ...
> 
> > +	    unlikely(!atomic_read(&cp->dest->weight)) &&
> 
> 	We should add '!iph.fragoffs &&' here.

Do we really need that?
I don't think a SYN can be fragmented.


Regards
Hans
> 
> > +	    iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> > +		ip_vs_conn_expire_now(cp);
> > +		__ip_vs_conn_put(cp);
> > +		cp = NULL;
> > +	}
> > +
> >  	if (unlikely(!cp) && !iph.fragoffs) {
> >  		/* No (second) fragments need to enter here, as nf_defrag_ipv6
> >  		 * replayed fragment zero will already have created the cp
> 
> 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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Julian Anastasov]

	Hello,

On Wed, 8 May 2013, Hans Schillstrom wrote:

> > > +	    unlikely(!atomic_read(&cp->dest->weight)) &&
> > 
> > 	We should add '!iph.fragoffs &&' here.
> 
> Do we really need that?
> I don't think a SYN can be fragmented.

	SYNs are usually not fragmented. But we don't have the
right to search for SYN in a packet without TCP header. It can
be data fragment that contains 1 at this bit position, when
weight=0 such packets should be forwarded as part of existing
connection.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Hans Schillstrom]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]



On Wed, 2013-05-08 at 11:53 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Wed, 8 May 2013, Hans Schillstrom wrote:
> 
> > > > +	    unlikely(!atomic_read(&cp->dest->weight)) &&
> > > 
> > > 	We should add '!iph.fragoffs &&' here.
> > 
> > Do we really need that?
> > I don't think a SYN can be fragmented.
> 
> 	SYNs are usually not fragmented. But we don't have the
> right to search for SYN in a packet without TCP header. It can
> be data fragment that contains 1 at this bit position, when
> weight=0 such packets should be forwarded as part of existing
> connection.
> 

Shame on me, I should really have thought about that...
I think I know who invented the iph.fragoffs :-)

Regards
Hans

> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]

[PATCH] ipvs: Fix reuse connection if real server is dead

[Grzegorz Łyczba]

From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>

Expire cached connection for new TCP/SCTP connection if real server is down

Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
---
 net/netfilter/ipvs/ip_vs_core.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 47edf5a..9ee8b70 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
 	return th->rst;
 }
 
+static inline bool is_new_conn(const struct sk_buff *skb,
+	    struct ip_vs_iphdr *iph)
+{
+	switch (iph->protocol) {
+	case IPPROTO_TCP: {
+		struct tcphdr _tcph, *th;
+		th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
+		if (th == NULL) {
+			return false;
+		}
+		return th->syn;
+	}
+	case IPPROTO_SCTP: {
+		sctp_chunkhdr_t *sch, schunk;
+		sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
+				sizeof(schunk), &schunk);
+		if (sch == NULL) {
+			return false;
+		}
+		return (sch->type == SCTP_CID_INIT);
+	}
+	default:
+		return false;
+	}
+}
+
+
 /* Handle response packets: rewrite addresses and send away...
  */
 static unsigned int
@@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
 	 * Check if the packet belongs to an existing connection entry
 	 */
 	cp = pp->conn_in_get(af, skb, &iph, 0);
+
+	if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
+	    !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&
+	    is_new_conn(skb, &iph)) {
+		ip_vs_conn_expire_now(cp);
+		__ip_vs_conn_put(cp);
+		cp = NULL;
+	}
+
 	if (unlikely(!cp) && !iph.fragoffs) {
 		/* No (second) fragments need to enter here, as nf_defrag_ipv6
 		 * replayed fragment zero will already have created the cp
-- 
1.7.5.4

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Julian Anastasov]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3483 bytes --]


	Hello,

On Sat, 11 May 2013, Grzegorz Łyczba wrote:

> From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> 
> Expire cached connection for new TCP/SCTP connection if real server is down
> 
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>

	The change looks correct but you have to fix some
coding style issues. Otherwise, patch will be rejected.
You can also check Documentation/CodingStyle

# scripts/checkpatch.pl /tmp/patch.diff
WARNING: braces {} are not necessary for single statement blocks
#25: FILE: net/netfilter/ipvs/ip_vs_core.c:1012:
+               if (th == NULL) {
+                       return false;
+               }

WARNING: braces {} are not necessary for single statement blocks
#34: FILE: net/netfilter/ipvs/ip_vs_core.c:1021:
+               if (sch == NULL) {
+                       return false;
+               }

total: 0 errors, 2 warnings, 48 lines checked

> ---
>  net/netfilter/ipvs/ip_vs_core.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..9ee8b70 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
>  	return th->rst;
>  }
>  
> +static inline bool is_new_conn(const struct sk_buff *skb,
> +	    struct ip_vs_iphdr *iph)

	Above "struct" should be at the same column as "const"
in first argument, like this:

static inline bool is_new_conn(const struct sk_buff *skb,
			       struct ip_vs_iphdr *iph)

> +{
> +	switch (iph->protocol) {

	I'm still not sure if this construct is allowed:

	case ...: {
	}

	May be it should be:

	case ...:
		{
		}
	case ...

	but CodingStyle does not include rules about it.
If in doubt, you can use 'if' instead of 'switch' without
adding new indentation.

> +	case IPPROTO_TCP: {
> +		struct tcphdr _tcph, *th;

	empty line is needed here

> +		th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> +		if (th == NULL) {
> +			return false;

	extra braces

> +		}
> +		return th->syn;
> +	}
> +	case IPPROTO_SCTP: {
> +		sctp_chunkhdr_t *sch, schunk;

	empty line is needed here

> +		sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> +				sizeof(schunk), &schunk);
> +		if (sch == NULL) {
> +			return false;

	extra braces

> +		}
> +		return (sch->type == SCTP_CID_INIT);
> +	}
> +	default:
> +		return false;
> +	}
> +}
> +
> +
>  /* Handle response packets: rewrite addresses and send away...
>   */
>  static unsigned int
> @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
>  	 * Check if the packet belongs to an existing connection entry
>  	 */
>  	cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> +	if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> +	    !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&

	Lets move '!iph.fragoffs' after the
'unlikely(!atomic_read(&cp->dest->weight))' because fragoffs
is usually 0. Only IPv6 can see fragments at that point.

> +	    is_new_conn(skb, &iph)) {
> +		ip_vs_conn_expire_now(cp);
> +		__ip_vs_conn_put(cp);
> +		cp = NULL;
> +	}
> +
>  	if (unlikely(!cp) && !iph.fragoffs) {
>  		/* No (second) fragments need to enter here, as nf_defrag_ipv6
>  		 * replayed fragment zero will already have created the cp
> -- 
> 1.7.5.4

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Simon Horman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1252", Size: 4052 bytes --]

On Sat, May 11, 2013 at 04:23:33PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 11 May 2013, Grzegorz Łyczba wrote:
> 
> > From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> > 
> > Expire cached connection for new TCP/SCTP connection if real server is down
> > 
> > Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> 
> 	The change looks correct but you have to fix some
> coding style issues. Otherwise, patch will be rejected.
> You can also check Documentation/CodingStyle
> 
> # scripts/checkpatch.pl /tmp/patch.diff
> WARNING: braces {} are not necessary for single statement blocks
> #25: FILE: net/netfilter/ipvs/ip_vs_core.c:1012:
> +               if (th == NULL) {
> +                       return false;
> +               }
> 
> WARNING: braces {} are not necessary for single statement blocks
> #34: FILE: net/netfilter/ipvs/ip_vs_core.c:1021:
> +               if (sch == NULL) {
> +                       return false;
> +               }
> 
> total: 0 errors, 2 warnings, 48 lines checked
> 
> > ---
> >  net/netfilter/ipvs/ip_vs_core.c |   36 ++++++++++++++++++++++++++++++++++++
> >  1 files changed, 36 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index 47edf5a..9ee8b70 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> >  	return th->rst;
> >  }
> >  
> > +static inline bool is_new_conn(const struct sk_buff *skb,
> > +	    struct ip_vs_iphdr *iph)
> 
> 	Above "struct" should be at the same column as "const"
> in first argument, like this:
> 
> static inline bool is_new_conn(const struct sk_buff *skb,
> 			       struct ip_vs_iphdr *iph)
> 
> > +{
> > +	switch (iph->protocol) {
> 
> 	I'm still not sure if this construct is allowed:
> 
> 	case ...: {
> 	}

I see some evidence of the above style in net/core/
so personally I am fine with it.

> 
> 	May be it should be:
> 
> 	case ...:
> 		{
> 		}
> 	case ...
> 
> 	but CodingStyle does not include rules about it.
> If in doubt, you can use 'if' instead of 'switch' without
> adding new indentation.
> 
> > +	case IPPROTO_TCP: {
> > +		struct tcphdr _tcph, *th;
> 
> 	empty line is needed here
> 
> > +		th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> > +		if (th == NULL) {
> > +			return false;
> 
> 	extra braces
> 
> > +		}
> > +		return th->syn;
> > +	}
> > +	case IPPROTO_SCTP: {
> > +		sctp_chunkhdr_t *sch, schunk;
> 
> 	empty line is needed here
> 
> > +		sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> > +				sizeof(schunk), &schunk);
> > +		if (sch == NULL) {
> > +			return false;
> 
> 	extra braces
> 
> > +		}
> > +		return (sch->type == SCTP_CID_INIT);
> > +	}
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +
> >  /* Handle response packets: rewrite addresses and send away...
> >   */
> >  static unsigned int
> > @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> >  	 * Check if the packet belongs to an existing connection entry
> >  	 */
> >  	cp = pp->conn_in_get(af, skb, &iph, 0);
> > +
> > +	if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> > +	    !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&
> 
> 	Lets move '!iph.fragoffs' after the
> 'unlikely(!atomic_read(&cp->dest->weight))' because fragoffs
> is usually 0. Only IPv6 can see fragments at that point.
> 
> > +	    is_new_conn(skb, &iph)) {
> > +		ip_vs_conn_expire_now(cp);
> > +		__ip_vs_conn_put(cp);
> > +		cp = NULL;
> > +	}
> > +
> >  	if (unlikely(!cp) && !iph.fragoffs) {
> >  		/* No (second) fragments need to enter here, as nf_defrag_ipv6
> >  		 * replayed fragment zero will already have created the cp
> > -- 
> > 1.7.5.4
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Jesper Dangaard Brouer]

On Sat, 11 May 2013 16:23:33 +0300 (EEST) Julian Anastasov <ja@ssi.bg>
wrote:
 
> 	The change looks correct but you have to fix some
> coding style issues. Otherwise, patch will be rejected.
> You can also check Documentation/CodingStyle

Also remember to add a "version" tag to the subject, when you resubmit.

Subject like:
 [PATCH v3] ipvs: Fix reuse connection if real server is dead


-- 
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