[PATCH 1/3] ipvs: use SYNC_MESG_HEADER_LEN instead of explicit header length

[PATCH 1/3] ipvs: use SYNC_MESG_HEADER_LEN instead of explicit header length

[Tinggong Wang]

Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
---
 net/netfilter/ipvs/ip_vs_sync.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index ab85aed..7632a17 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -102,7 +102,6 @@ struct ip_vs_sync_thread_data {
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 */
 
-#define SYNC_MESG_HEADER_LEN	4
 #define MAX_CONNS_PER_SYNCBUFF	255 /* nr_conns in ip_vs_sync_mesg is 8 bit */
 
 struct ip_vs_sync_mesg {
@@ -112,6 +111,7 @@ struct ip_vs_sync_mesg {
 
 	/* ip_vs_sync_conn entries start here */
 };
+#define SYNC_MESG_HEADER_LEN	(sizeof(struct ip_vs_sync_mesg))
 
 /* the maximum length of sync (sending/receiving) message */
 static int sync_send_mesg_maxlen;
@@ -188,8 +188,8 @@ static inline struct ip_vs_sync_buff * ip_vs_sync_buff_create(void)
 	}
 	sb->mesg->nr_conns = 0;
 	sb->mesg->syncid = ip_vs_master_syncid;
-	sb->mesg->size = 4;
-	sb->head = (unsigned char *)sb->mesg + 4;
+	sb->mesg->size = SYNC_MESG_HEADER_LEN;
+	sb->head = (unsigned char *)sb->mesg + SYNC_MESG_HEADER_LEN;
 	sb->end = (unsigned char *)sb->mesg + sync_send_mesg_maxlen;
 	sb->firstuse = jiffies;
 	return sb;
@@ -315,7 +315,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
 	char *p;
 	int i;
 
-	if (buflen < sizeof(struct ip_vs_sync_mesg)) {
+	if (buflen < SYNC_MESG_HEADER_LEN) {
 		IP_VS_ERR_RL("sync message header too short\n");
 		return;
 	}
@@ -335,7 +335,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
 		return;
 	}
 
-	p = (char *)buffer + sizeof(struct ip_vs_sync_mesg);
+	p = (char *)buffer + SYNC_MESG_HEADER_LEN;
 	for (i=0; i<m->nr_conns; i++) {
 		unsigned flags, state;
 
-- 
1.7.2.3

[PATCH 2/3] ipvs: check data validation before local_bh_disable

[Tinggong Wang]

Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
---
 net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 7632a17..2b6b0cb 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
 	char *p;
 	int i;
 
-	if (buflen < SYNC_MESG_HEADER_LEN) {
-		IP_VS_ERR_RL("sync message header too short\n");
-		return;
-	}

[PATCH 3/3] ipvs: fix get_curr_sync_buff

[Tinggong Wang]

use time_after get the current buffer created more than the specified time.

time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
if curr_sb has been created more than 2*HZ then it will still sit in master.
so use time_after instead.

Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
---
 net/netfilter/ipvs/ip_vs_sync.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 2b6b0cb..555b0dd 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
 	struct ip_vs_sync_buff *sb;
 
 	spin_lock_bh(&curr_sb_lock);
-	if (curr_sb && (time == 0 ||
-			time_before(jiffies - curr_sb->firstuse, time))) {
+	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
 		sb = curr_sb;
 		curr_sb = NULL;
 	} else
-- 
1.7.2.3

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Simon Horman]

[ CCed Hans Schillstrom and Julian Anastasov ]

On Sun, Dec 12, 2010 at 07:43:04PM +0800, Tinggong Wang wrote:
> use time_after get the current buffer created more than the specified time.
> 
> time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
> if curr_sb has been created more than 2*HZ then it will still sit in master.
> so use time_after instead.
> 
> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_sync.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 2b6b0cb..555b0dd 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
>  	struct ip_vs_sync_buff *sb;
>  
>  	spin_lock_bh(&curr_sb_lock);
> -	if (curr_sb && (time == 0 ||
> -			time_before(jiffies - curr_sb->firstuse, time))) {
> +	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
>  		sb = curr_sb;
>  		curr_sb = NULL;
>  	} else
> -- 
> 1.7.2.3

This change seems correct to me. Hans, can I get an Ack or otherwise from you?

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Hans Schillstrom]

