Patchwork [net-next] nat: remove obsolete rcu_read_unlock call

login
register
mail settings
Submitter Ulrich Weber
Date Sept. 20, 2012, 1:52 p.m.
Message ID <20120920135133.GA21538@uweber-WS>
Download mbox | patch
Permalink /patch/185396/
State Accepted
Headers show

Comments

Ulrich Weber - Sept. 20, 2012, 1:52 p.m.
hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
so rcu_read_unlock() is unnecessary if in_range() matches.

Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
---
 net/netfilter/nf_nat_core.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Eric Dumazet - Sept. 20, 2012, 2:14 p.m.
On Thu, 2012-09-20 at 15:52 +0200, Ulrich Weber wrote:
> hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
> so rcu_read_unlock() is unnecessary if in_range() matches.
> 
> Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
> ---
>  net/netfilter/nf_nat_core.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index 1816ad3..125d832 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -201,10 +201,8 @@ find_appropriate_src(struct net *net, u16 zone,
>  				       &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
>  			result->dst = tuple->dst;
>  
> -			if (in_range(l3proto, l4proto, result, range)) {
> -				rcu_read_unlock();
> +			if (in_range(l3proto, l4proto, result, range))
>  				return 1;
> -			}
>  		}
>  	}
>  	return 0;


This could explain the rcu_read_unlock() imbalance Stephen had (lockdep
splat with net-next)


How old is this bug ?



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulrich Weber - Sept. 20, 2012, 2:17 p.m.
On 09/20/2012 04:14 PM, Eric Dumazet wrote:
> This could explain the rcu_read_unlock() imbalance Stephen had 
> (lockdep splat with net-next) How old is this bug ? 
Bug was introduced with c7232c9979cba684c50b64c513c4a83c9aa70563

Cheers
  Ulrich
stephen hemminger - Sept. 20, 2012, 4:27 p.m.
On Thu, 20 Sep 2012 16:14:45 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Thu, 2012-09-20 at 15:52 +0200, Ulrich Weber wrote:
> > hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
> > so rcu_read_unlock() is unnecessary if in_range() matches.
> > 
> > Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
> > ---
> >  net/netfilter/nf_nat_core.c |    4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index 1816ad3..125d832 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -201,10 +201,8 @@ find_appropriate_src(struct net *net, u16 zone,
> >  				       &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> >  			result->dst = tuple->dst;
> >  
> > -			if (in_range(l3proto, l4proto, result, range)) {
> > -				rcu_read_unlock();
> > +			if (in_range(l3proto, l4proto, result, range))
> >  				return 1;
> > -			}
> >  		}
> >  	}
> >  	return 0;
> 
> 
> This could explain the rcu_read_unlock() imbalance Stephen had (lockdep
> splat with net-next)
> 
> 
> How old is this bug ?
> 
> 
> 

Will validate the fix tomorrow when back home.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Sept. 21, 2012, 10:14 a.m.
On Thu, Sep 20, 2012 at 03:52:04PM +0200, Ulrich Weber wrote:
> hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
> so rcu_read_unlock() is unnecessary if in_range() matches.

Applied, thanks Ulrich.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Sept. 21, 2012, 10:14 a.m.
On Thu, Sep 20, 2012 at 04:17:25PM +0200, Ulrich Weber wrote:
> On 09/20/2012 04:14 PM, Eric Dumazet wrote:
> >This could explain the rcu_read_unlock() imbalance Stephen had
> >(lockdep splat with net-next) How old is this bug ?
>
> Bug was introduced with c7232c9979cba684c50b64c513c4a83c9aa70563

I've added this to the patch description, it's good for the record.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Sept. 21, 2012, 5:08 p.m.
On Thu, 20 Sep 2012 16:14:45 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Thu, 2012-09-20 at 15:52 +0200, Ulrich Weber wrote:
> > hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
> > so rcu_read_unlock() is unnecessary if in_range() matches.
> > 
> > Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
> > ---
> >  net/netfilter/nf_nat_core.c |    4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index 1816ad3..125d832 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -201,10 +201,8 @@ find_appropriate_src(struct net *net, u16 zone,
> >  				       &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> >  			result->dst = tuple->dst;
> >  
> > -			if (in_range(l3proto, l4proto, result, range)) {
> > -				rcu_read_unlock();
> > +			if (in_range(l3proto, l4proto, result, range))
> >  				return 1;
> > -			}
> >  		}
> >  	}
> >  	return 0;
> 
> 
> This could explain the rcu_read_unlock() imbalance Stephen had (lockdep
> splat with net-next)
> 
> 
> How old is this bug ?
> 
> 
> 

This fixes the lockdep splat I saw when staring KVM vm's with net-next and lockdep enabled
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - Sept. 21, 2012, 7:47 p.m.
On Fri, Sep 21, 2012 at 10:08:42AM -0700, Stephen Hemminger wrote:
> On Thu, 20 Sep 2012 16:14:45 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > On Thu, 2012-09-20 at 15:52 +0200, Ulrich Weber wrote:
> > > hlist walk in find_appropriate_src() is not protected anymore by rcu_read_lock(),
> > > so rcu_read_unlock() is unnecessary if in_range() matches.
> > > 
> > > Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
> > > ---
> > >  net/netfilter/nf_nat_core.c |    4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > > index 1816ad3..125d832 100644
> > > --- a/net/netfilter/nf_nat_core.c
> > > +++ b/net/netfilter/nf_nat_core.c
> > > @@ -201,10 +201,8 @@ find_appropriate_src(struct net *net, u16 zone,
> > >  				       &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> > >  			result->dst = tuple->dst;
> > >  
> > > -			if (in_range(l3proto, l4proto, result, range)) {
> > > -				rcu_read_unlock();
> > > +			if (in_range(l3proto, l4proto, result, range))
> > >  				return 1;
> > > -			}
> > >  		}
> > >  	}
> > >  	return 0;
> > 
> > 
> > This could explain the rcu_read_unlock() imbalance Stephen had (lockdep
> > splat with net-next)
> > 
> > 
> > How old is this bug ?
> > 
> > 
> > 
> 
> This fixes the lockdep splat I saw when staring KVM vm's with net-next and lockdep enabled

It's in my tree already.

I'll pass it to David asap. Thanks for the report.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 1816ad3..125d832 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -201,10 +201,8 @@  find_appropriate_src(struct net *net, u16 zone,
 				       &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
 			result->dst = tuple->dst;
 
-			if (in_range(l3proto, l4proto, result, range)) {
-				rcu_read_unlock();
+			if (in_range(l3proto, l4proto, result, range))
 				return 1;
-			}
 		}
 	}
 	return 0;