ip_vs_lblcr logic error causing table flushing

ip_vs_lblcr logic error causing table flushing

[Simon Kirby]

[ Resent with a reasonable subject and to lvs-devel :) ]

Hello!

I was noticing a significant amount of what seems/seemed to be
destination lists with multiple entries with the lblcr LVS algorithm. 
While tracking it down, I think I stumbled over a mistake.  In
ip_vs_lblcr_full_check(), it appears the time check logic is reversed:

        for (i=0, j=tbl->rover; i<IP_VS_LBLCR_TAB_SIZE; i++) {
                j = (j + 1) & IP_VS_LBLCR_TAB_MASK;

                write_lock(&svc->sched_lock);
                list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
                        if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
                                       now))
                                continue;
                        
                        ip_vs_lblcr_free(en);
                        atomic_dec(&tbl->entries);
                }
                write_unlock(&svc->sched_lock);
        }

Shouldn't this be "time_before"?  It seems that it currently nukes all
recently-used entries every time this function is called, which seems to
be every 30 minutes, rather than removing the not-recently-used ones.

If my reading is correct, this patch should fix it.  Am I missing
something?

Cheers,

Simon-

diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
index 715b57f..937743f 100644
--- a/net/netfilter/ipvs/ip_vs_lblcr.c
+++ b/net/netfilter/ipvs/ip_vs_lblcr.c
@@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc)
 
 		write_lock(&svc->sched_lock);
 		list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
-			if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
+			if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration,
 				       now))
 				continue;
 

Re: ip_vs_lblcr logic error causing table flushing

[Simon Horman]

On Wed, Nov 04, 2009 at 04:38:01PM -0800, Simon Kirby wrote:
> [ Resent with a reasonable subject and to lvs-devel :) ]
> 
> Hello!
> 
> I was noticing a significant amount of what seems/seemed to be
> destination lists with multiple entries with the lblcr LVS algorithm. 
> While tracking it down, I think I stumbled over a mistake.  In
> ip_vs_lblcr_full_check(), it appears the time check logic is reversed:
> 
>         for (i=0, j=tbl->rover; i<IP_VS_LBLCR_TAB_SIZE; i++) {
>                 j = (j + 1) & IP_VS_LBLCR_TAB_MASK;
> 
>                 write_lock(&svc->sched_lock);
>                 list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
>                         if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
>                                        now))
>                                 continue;
>                         
>                         ip_vs_lblcr_free(en);
>                         atomic_dec(&tbl->entries);
>                 }
>                 write_unlock(&svc->sched_lock);
>         }
> 
> Shouldn't this be "time_before"?  It seems that it currently nukes all
> recently-used entries every time this function is called, which seems to
> be every 30 minutes, rather than removing the not-recently-used ones.
> 
> If my reading is correct, this patch should fix it.  Am I missing
> something?
> 
> Cheers,
> 
> Simon-
> 
> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
> index 715b57f..937743f 100644
> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c
> @@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc)
>  
>  		write_lock(&svc->sched_lock);
>  		list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
> -			if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
> +			if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration,
>  				       now))
>  				continue;
>  

Hi Simon,

your analysis seems correct to me. Could you supply a Signed-off-line
with the patch and I'll see about getting it merged.

[PATCH] Fix ip_vs_lblcr_full_check to remove unused entries instead of used entries.

[Simon Kirby]

Tested with real packets -- seems to work.

Signed-off-by: Simon Kirby <sim@hostway.ca>

---
 net/netfilter/ipvs/ip_vs_lblcr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
index 715b57f..937743f 100644
--- a/net/netfilter/ipvs/ip_vs_lblcr.c
+++ b/net/netfilter/ipvs/ip_vs_lblcr.c
@@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc)
 
 		write_lock(&svc->sched_lock);
 		list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
-			if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
+			if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration,
 				       now))
 				continue;
 
-- 
1.6.5.2

Re: [PATCH] Fix ip_vs_lblcr_full_check to remove unused entries instead of used entries.

[Simon Kirby]

D'oh.  Don't apply this -- the argument order makes the original correct.

Simon-

On Thu, Nov 05, 2009 at 12:37:16AM -0800, Simon Kirby wrote:

> Tested with real packets -- seems to work.
> 
> Signed-off-by: Simon Kirby <sim@hostway.ca>
> 
> ---
>  net/netfilter/ipvs/ip_vs_lblcr.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
> index 715b57f..937743f 100644
> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c
> @@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc)
>  
>  		write_lock(&svc->sched_lock);
>  		list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
> -			if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
> +			if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration,
>  				       now))
>  				continue;
>  
> -- 
> 1.6.5.2