On Sun, 2010-12-12 at 22:49 +0100, Simon Horman wrote:
> [ CCed Hans Schillstrom and Julian Anastasov ]
> 
> On Sun, Dec 12, 2010 at 07:43:04PM +0800, Tinggong Wang wrote:
> > use time_after get the current buffer created more than the specified time.
> > 
> > time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
> > if curr_sb has been created more than 2*HZ then it will still sit in master.
> > so use time_after instead.
> > 
> > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > ---
> >  net/netfilter/ipvs/ip_vs_sync.c |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index 2b6b0cb..555b0dd 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
> >  	struct ip_vs_sync_buff *sb;
> >  
> >  	spin_lock_bh(&curr_sb_lock);
> > -	if (curr_sb && (time == 0 ||
> > -			time_before(jiffies - curr_sb->firstuse, time))) {
> > +	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
> >  		sb = curr_sb;
> >  		curr_sb = NULL;
> >  	} else
> > -- 
> > 1.7.2.3
> 
> This change seems correct to me. Hans, can I get an Ack or otherwise from you?
> 
This is OK except for that it needs a rebase if it's applied after IPv6
and PE patches.

Acked-by: Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Julian Anastasov]

 	Hello,

On Sun, 12 Dec 2010, Tinggong Wang wrote:

> use time_after get the current buffer created more than the specified time.
>
> time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
> if curr_sb has been created more than 2*HZ then it will still sit in master.
> so use time_after instead.
>
> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> ---
> net/netfilter/ipvs/ip_vs_sync.c |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 2b6b0cb..555b0dd 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
> 	struct ip_vs_sync_buff *sb;
>
> 	spin_lock_bh(&curr_sb_lock);
> -	if (curr_sb && (time == 0 ||
> -			time_before(jiffies - curr_sb->firstuse, time))) {
> +	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {

 	This breaks the time=0 case when jiffies matches firstuse.

 	May be the fix should be as follows?:

 	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {

 	i.e. passed >= limit (2 or 0).

> 		sb = curr_sb;
> 		curr_sb = NULL;
> 	} else
> -- 
> 1.7.2.3

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Tinggong Wang]

on Tue, 14 Dec 2010 01:32:36AM +0200 Julian Anastasov (ja@ssi.bg) wrote:
> 
> 	Hello,
> 
> On Sun, 12 Dec 2010, Tinggong Wang wrote:
> 
> >use time_after get the current buffer created more than the specified time.
> >
> >time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
> >if curr_sb has been created more than 2*HZ then it will still sit in master.
> >so use time_after instead.
> >
> >Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> >---
> >net/netfilter/ipvs/ip_vs_sync.c |    3 +--
> >1 files changed, 1 insertions(+), 2 deletions(-)
> >
> >diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> >index 2b6b0cb..555b0dd 100644
> >--- a/net/netfilter/ipvs/ip_vs_sync.c
> >+++ b/net/netfilter/ipvs/ip_vs_sync.c
> >@@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
> >	struct ip_vs_sync_buff *sb;
> >
> >	spin_lock_bh(&curr_sb_lock);
> >-	if (curr_sb && (time == 0 ||
> >-			time_before(jiffies - curr_sb->firstuse, time))) {
> >+	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
> 
> 	This breaks the time=0 case when jiffies matches firstuse.
> 
> 	May be the fix should be as follows?:
> 
> 	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {
> 
> 	i.e. passed >= limit (2 or 0).
> 

Thanks!

here is the improved patch:

From 68f30a4e8759dae7de4fb846db8ad264301c0bc6 Mon Sep 17 00:00:00 2001
From: Tinggong Wang <wangtinggong@gmail.com>
Date: Tue, 14 Dec 2010 10:53:24 +0800
Subject: [PATCH] ipvs: fix get_curr_sync_buff

use time_after_eq get the current buffer created more than the specified time,
or equals it.

Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
---
 net/netfilter/ipvs/ip_vs_sync.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index c1c167a..e9d2196 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -395,8 +395,7 @@ get_curr_sync_buff(unsigned long time)
 	struct ip_vs_sync_buff *sb;
 
 	spin_lock_bh(&curr_sb_lock);
-	if (curr_sb && (time == 0 ||
-			time_before(jiffies - curr_sb->firstuse, time))) {
+	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {
 		sb = curr_sb;
 		curr_sb = NULL;
 	} else
-- 
1.7.2.3

> >		sb = curr_sb;
> >		curr_sb = NULL;
> >	} else
> >-- 
> >1.7.2.3
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Julian Anastasov]

 	Hello,

On Tue, 14 Dec 2010, Tinggong Wang wrote:

>>> use time_after get the current buffer created more than the specified time.
>>>
>>> time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
>>> if curr_sb has been created more than 2*HZ then it will still sit in master.
>>> so use time_after instead.
>>>
>>> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
>>> ---
>>> net/netfilter/ipvs/ip_vs_sync.c |    3 +--
>>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
>>> index 2b6b0cb..555b0dd 100644
>>> --- a/net/netfilter/ipvs/ip_vs_sync.c
>>> +++ b/net/netfilter/ipvs/ip_vs_sync.c
>>> @@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
>>> 	struct ip_vs_sync_buff *sb;
>>>
>>> 	spin_lock_bh(&curr_sb_lock);
>>> -	if (curr_sb && (time == 0 ||
>>> -			time_before(jiffies - curr_sb->firstuse, time))) {
>>> +	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
>>
>> 	This breaks the time=0 case when jiffies matches firstuse.
>>
>> 	May be the fix should be as follows?:
>>
>> 	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {
>>
>> 	i.e. passed >= limit (2 or 0).
>>
>
> Thanks!
>
> here is the improved patch:
>
>> From 68f30a4e8759dae7de4fb846db8ad264301c0bc6 Mon Sep 17 00:00:00 2001
> From: Tinggong Wang <wangtinggong@gmail.com>
> Date: Tue, 14 Dec 2010 10:53:24 +0800
> Subject: [PATCH] ipvs: fix get_curr_sync_buff
>
> use time_after_eq get the current buffer created more than the specified time,
> or equals it.
>
> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>

 	Simon, this change looks ok, consider for
applying, may be after the changes from Hans if there is
collision.

Acked-by: Julian Anastasov <ja@ssi.bg>

> ---
> net/netfilter/ipvs/ip_vs_sync.c |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index c1c167a..e9d2196 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -395,8 +395,7 @@ get_curr_sync_buff(unsigned long time)
> 	struct ip_vs_sync_buff *sb;
>
> 	spin_lock_bh(&curr_sb_lock);
> -	if (curr_sb && (time == 0 ||
> -			time_before(jiffies - curr_sb->firstuse, time))) {
> +	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {
> 		sb = curr_sb;
> 		curr_sb = NULL;
> 	} else
> -- 
> 1.7.2.3

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH 3/3] ipvs: fix get_curr_sync_buff

[Simon Horman]

On Tue, Dec 14, 2010 at 10:28:04AM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Tue, 14 Dec 2010, Tinggong Wang wrote:
> 
> >>>use time_after get the current buffer created more than the specified time.
> >>>
> >>>time_before in get_curr_sync_buff(2 * HZ) will get the buffer created newly.
> >>>if curr_sb has been created more than 2*HZ then it will still sit in master.
> >>>so use time_after instead.
> >>>
> >>>Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> >>>---
> >>>net/netfilter/ipvs/ip_vs_sync.c |    3 +--
> >>>1 files changed, 1 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> >>>index 2b6b0cb..555b0dd 100644
> >>>--- a/net/netfilter/ipvs/ip_vs_sync.c
> >>>+++ b/net/netfilter/ipvs/ip_vs_sync.c
> >>>@@ -221,8 +221,7 @@ get_curr_sync_buff(unsigned long time)
> >>>	struct ip_vs_sync_buff *sb;
> >>>
> >>>	spin_lock_bh(&curr_sb_lock);
> >>>-	if (curr_sb && (time == 0 ||
> >>>-			time_before(jiffies - curr_sb->firstuse, time))) {
> >>>+	if (curr_sb && time_after(jiffies - curr_sb->firstuse, time)) {
> >>
> >>	This breaks the time=0 case when jiffies matches firstuse.
> >>
> >>	May be the fix should be as follows?:
> >>
> >>	if (curr_sb && time_after_eq(jiffies - curr_sb->firstuse, time)) {
> >>
> >>	i.e. passed >= limit (2 or 0).
> >>
> >
> >Thanks!
> >
> >here is the improved patch:
> >
> >>From 68f30a4e8759dae7de4fb846db8ad264301c0bc6 Mon Sep 17 00:00:00 2001
> >From: Tinggong Wang <wangtinggong@gmail.com>
> >Date: Tue, 14 Dec 2010 10:53:24 +0800
> >Subject: [PATCH] ipvs: fix get_curr_sync_buff
> >
> >use time_after_eq get the current buffer created more than the specified time,
> >or equals it.
> >
> >Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> 
> 	Simon, this change looks ok, consider for
> applying, may be after the changes from Hans if there is
> collision.

Thanks, I'll put this change in after Hans's changes as you suggest.

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Simon Horman]

[ CCed Hans Schillstrom and Julian Anastasov ]

On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 7632a17..2b6b0cb 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
>  	char *p;
>  	int i;
>  
> -	if (buflen < SYNC_MESG_HEADER_LEN) {
> -		IP_VS_ERR_RL("sync message header too short\n");
> -		return;
> -	}
> -
>  	/* Convert size back to host byte order */
>  	m->size = ntohs(m->size);
>  
> @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
>  				break;
>  			}
>  
> +			/* throw invalid data before local_bh_disable,
> +			 * so performance won't be downgraded by it
> +			 */
> +			if (len < SYNC_MESG_HEADER_LEN) {
> +				IP_VS_ERR_RL("sync message header too short\n");
> +				continue;
> +			}
> +
>  			/* disable bottom half, because it accesses the data
>  			   shared by softirq while getting/creating conns */
>  			local_bh_disable();
> -- 
> 1.7.2.3
> 

Could you explain the motivation for this change?

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Tinggong Wang]

on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@verge.net.au) wrote:
> [ CCed Hans Schillstrom and Julian Anastasov ]
> 
> On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > ---
> >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> >  1 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index 7632a17..2b6b0cb 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> >  	char *p;
> >  	int i;
> >  
> > -	if (buflen < SYNC_MESG_HEADER_LEN) {
> > -		IP_VS_ERR_RL("sync message header too short\n");
> > -		return;
> > -	}
> > -
> >  	/* Convert size back to host byte order */
> >  	m->size = ntohs(m->size);
> >  
> > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> >  				break;
> >  			}
> >  
> > +			/* throw invalid data before local_bh_disable,
> > +			 * so performance won't be downgraded by it
> > +			 */
> > +			if (len < SYNC_MESG_HEADER_LEN) {
> > +				IP_VS_ERR_RL("sync message header too short\n");
> > +				continue;
> > +			}
> > +
> >  			/* disable bottom half, because it accesses the data
> >  			   shared by softirq while getting/creating conns */
> >  			local_bh_disable();
> > -- 
> > 1.7.2.3
> > 
> 
> Could you explain the motivation for this change?

in my opinion, before local_bh_disable, should ensure packets are look
like more resonable.

local_bh_disable will disable all bottom-half processing on local cpu,
if the multicast group flood of packets containing bad sync message,
local cpu will be busy doing local_bh_disable and local_bh_enable. 

if the backup pc has only one cpu, all other tasks will be pending until
the flood finished.

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Simon Horman]

On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@verge.net.au) wrote:
> > [ CCed Hans Schillstrom and Julian Anastasov ]
> > 
> > On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > > ---
> > >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> > >  1 files changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > > index 7632a17..2b6b0cb 100644
> > > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> > >  	char *p;
> > >  	int i;
> > >  
> > > -	if (buflen < SYNC_MESG_HEADER_LEN) {
> > > -		IP_VS_ERR_RL("sync message header too short\n");
> > > -		return;
> > > -	}
> > > -
> > >  	/* Convert size back to host byte order */
> > >  	m->size = ntohs(m->size);
> > >  
> > > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> > >  				break;
> > >  			}
> > >  
> > > +			/* throw invalid data before local_bh_disable,
> > > +			 * so performance won't be downgraded by it
> > > +			 */
> > > +			if (len < SYNC_MESG_HEADER_LEN) {
> > > +				IP_VS_ERR_RL("sync message header too short\n");
> > > +				continue;
> > > +			}
> > > +
> > >  			/* disable bottom half, because it accesses the data
> > >  			   shared by softirq while getting/creating conns */
> > >  			local_bh_disable();
> > > -- 
> > > 1.7.2.3
> > > 
> > 
> > Could you explain the motivation for this change?
> 
> in my opinion, before local_bh_disable, should ensure packets are look
> like more resonable.
> 
> local_bh_disable will disable all bottom-half processing on local cpu,
> if the multicast group flood of packets containing bad sync message,
> local cpu will be busy doing local_bh_disable and local_bh_enable. 
> 
> if the backup pc has only one cpu, all other tasks will be pending until
> the flood finished.

