[patch 0/3] ipvs: sparse fixes

[patch 0/3] ipvs: sparse fixes

[Simon Horman]

Some fixes and annotations for sparse warnings.

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

[patch 1/3] ipvs: only unlock in ip_vs_edit_service() if already locked

[Simon Horman]

[-- Attachment #1: ip_vs_edit_service-out_unlock.patch --]
[-- Type: text/plain, Size: 1259 bytes --]

Jumping to out unlocks __ip_vs_svc_lock, but that lock is not taken until
after code that may jump to out.

This problem was detected by sparse.

make C=1
  CHECK   net/ipv4/ipvs/ip_vs_ctl.c
net/ipv4/ipvs/ip_vs_ctl.c:1332:2: warning: context imbalance in 'ip_vs_edit_service' - unexpected unlock

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: lvs-2.6/net/ipv4/ipvs/ip_vs_ctl.c
===================================================================
--- lvs-2.6.orig/net/ipv4/ipvs/ip_vs_ctl.c	2008-09-16 15:16:01.000000000 +1000
+++ lvs-2.6/net/ipv4/ipvs/ip_vs_ctl.c	2008-09-16 15:16:06.000000000 +1000
@@ -1305,7 +1305,7 @@ ip_vs_edit_service(struct ip_vs_service 
 		 */
 		if ((ret = ip_vs_unbind_scheduler(svc))) {
 			old_sched = sched;
-			goto out;
+			goto out_unlock;
 		}
 
 		/*
@@ -1324,12 +1324,13 @@ ip_vs_edit_service(struct ip_vs_service 
 			 */
 			ip_vs_bind_scheduler(svc, old_sched);
 			old_sched = sched;
-			goto out;
+			goto out_unlock;
 		}
 	}
 
-  out:
+  out_unlock:
 	write_unlock_bh(&__ip_vs_svc_lock);
+  out:
 
 	if (old_sched)
 		ip_vs_scheduler_put(old_sched);

-- 

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

[patch 2/3] ipvs: supply a valid 0 address to ip_vs_conn_new()

[Simon Horman]

