[1/2] NFSD: notifiers registration cleanup
diff mbox

Message ID 6bce434f-4798-42bf-8d4f-e1f33ceb8ad6@virtuozzo.com
State New
Headers show

Commit Message

Vasily Averin Sept. 21, 2016, 12:33 p.m. UTC
By design notifier can be registered once only,
however nfsd registers the same inetaddr notifiers per net-namespace.
When this happen it corrupts list of notifiers,
as result some notifiers can be not called on proper event,
traverse on list can be cycled forever,
and second unregister can access already freed memory.

fixes: 36684996 ("nfsd: Register callbacks on the inetaddr_chain and inet6addr_chain")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/nfsd/nfssvc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Jeff Layton Sept. 21, 2016, 1:20 p.m. UTC | #1
On Wed, 2016-09-21 at 15:33 +0300, Vasily Averin wrote:
> By design notifier can be registered once only,
> however nfsd registers the same inetaddr notifiers per net-namespace.
> When this happen it corrupts list of notifiers,
> as result some notifiers can be not called on proper event,
> traverse on list can be cycled forever,
> and second unregister can access already freed memory.
> 
> fixes: 36684996 ("nfsd: Register callbacks on the inetaddr_chain and inet6addr_chain")
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/nfsd/nfssvc.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 45007ac..7f8914f 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -366,14 +366,20 @@ static struct notifier_block nfsd_inet6addr_notifier = {
>  };
>  #endif
>  
> +static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
> +
>  static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
>  {
> >  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  
> > -	unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
> > +	/* check if the notifier still has clients */
> > +	if (atomic_dec_return(&nfsd_notifier_refcount) == 0) {
> > +		unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
>  #if IS_ENABLED(CONFIG_IPV6)
> > -	unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
> > +		unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
>  #endif
> > +	}
> +
> >  	/*
> >  	 * write_ports can create the server without actually starting
> >  	 * any threads--if we get shut down before any threads are
> @@ -488,10 +494,13 @@ int nfsd_create_serv(struct net *net)
> >  	}
>  
> >  	set_max_drc();
> > -	register_inetaddr_notifier(&nfsd_inetaddr_notifier);
> > +	/* check if the notifier is already set */
> > +	if (atomic_inc_return(&nfsd_notifier_refcount) == 1) {
> > +		register_inetaddr_notifier(&nfsd_inetaddr_notifier);
>  #if IS_ENABLED(CONFIG_IPV6)
> > -	register_inet6addr_notifier(&nfsd_inet6addr_notifier);
> > +		register_inet6addr_notifier(&nfsd_inet6addr_notifier);
>  #endif
> > +	}
> > >  	do_gettimeofday(&nn->nfssvc_boot);		/* record boot time */
> >  	return 0;
>  }

Good catch. I'm not very fond of the refcounting this here but it
should
serve the purpose and I don't have anything better to suggest. FWIW, I
think the nfsd_mutex is held during all of these operations so we
probably don't need atomics for the refcount.

--
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vasily Averin Sept. 22, 2016, 8:35 a.m. UTC | #2
On 21.09.2016 16:20, Jeff Layton wrote:
> Good catch. I'm not very fond of the refcounting this here but it
> should
> serve the purpose and I don't have anything better to suggest. FWIW, I
> think the nfsd_mutex is held during all of these operations so we
> probably don't need atomics for the refcount.

I've copied idea from netfilters,
look at using of masquerade_notifier_refcount

Perhaps someday in future we'll replace all such cases by some common primitive.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 22, 2016, 4:15 p.m. UTC | #3
On Wed, Sep 21, 2016 at 09:20:10AM -0400, Jeff Layton wrote:
> On Wed, 2016-09-21 at 15:33 +0300, Vasily Averin wrote:
> > By design notifier can be registered once only,
> > however nfsd registers the same inetaddr notifiers per net-namespace.
> > When this happen it corrupts list of notifiers,
> > as result some notifiers can be not called on proper event,
> > traverse on list can be cycled forever,
> > and second unregister can access already freed memory.
> > 
> > fixes: 36684996 ("nfsd: Register callbacks on the inetaddr_chain and inet6addr_chain")
> > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > ---
> >  fs/nfsd/nfssvc.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 45007ac..7f8914f 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -366,14 +366,20 @@ static struct notifier_block nfsd_inet6addr_notifier = {
> >  };
> >  #endif
> >  
> > +static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
> > +
> >  static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
> >  {
> > >  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> >  
> > > -	unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
> > > +	/* check if the notifier still has clients */
> > > +	if (atomic_dec_return(&nfsd_notifier_refcount) == 0) {
> > > +		unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
> >  #if IS_ENABLED(CONFIG_IPV6)
> > > -	unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
> > > +		unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
> >  #endif
> > > +	}
> > +
> > >  	/*
> > >  	 * write_ports can create the server without actually starting
> > >  	 * any threads--if we get shut down before any threads are
> > @@ -488,10 +494,13 @@ int nfsd_create_serv(struct net *net)
> > >  	}
> >  
> > >  	set_max_drc();
> > > -	register_inetaddr_notifier(&nfsd_inetaddr_notifier);
> > > +	/* check if the notifier is already set */
> > > +	if (atomic_inc_return(&nfsd_notifier_refcount) == 1) {
> > > +		register_inetaddr_notifier(&nfsd_inetaddr_notifier);
> >  #if IS_ENABLED(CONFIG_IPV6)
> > > -	register_inet6addr_notifier(&nfsd_inet6addr_notifier);
> > > +		register_inet6addr_notifier(&nfsd_inet6addr_notifier);
> >  #endif
> > > +	}
> > > >  	do_gettimeofday(&nn->nfssvc_boot);		/* record boot time */
> > >  	return 0;
> >  }
> 
> Good catch. I'm not very fond of the refcounting this here but it
> should
> serve the purpose and I don't have anything better to suggest. FWIW, I
> think the nfsd_mutex is held during all of these operations so we
> probably don't need atomics for the refcount.

Inclined to apply with a note like:

	/* Only used under nfsd_mutex, so this atomic may be overkill: */
	static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
	...

--b.
> 
> --
> Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 45007ac..7f8914f 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -366,14 +366,20 @@  static struct notifier_block nfsd_inet6addr_notifier = {
 };
 #endif
 
+static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
+
 static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
-	unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
+	/* check if the notifier still has clients */
+	if (atomic_dec_return(&nfsd_notifier_refcount) == 0) {
+		unregister_inetaddr_notifier(&nfsd_inetaddr_notifier);
 #if IS_ENABLED(CONFIG_IPV6)
-	unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
+		unregister_inet6addr_notifier(&nfsd_inet6addr_notifier);
 #endif
+	}
+
 	/*
 	 * write_ports can create the server without actually starting
 	 * any threads--if we get shut down before any threads are
@@ -488,10 +494,13 @@  int nfsd_create_serv(struct net *net)
 	}
 
 	set_max_drc();
-	register_inetaddr_notifier(&nfsd_inetaddr_notifier);
+	/* check if the notifier is already set */
+	if (atomic_inc_return(&nfsd_notifier_refcount) == 1) {
+		register_inetaddr_notifier(&nfsd_inetaddr_notifier);
 #if IS_ENABLED(CONFIG_IPV6)
-	register_inet6addr_notifier(&nfsd_inet6addr_notifier);
+		register_inet6addr_notifier(&nfsd_inet6addr_notifier);
 #endif
+	}
 	do_gettimeofday(&nn->nfssvc_boot);		/* record boot time */
 	return 0;
 }