Ok, that does sound reasonable to some extent. But realistically
this should only occur if bogus packets are being sent. And in
that case it would be possible for bogus packets to be more carefully
crafted such that we need to enter ip_vs_process_message() anyway.
So I'm not sure if there really is a gain here.

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Hans Schillstrom]

On Mon, 2010-12-13 at 07:29 +0100, Simon Horman wrote:
> On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> > on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@verge.net.au) wrote:
> > > [ CCed Hans Schillstrom and Julian Anastasov ]
> > > 
> > > On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > > > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > > > ---
> > > >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> > > >  1 files changed, 8 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > > > index 7632a17..2b6b0cb 100644
> > > > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > > > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > > > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> > > >  	char *p;
> > > >  	int i;
> > > >  
> > > > -	if (buflen < SYNC_MESG_HEADER_LEN) {
> > > > -		IP_VS_ERR_RL("sync message header too short\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	/* Convert size back to host byte order */
> > > >  	m->size = ntohs(m->size);
> > > >  
> > > > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> > > >  				break;
> > > >  			}
> > > >  
> > > > +			/* throw invalid data before local_bh_disable,
> > > > +			 * so performance won't be downgraded by it
> > > > +			 */
> > > > +			if (len < SYNC_MESG_HEADER_LEN) {
> > > > +				IP_VS_ERR_RL("sync message header too short\n");
> > > > +				continue;
> > > > +			}
> > > > +
> > > >  			/* disable bottom half, because it accesses the data
> > > >  			   shared by softirq while getting/creating conns */
> > > >  			local_bh_disable();
> > > > -- 
> > > > 1.7.2.3
> > > > 
> > > 
> > > Could you explain the motivation for this change?
> > 
> > in my opinion, before local_bh_disable, should ensure packets are look
> > like more resonable.
> > 
> > local_bh_disable will disable all bottom-half processing on local cpu,
> > if the multicast group flood of packets containing bad sync message,
> > local cpu will be busy doing local_bh_disable and local_bh_enable. 
> > 
> > if the backup pc has only one cpu, all other tasks will be pending until
> > the flood finished.
> 
> Ok, that does sound reasonable to some extent. But realistically
> this should only occur if bogus packets are being sent. And in
> that case it would be possible for bogus packets to be more carefully
> crafted such that we need to enter ip_vs_process_message() anyway.
> So I'm not sure if there really is a gain here.
> 
I do agree, first of all It's a multicast and they are never opened in
firewall so who should flood us? 
(If IPVS addr and port is open close it)
I don't think the extra rows actually adds anything as you say.

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Tinggong Wang]