[-- Attachment #1: null-daddr.patch --]
[-- Type: text/plain, Size: 1261 bytes --]

ip_vs_conn_new expects a union nf_inet_addr as the type for its address
parameters, not a plain integer.

This problem was detected by sparse.

make C=1
  CHECK   net/ipv4/ipvs/ip_vs_core.c
net/ipv4/ipvs/ip_vs_core.c:469:9: warning: Using plain integer as NULL pointer

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: lvs-2.6/net/ipv4/ipvs/ip_vs_core.c
===================================================================
--- lvs-2.6.orig/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:40.000000000 +1000
+++ lvs-2.6/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:47.000000000 +1000
@@ -457,6 +457,7 @@ int ip_vs_leave(struct ip_vs_service *sv
 	if (sysctl_ip_vs_cache_bypass && svc->fwmark && unicast) {
 		int ret, cs;
 		struct ip_vs_conn *cp;
+		union nf_inet_addr daddr =  { .all = { 0, 0, 0, 0 } };
 
 		ip_vs_service_put(svc);
 
@@ -465,7 +466,7 @@ int ip_vs_leave(struct ip_vs_service *sv
 		cp = ip_vs_conn_new(svc->af, iph.protocol,
 				    &iph.saddr, pptr[0],
 				    &iph.daddr, pptr[1],
-				    0, 0,
+				    &daddr, 0,
 				    IP_VS_CONN_F_BYPASS,
 				    NULL);
 		if (cp == NULL)

-- 

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

[patch 3/3] ipvs: add __aquire/__release annotations to ip_vs_info_seq_start/ip_vs_info_seq_stop

[Simon Horman]

[-- Attachment #1: ip_vs_info_seq_start-__acquires-annotation.patch --]
[-- Type: text/plain, Size: 1202 bytes --]

This teaches sparse that the following are not problems:

make C=1
  CHECK   net/ipv4/ipvs/ip_vs_ctl.c
net/ipv4/ipvs/ip_vs_ctl.c:1793:14: warning: context imbalance in 'ip_vs_info_seq_start' - wrong count at exit
net/ipv4/ipvs/ip_vs_ctl.c:1842:13: warning: context imbalance in 'ip_vs_info_seq_stop' - unexpected unlock

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: lvs-2.6/net/ipv4/ipvs/ip_vs_ctl.c
===================================================================
--- lvs-2.6.orig/net/ipv4/ipvs/ip_vs_ctl.c	2008-09-16 15:32:45.000000000 +1000
+++ lvs-2.6/net/ipv4/ipvs/ip_vs_ctl.c	2008-09-16 15:36:48.000000000 +1000
@@ -1787,6 +1787,7 @@ static struct ip_vs_service *ip_vs_info_
 }
 
 static void *ip_vs_info_seq_start(struct seq_file *seq, loff_t *pos)
+__acquires(__ip_vs_svc_lock)
 {
 
 	read_lock_bh(&__ip_vs_svc_lock);
@@ -1840,6 +1841,7 @@ static void *ip_vs_info_seq_next(struct 
 }
 
 static void ip_vs_info_seq_stop(struct seq_file *seq, void *v)
+__releases(__ip_vs_svc_lock)
 {
 	read_unlock_bh(&__ip_vs_svc_lock);
 }

-- 

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

Re: [patch 2/3] ipvs: supply a valid 0 address to ip_vs_conn_new()

[Sven Wegener]

On Tue, 16 Sep 2008, Simon Horman wrote:

> ip_vs_conn_new expects a union nf_inet_addr as the type for its address
> parameters, not a plain integer.
> 
> This problem was detected by sparse.
> 
> make C=1
>   CHECK   net/ipv4/ipvs/ip_vs_core.c
> net/ipv4/ipvs/ip_vs_core.c:469:9: warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> Index: lvs-2.6/net/ipv4/ipvs/ip_vs_core.c
> ===================================================================
> --- lvs-2.6.orig/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:40.000000000 +1000
> +++ lvs-2.6/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:47.000000000 +1000
> @@ -457,6 +457,7 @@ int ip_vs_leave(struct ip_vs_service *sv
>  	if (sysctl_ip_vs_cache_bypass && svc->fwmark && unicast) {
>  		int ret, cs;
>  		struct ip_vs_conn *cp;
> +		union nf_inet_addr daddr =  { .all = { 0, 0, 0, 0 } };

Could be static and const, to avoid allocation and initialization on the 
stack every time. But I don't think this a critical code path.

>  
>  		ip_vs_service_put(svc);
>  
> @@ -465,7 +466,7 @@ int ip_vs_leave(struct ip_vs_service *sv
>  		cp = ip_vs_conn_new(svc->af, iph.protocol,
>  				    &iph.saddr, pptr[0],
>  				    &iph.daddr, pptr[1],
> -				    0, 0,
> +				    &daddr, 0,
>  				    IP_VS_CONN_F_BYPASS,
>  				    NULL);
>  		if (cp == NULL)

Re: [patch 0/3] ipvs: sparse fixes

[Sven Wegener]

On Tue, 16 Sep 2008, Simon Horman wrote:

> Some fixes and annotations for sparse warnings.

All look good to me.

Re: [patch 2/3] ipvs: supply a valid 0 address to ip_vs_conn_new()

[Simon Horman]

On Tue, Sep 16, 2008 at 11:25:21AM +0200, Sven Wegener wrote:
> On Tue, 16 Sep 2008, Simon Horman wrote:
> 
> > ip_vs_conn_new expects a union nf_inet_addr as the type for its address
> > parameters, not a plain integer.
> > 
> > This problem was detected by sparse.
> > 
> > make C=1
> >   CHECK   net/ipv4/ipvs/ip_vs_core.c
> > net/ipv4/ipvs/ip_vs_core.c:469:9: warning: Using plain integer as NULL pointer
> > 
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > Index: lvs-2.6/net/ipv4/ipvs/ip_vs_core.c
> > ===================================================================
> > --- lvs-2.6.orig/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:40.000000000 +1000
> > +++ lvs-2.6/net/ipv4/ipvs/ip_vs_core.c	2008-09-16 15:28:47.000000000 +1000
> > @@ -457,6 +457,7 @@ int ip_vs_leave(struct ip_vs_service *sv
> >  	if (sysctl_ip_vs_cache_bypass && svc->fwmark && unicast) {
> >  		int ret, cs;
> >  		struct ip_vs_conn *cp;
> > +		union nf_inet_addr daddr =  { .all = { 0, 0, 0, 0 } };
> 
> Could be static and const, to avoid allocation and initialization on the 
> stack every time. But I don't think this a critical code path.

Agreed, but I think I would rather leave it as is for now.

> >  
> >  		ip_vs_service_put(svc);
> >  
> > @@ -465,7 +466,7 @@ int ip_vs_leave(struct ip_vs_service *sv
> >  		cp = ip_vs_conn_new(svc->af, iph.protocol,
> >  				    &iph.saddr, pptr[0],
> >  				    &iph.daddr, pptr[1],
> > -				    0, 0,
> > +				    &daddr, 0,
> >  				    IP_VS_CONN_F_BYPASS,
> >  				    NULL);
> >  		if (cp == NULL)

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

Re: [patch 0/3] ipvs: sparse fixes

[Julius Volz]

On Tue, Sep 16, 2008 at 11:28 AM, Sven Wegener wrote:
> On Tue, 16 Sep 2008, Simon Horman wrote:
>
>> Some fixes and annotations for sparse warnings.
>
> All look good to me.

To me too. Sorry for the v6-related screwups and thanks for fixing!

Julius

-- 
Julius Volz - Corporate Operations - SysOps

Google Switzerland GmbH - Identification No.: CH-020.4.028.116-1

Re: [patch 0/3] ipvs: sparse fixes

[Simon Horman]

On Tue, Sep 16, 2008 at 01:01:09PM +0200, Julius Volz wrote:
> On Tue, Sep 16, 2008 at 11:28 AM, Sven Wegener wrote:
> > On Tue, 16 Sep 2008, Simon Horman wrote:
> >
> >> Some fixes and annotations for sparse warnings.
> >
> > All look good to me.
> 
> To me too. Sorry for the v6-related screwups and thanks for fixing!

Not at all. Thanks for all your help :-)

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

[PATCH] ipvs: Fix unused label warning

[Sven Wegener]

Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
---
 net/ipv4/ipvs/ip_vs_ctl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Simon, small fixup patch for your patch. It wasn't obvious from the patch, 
that now only the IPV6 support needs this label.

diff --git a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
index 771551d..0302cf3 100644
--- a/net/ipv4/ipvs/ip_vs_ctl.c
+++ b/net/ipv4/ipvs/ip_vs_ctl.c
@@ -1330,7 +1330,9 @@ ip_vs_edit_service(struct ip_vs_service *svc, struct ip_vs_service_user_kern *u)
 
   out_unlock:
 	write_unlock_bh(&__ip_vs_svc_lock);
+#ifdef CONFIG_IP_VS_IPV6
   out:
+#endif
 
 	if (old_sched)
 		ip_vs_scheduler_put(old_sched);

Re: [PATCH] ipvs: Fix unused label warning

[Simon Horman]

On Sat, Sep 20, 2008 at 11:48:33AM +0200, Sven Wegener wrote:
> Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
> ---
>  net/ipv4/ipvs/ip_vs_ctl.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> Simon, small fixup patch for your patch. It wasn't obvious from the patch, 
> that now only the IPV6 support needs this label.

Thanks, sorry for messing that up.

Applied

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en