netdev - RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 20 Sep 2011 07:14:15 -0700
From:	"Myklebust, Trond" <Trond.Myklebust@...app.com>
To:	"Stanislav Kinsbursky" <skinsbursky@...allels.com>
Cc:	"Schumaker, Bryan" <Bryan.Schumaker@...app.com>,
	<linux-nfs@...r.kernel.org>,
	"Pavel Emelianov" <xemul@...allels.com>, <neilb@...e.de>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<bfields@...ldses.org>, <davem@...emloft.net>
Subject: RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@...allels.com]
> Sent: Tuesday, September 20, 2011 9:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@...r.kernel.org; Pavel Emelianov;
> neilb@...e.de; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> bfields@...ldses.org; davem@...emloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> 20.09.2011 17:15, Myklebust, Trond пишет:
> >> -----Original Message-----
> >> From: Schumaker, Bryan
> >> Sent: Tuesday, September 20, 2011 9:05 AM
> >> To: Stanislav Kinsbursky
> >> Cc: Myklebust, Trond; linux-nfs@...r.kernel.org; xemul@...allels.com;
> >> neilb@...e.de; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> >> bfields@...ldses.org; davem@...emloft.net
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> >>> This helpers will be used for dynamical creation and destruction of
> >>> rpcbind clients.
> >>> Variable rpcb_users is actually a counter of lauched RPC services.
> >>> If rpcbind clients has been created already, then we just increase
> rpcb_users.
> >>>
> >>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@...allels.com>
> >>>
> >>> ---
> >>>   net/sunrpc/rpcb_clnt.c |   50
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 files changed, 50 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> >>> e45d2fb..8724780 100644
> >>> --- a/net/sunrpc/rpcb_clnt.c
> >>> +++ b/net/sunrpc/rpcb_clnt.c
> >>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >>>   static struct rpc_clnt *	rpcb_local_clnt;
> >>>   static struct rpc_clnt *	rpcb_local_clnt4;
> >>>
> >>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >>> +unsigned int			rpcb_users;
> >>> +
> >>>   struct rpcbind_args {
> >>>   	struct rpc_xprt *	r_xprt;
> >>>
> >>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
> >>>   	kfree(map);
> >>>   }
> >>>
> >>> +static int rpcb_get_local(void)
> >>> +{
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (rpcb_users)
> >>> +		rpcb_users++;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	return rpcb_users;
> >>          ^^^^^^^^^^^^^^^^^^
> >> Is it safe to use this variable outside of the rpcb_clnt_lock?
> >>
> > Nope. If rpcb_users was zero in the protected section above, nothing
> guarantees that it will still be zero here, and so the caller may get the (wrong)
> impression that the counter was incremented.
> >
> 
> Yep, you right. Will fix this races.
> 
> >>> +}
> >>> +
> >>> +void rpcb_put_local(void)
> >>> +{
> >>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> >>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >>> +	int shutdown;
> >>> +
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (--rpcb_users == 0) {
> >>> +		rpcb_local_clnt = NULL;
> >>> +		rpcb_local_clnt4 = NULL;
> >>> +	}
> >>> +	shutdown = !rpcb_users;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	if (shutdown) {
> >>> +		/*
> >>> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> >>> +		 */
> >>> +		if (clnt4)
> >>> +			rpc_shutdown_client(clnt4);
> >>> +		if (clnt)
> >>> +			rpc_shutdown_client(clnt);
> >>> +	}
> >>> +	return;
> >>> +}
> >>> +
> >>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
> >>> +*clnt4) {
> >>> +	/* Protected by rpcb_create_local_mutex */
> >
> > Doesn't it  need to be protected by rpcb_clnt_lock too?
> >
> 
> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
> will change rpcb_users since it's zero. If it's non zero - we willn't get to
> rpcb_set_local().

OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?

In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?

> >>> +	rpcb_local_clnt = clnt;
> >>> +	rpcb_local_clnt4 = clnt4;
> >>> +	rpcb_users++;
> > ^^^^^^^^^^^^^^^^^^^
> >
> >>> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> >>> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> >>> +			rpcb_local_clnt4);
> >>> +}
> >>> +