on Mon, 13 Dec 2010 09:53:01AM +0100 Hans Schillstrom (hans.schillstrom@ericsson.com) wrote:
> On Mon, 2010-12-13 at 07:29 +0100, Simon Horman wrote:
> > On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> > > on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@verge.net.au) wrote:
> > > > [ CCed Hans Schillstrom and Julian Anastasov ]
> > > > 
> > > > On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > > > > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > > > > ---
> > > > >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> > > > >  1 files changed, 8 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > > > > index 7632a17..2b6b0cb 100644
> > > > > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > > > > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > > > > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> > > > >  	char *p;
> > > > >  	int i;
> > > > >  
> > > > > -	if (buflen < SYNC_MESG_HEADER_LEN) {
> > > > > -		IP_VS_ERR_RL("sync message header too short\n");
> > > > > -		return;
> > > > > -	}
> > > > > -
> > > > >  	/* Convert size back to host byte order */
> > > > >  	m->size = ntohs(m->size);
> > > > >  
> > > > > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> > > > >  				break;
> > > > >  			}
> > > > >  
> > > > > +			/* throw invalid data before local_bh_disable,
> > > > > +			 * so performance won't be downgraded by it
> > > > > +			 */
> > > > > +			if (len < SYNC_MESG_HEADER_LEN) {
> > > > > +				IP_VS_ERR_RL("sync message header too short\n");
> > > > > +				continue;
> > > > > +			}
> > > > > +
> > > > >  			/* disable bottom half, because it accesses the data
> > > > >  			   shared by softirq while getting/creating conns */
> > > > >  			local_bh_disable();
> > > > > -- 
> > > > > 1.7.2.3
> > > > > 
> > > > 
> > > > Could you explain the motivation for this change?
> > > 
> > > in my opinion, before local_bh_disable, should ensure packets are look
> > > like more resonable.
> > > 
> > > local_bh_disable will disable all bottom-half processing on local cpu,
> > > if the multicast group flood of packets containing bad sync message,
> > > local cpu will be busy doing local_bh_disable and local_bh_enable. 
> > > 
> > > if the backup pc has only one cpu, all other tasks will be pending until
> > > the flood finished.
> > 
> > Ok, that does sound reasonable to some extent. But realistically
> > this should only occur if bogus packets are being sent. And in
> > that case it would be possible for bogus packets to be more carefully
> > crafted such that we need to enter ip_vs_process_message() anyway.
> > So I'm not sure if there really is a gain here.
> > 
> I do agree, first of all It's a multicast and they are never opened in
> firewall so who should flood us? 
> (If IPVS addr and port is open close it)
> I don't think the extra rows actually adds anything as you say.
> 
Yes, it has small possibility to occur. and this patch only make sense
when the bogus packets length less than SYNC_MESG_HEADER_LEN.

but if it occurs, for example, someone write a program, join the
multicast group cursorily, and floods bogus packets accidentally.
backup's performace will be downgraded.

is this scenario should be included? if so, i'll try to improve this
patch. 

Thanks!

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

[Tinggong Wang]

on Mon, 13 Dec 2010 06:49:11PM +0800 Tinggong Wang (wangtinggong@gmail.com) wrote:
> on Mon, 13 Dec 2010 09:53:01AM +0100 Hans Schillstrom (hans.schillstrom@ericsson.com) wrote:
> > On Mon, 2010-12-13 at 07:29 +0100, Simon Horman wrote:
> > > On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> > > > on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@verge.net.au) wrote:
> > > > > [ CCed Hans Schillstrom and Julian Anastasov ]
> > > > > 
> > > > > On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > > > > > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > > > > > ---
> > > > > >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> > > > > >  1 files changed, 8 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > > > > > index 7632a17..2b6b0cb 100644
> > > > > > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > > > > > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > > > > > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> > > > > >  	char *p;
> > > > > >  	int i;
> > > > > >  
> > > > > > -	if (buflen < SYNC_MESG_HEADER_LEN) {
> > > > > > -		IP_VS_ERR_RL("sync message header too short\n");
> > > > > > -		return;
> > > > > > -	}
> > > > > > -
> > > > > >  	/* Convert size back to host byte order */
> > > > > >  	m->size = ntohs(m->size);
> > > > > >  
> > > > > > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> > > > > >  				break;
> > > > > >  			}
> > > > > >  
> > > > > > +			/* throw invalid data before local_bh_disable,
> > > > > > +			 * so performance won't be downgraded by it
> > > > > > +			 */
> > > > > > +			if (len < SYNC_MESG_HEADER_LEN) {
> > > > > > +				IP_VS_ERR_RL("sync message header too short\n");
> > > > > > +				continue;
> > > > > > +			}
> > > > > > +
> > > > > >  			/* disable bottom half, because it accesses the data
> > > > > >  			   shared by softirq while getting/creating conns */
> > > > > >  			local_bh_disable();
> > > > > > -- 
> > > > > > 1.7.2.3
> > > > > > 
> > > > > 
> > > > > Could you explain the motivation for this change?
> > > > 
> > > > in my opinion, before local_bh_disable, should ensure packets are look
> > > > like more resonable.
> > > > 
> > > > local_bh_disable will disable all bottom-half processing on local cpu,
> > > > if the multicast group flood of packets containing bad sync message,
> > > > local cpu will be busy doing local_bh_disable and local_bh_enable. 
> > > > 
> > > > if the backup pc has only one cpu, all other tasks will be pending until
> > > > the flood finished.
> > > 
> > > Ok, that does sound reasonable to some extent. But realistically
> > > this should only occur if bogus packets are being sent. And in
> > > that case it would be possible for bogus packets to be more carefully
> > > crafted such that we need to enter ip_vs_process_message() anyway.
> > > So I'm not sure if there really is a gain here.
> > > 
> > I do agree, first of all It's a multicast and they are never opened in
> > firewall so who should flood us? 
> > (If IPVS addr and port is open close it)
> > I don't think the extra rows actually adds anything as you say.
> > 
> Yes, it has small possibility to occur. and this patch only make sense
> when the bogus packets length less than SYNC_MESG_HEADER_LEN.
> 
> but if it occurs, for example, someone write a program, join the
> multicast group cursorily, and floods bogus packets accidentally.
> backup's performace will be downgraded.
> 
> is this scenario should be included? if so, i'll try to improve this
> patch. 
> 
> Thanks!

this patch disable bottom half after sanity check.
it will slightly improve backup's performance when bogus packets not
using the sync message format.

From 19c9d8bd38d3d4694ff5d0f6e16d02fcc13b7f1e Mon Sep 17 00:00:00 2001
From: Tinggong Wang <wangtinggong@gmail.com>
Date: Tue, 14 Dec 2010 01:42:18 +0800
Subject: [PATCH] ipvs: check data validation before local_bh_disable

Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
---
 net/netfilter/ipvs/ip_vs_sync.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index c1c167a..077fcdf 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1105,6 +1105,11 @@ static void ip_vs_process_message(__u8 *buffer, const size_t buflen)
 		IP_VS_DBG(7, "BACKUP, Ignoring syncid = %d\n", m2->syncid);
 		return;
 	}
+
+	/* disable bottom half, because it accesses the data
+	   shared by softirq while getting/creating conns */
+	local_bh_disable();
+
 	/* Handle version 1  message */
 	if ((m2->version == SYNC_PROTO_VER) && (m2->reserved == 0)
 	    && (m2->spare == 0)) {
@@ -1120,7 +1125,7 @@ static void ip_vs_process_message(__u8 *buffer, const size_t buflen)
 			p = msg_end;
 			if (p + sizeof(s->v4) > buffer+buflen) {
 				IP_VS_ERR_RL("BACKUP, Dropping buffer, to small\n");
-				return;
+				goto out;
 			}
 			s = (union ip_vs_sync_conn *)p;
 			size = ntohs(s->v4.ver_size) & SVER_MASK;
@@ -1128,18 +1133,18 @@ static void ip_vs_process_message(__u8 *buffer, const size_t buflen)
 			/* Basic sanity checks */
 			if (msg_end  > buffer+buflen) {
 				IP_VS_ERR_RL("BACKUP, Dropping buffer, msg > buffer\n");
-				return;
+				goto out;
 			}
 			if (ntohs(s->v4.ver_size) >> SVER_SHIFT) {
 				IP_VS_ERR_RL("BACKUP, Dropping buffer, Unknown version %d\n",
 					      ntohs(s->v4.ver_size) >> SVER_SHIFT);
-				return;
+				goto out;
 			}
 			/* Process a single sync_conn */
 			if ((retc=ip_vs_proc_sync_conn(p, msg_end)) < 0) {
 				IP_VS_ERR_RL("BACKUP, Dropping buffer, Err: %d in decoding\n",
 					     retc);
-				return;
+				goto out;
 			}
 			/* Make sure we have 32 bit alignment */
 			msg_end = p + ((size + 3) & ~3);
@@ -1147,8 +1152,10 @@ static void ip_vs_process_message(__u8 *buffer, const size_t buflen)
 	} else {
 		/* Old type of message */
 		ip_vs_process_message_v0(buffer, buflen);
-		return;
 	}
+
+out:
+	local_bh_enable();
 }
 
 
@@ -1497,11 +1504,7 @@ static int sync_thread_backup(void *data)
 				break;
 			}
 
-			/* disable bottom half, because it accesses the data
-			   shared by softirq while getting/creating conns */
-			local_bh_disable();
 			ip_vs_process_message(tinfo->buf, len);
-			local_bh_enable();
 		}
 	}
 
-- 
1.7.2.3

Re: [PATCH 1/3] ipvs: use SYNC_MESG_HEADER_LEN instead of explicit header length

[Simon Horman]

[ CCed Hans Schillstrom ]

On Sun, Dec 12, 2010 at 07:41:45PM +0800, Tinggong Wang wrote:
> Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_sync.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index ab85aed..7632a17 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -102,7 +102,6 @@ struct ip_vs_sync_thread_data {
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  */
>  
> -#define SYNC_MESG_HEADER_LEN	4
>  #define MAX_CONNS_PER_SYNCBUFF	255 /* nr_conns in ip_vs_sync_mesg is 8 bit */
>  
>  struct ip_vs_sync_mesg {
> @@ -112,6 +111,7 @@ struct ip_vs_sync_mesg {
>  
>  	/* ip_vs_sync_conn entries start here */
>  };
> +#define SYNC_MESG_HEADER_LEN	(sizeof(struct ip_vs_sync_mesg))
>  
>  /* the maximum length of sync (sending/receiving) message */
>  static int sync_send_mesg_maxlen;
> @@ -188,8 +188,8 @@ static inline struct ip_vs_sync_buff * ip_vs_sync_buff_create(void)
>  	}
>  	sb->mesg->nr_conns = 0;
>  	sb->mesg->syncid = ip_vs_master_syncid;
> -	sb->mesg->size = 4;
> -	sb->head = (unsigned char *)sb->mesg + 4;
> +	sb->mesg->size = SYNC_MESG_HEADER_LEN;
> +	sb->head = (unsigned char *)sb->mesg + SYNC_MESG_HEADER_LEN;
>  	sb->end = (unsigned char *)sb->mesg + sync_send_mesg_maxlen;
>  	sb->firstuse = jiffies;
>  	return sb;
> @@ -315,7 +315,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
>  	char *p;
>  	int i;
>  
> -	if (buflen < sizeof(struct ip_vs_sync_mesg)) {
> +	if (buflen < SYNC_MESG_HEADER_LEN) {
>  		IP_VS_ERR_RL("sync message header too short\n");
>  		return;
>  	}
> @@ -335,7 +335,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
>  		return;
>  	}
>  
> -	p = (char *)buffer + sizeof(struct ip_vs_sync_mesg);
> +	p = (char *)buffer + SYNC_MESG_HEADER_LEN;
>  	for (i=0; i<m->nr_conns; i++) {
>  		unsigned flags, state;
>  
> -- 
> 1.7.2.3
> 

This looks good to me, Hans can I get an Ack from you?

Re: [PATCH 1/3] ipvs: use SYNC_MESG_HEADER_LEN instead of explicit header length

[Hans Schillstrom]

On Sun, 2010-12-12 at 22:46 +0100, Simon Horman wrote:
> [ CCed Hans Schillstrom ]
> 
> On Sun, Dec 12, 2010 at 07:41:45PM +0800, Tinggong Wang wrote:
> > Signed-off-by: Tinggong Wang <wangtinggong@gmail.com>
> > ---
> >  net/netfilter/ipvs/ip_vs_sync.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index ab85aed..7632a17 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -102,7 +102,6 @@ struct ip_vs_sync_thread_data {
> >        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >  */
> >  
> > -#define SYNC_MESG_HEADER_LEN	4
> >  #define MAX_CONNS_PER_SYNCBUFF	255 /* nr_conns in ip_vs_sync_mesg is 8 bit */
> >  
> >  struct ip_vs_sync_mesg {
> > @@ -112,6 +111,7 @@ struct ip_vs_sync_mesg {
> >  
> >  	/* ip_vs_sync_conn entries start here */
> >  };
> > +#define SYNC_MESG_HEADER_LEN	(sizeof(struct ip_vs_sync_mesg))
> >  
> >  /* the maximum length of sync (sending/receiving) message */
> >  static int sync_send_mesg_maxlen;
> > @@ -188,8 +188,8 @@ static inline struct ip_vs_sync_buff * ip_vs_sync_buff_create(void)
> >  	}
> >  	sb->mesg->nr_conns = 0;
> >  	sb->mesg->syncid = ip_vs_master_syncid;
> > -	sb->mesg->size = 4;
> > -	sb->head = (unsigned char *)sb->mesg + 4;
> > +	sb->mesg->size = SYNC_MESG_HEADER_LEN;
> > +	sb->head = (unsigned char *)sb->mesg + SYNC_MESG_HEADER_LEN;
> >  	sb->end = (unsigned char *)sb->mesg + sync_send_mesg_maxlen;
> >  	sb->firstuse = jiffies;
> >  	return sb;
> > @@ -315,7 +315,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> >  	char *p;
> >  	int i;
> >  
> > -	if (buflen < sizeof(struct ip_vs_sync_mesg)) {
> > +	if (buflen < SYNC_MESG_HEADER_LEN) {
> >  		IP_VS_ERR_RL("sync message header too short\n");
> >  		return;
> >  	}
> > @@ -335,7 +335,7 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> >  		return;
> >  	}
> >  
> > -	p = (char *)buffer + sizeof(struct ip_vs_sync_mesg);
> > +	p = (char *)buffer + SYNC_MESG_HEADER_LEN;
> >  	for (i=0; i<m->nr_conns; i++) {
> >  		unsigned flags, state;
> >  
> > -- 
> > 1.7.2.3
> > 
> 
> This looks good to me, Hans can I get an Ack from you?

This patch is outdated, it doesn't look like this after IPv6 & PE
additions. 

(BTW I will post the network name space patch today, and the 4 is
already replaced there.)

Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>