[PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[Stanislav Kinsbursky]

v2: atomic_inc_return() was replaced by atomic_inc_not_zero().

These clients can't be safely dereferenced if their counter in 0.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/clnt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6797246..d10ebc4 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -218,7 +218,8 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 		if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
 		    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
 			continue;
-		atomic_inc(&clnt->cl_count);
+		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
+			continue;
 		spin_unlock(&sn->rpc_client_lock);
 		return clnt;
 	}

Re: [PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[J. Bruce Fields]

On Fri, Apr 20, 2012 at 06:11:02PM +0400, Stanislav Kinsbursky wrote:
> v2: atomic_inc_return() was replaced by atomic_inc_not_zero().
> 
> These clients can't be safely dereferenced if their counter in 0.

I'm pretty confused by how these notifiers work....

rpc_release_client decrements cl_count to zero temporarily, to have it
immediately re-incremented by rpc_free_auth.

So if we're called concurrently with rpc_release_client then it's sort
of random whether someone gets this callback.

Is that a problem?

Also, is this an existing bug?  (In which case Trond should take it
now.)

--b.

> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> 
> ---
>  net/sunrpc/clnt.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 6797246..d10ebc4 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -218,7 +218,8 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
>  		if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
>  		    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
>  			continue;
> -		atomic_inc(&clnt->cl_count);
> +		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
> +			continue;
>  		spin_unlock(&sn->rpc_client_lock);
>  		return clnt;
>  	}
> 

Re: [PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[Myklebust, Trond]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1352 bytes --]

On Wed, 2012-04-25 at 13:30 -0400, J. Bruce Fields wrote:
> On Fri, Apr 20, 2012 at 06:11:02PM +0400, Stanislav Kinsbursky wrote:
> > v2: atomic_inc_return() was replaced by atomic_inc_not_zero().
> > 
> > These clients can't be safely dereferenced if their counter in 0.
> 
> I'm pretty confused by how these notifiers work....
> 
> rpc_release_client decrements cl_count to zero temporarily, to have it
> immediately re-incremented by rpc_free_auth.
> 
> So if we're called concurrently with rpc_release_client then it's sort
> of random whether someone gets this callback.
> 
> Is that a problem?

Not really. If we re-increment the client->cl_count in rpc_free_auth()
then it would be so that we can send off a bunch of NULL rpc calls to
destroy existing RPCSEC_GSS contexts. We shouldn't need to do any more
upcalls in pipefs.

If we care, we could simply move the call to rpc_unregister_client()
into rpc_free_auth() so that the pipefs notifier doesn't see us, or we
could set a flag to have it ignore us.

> Also, is this an existing bug?  (In which case Trond should take it
> now.)


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[Myklebust, Trond]

T24gV2VkLCAyMDEyLTA0LTI1IGF0IDEzOjMwIC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIEZyaSwgQXByIDIwLCAyMDEyIGF0IDA2OjExOjAyUE0gKzA0MDAsIFN0YW5pc2xhdiBL
aW5zYnVyc2t5IHdyb3RlOg0KPiA+IHYyOiBhdG9taWNfaW5jX3JldHVybigpIHdhcyByZXBsYWNl
ZCBieSBhdG9taWNfaW5jX25vdF96ZXJvKCkuDQo+ID4gDQo+ID4gVGhlc2UgY2xpZW50cyBjYW4n
dCBiZSBzYWZlbHkgZGVyZWZlcmVuY2VkIGlmIHRoZWlyIGNvdW50ZXIgaW4gMC4NCj4gDQo+IEkn
bSBwcmV0dHkgY29uZnVzZWQgYnkgaG93IHRoZXNlIG5vdGlmaWVycyB3b3JrLi4uLg0KPiANCj4g
cnBjX3JlbGVhc2VfY2xpZW50IGRlY3JlbWVudHMgY2xfY291bnQgdG8gemVybyB0ZW1wb3Jhcmls
eSwgdG8gaGF2ZSBpdA0KPiBpbW1lZGlhdGVseSByZS1pbmNyZW1lbnRlZCBieSBycGNfZnJlZV9h
dXRoLg0KPiANCj4gU28gaWYgd2UncmUgY2FsbGVkIGNvbmN1cnJlbnRseSB3aXRoIHJwY19yZWxl
YXNlX2NsaWVudCB0aGVuIGl0J3Mgc29ydA0KPiBvZiByYW5kb20gd2hldGhlciBzb21lb25lIGdl
dHMgdGhpcyBjYWxsYmFjay4NCj4gDQo+IElzIHRoYXQgYSBwcm9ibGVtPw0KDQpOb3QgcmVhbGx5
LiBJZiB3ZSByZS1pbmNyZW1lbnQgdGhlIGNsaWVudC0+Y2xfY291bnQgaW4gcnBjX2ZyZWVfYXV0
aCgpDQp0aGVuIGl0IHdvdWxkIGJlIHNvIHRoYXQgd2UgY2FuIHNlbmQgb2ZmIGEgYnVuY2ggb2Yg
TlVMTCBycGMgY2FsbHMgdG8NCmRlc3Ryb3kgZXhpc3RpbmcgUlBDU0VDX0dTUyBjb250ZXh0cy4g
V2Ugc2hvdWxkbid0IG5lZWQgdG8gZG8gYW55IG1vcmUNCnVwY2FsbHMgaW4gcGlwZWZzLg0KDQpJ
ZiB3ZSBjYXJlLCB3ZSBjb3VsZCBzaW1wbHkgbW92ZSB0aGUgY2FsbCB0byBycGNfdW5yZWdpc3Rl
cl9jbGllbnQoKQ0KaW50byBycGNfZnJlZV9hdXRoKCkgc28gdGhhdCB0aGUgcGlwZWZzIG5vdGlm
aWVyIGRvZXNuJ3Qgc2VlIHVzLCBvciB3ZQ0KY291bGQgc2V0IGEgZmxhZyB0byBoYXZlIGl0IGln
bm9yZSB1cy4NCg0KPiBBbHNvLCBpcyB0aGlzIGFuIGV4aXN0aW5nIGJ1Zz8gIChJbiB3aGljaCBj
YXNlIFRyb25kIHNob3VsZCB0YWtlIGl0DQo+IG5vdy4pDQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1
c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVz
dEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[Stanislav Kinsbursky]

25.04.2012 21:30, J. Bruce Fields написал:
> On Fri, Apr 20, 2012 at 06:11:02PM +0400, Stanislav Kinsbursky wrote:
>> v2: atomic_inc_return() was replaced by atomic_inc_not_zero().
>>
>> These clients can't be safely dereferenced if their counter in 0.
> I'm pretty confused by how these notifiers work....

There were made as simple as possible - i.e. notifier holds a client 
while creating of destroying PipeFS dentries. But event in this case 
there were races.

> rpc_release_client decrements cl_count to zero temporarily, to have it
> immediately re-incremented by rpc_free_auth.

BTW, I'm really confused with these re-incrementing reference counter 
technic. It makes life-time of RPC client unpredictable.
Is this a real-world valid situation, when usage of it reached zero, but 
while we destroying auth, there can some other user of client appear and 
client become alive again?
It it was done just to make sure that client is still active while we 
destroying auth, then maybe we can just remove the client from the 
clients list before rpc_free_auth? It will simplify the notifier 
callback logic greatly...


> So if we're called concurrently with rpc_release_client then it's sort
> of random whether someone gets this callback.
>
> Is that a problem?
>
> Also, is this an existing bug?  (In which case Trond should take it
> now.)

This is probably not a bug (I can't llok at the code right now; because 
these dentries will be destroyed), but a flaw.
Today (without this patch) notifier can try to create dentries for 
clients, which are dead already (i.e. auth was destroyed and client is 
going to be destroyed very soon, but notifier gained lock first.


>
> --b.
>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>
>> ---
>>   net/sunrpc/clnt.c |    3 ++-
>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 6797246..d10ebc4 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -218,7 +218,8 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
>>   		if (((event == RPC_PIPEFS_MOUNT)&&  clnt->cl_dentry) ||
>>   		    ((event == RPC_PIPEFS_UMOUNT)&&  !clnt->cl_dentry))
>>   			continue;
>> -		atomic_inc(&clnt->cl_count);
>> +		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
>> +			continue;
>>   		spin_unlock(&sn->rpc_client_lock);
>>   		return clnt;
>>   	}
>>

Re: [PATCH v2] SUNRPC: skip dead but not buried clients on PipeFS events

[Stanislav Kinsbursky]

25.04.2012 21:30, J. Bruce Fields написал:
> On Fri, Apr 20, 2012 at 06:11:02PM +0400, Stanislav Kinsbursky wrote:
>> v2: atomic_inc_return() was replaced by atomic_inc_not_zero().
>>
>> These clients can't be safely dereferenced if their counter in 0.
> I'm pretty confused by how these notifiers work....
>
> rpc_release_client decrements cl_count to zero temporarily, to have it
> immediately re-incremented by rpc_free_auth.
>
> So if we're called concurrently with rpc_release_client then it's sort
> of random whether someone gets this callback.
>
> Is that a problem?
>
> Also, is this an existing bug?  (In which case Trond should take it
> now.)

Sorry, I was mistaken in previous letter.
Yes, this is an existent bug.
I.e. without this patch notifier can dereference a client, which is 
actually dead already, but haven't deleted itself from the client's list.
And then notifier will try to work with this client and even release it 
at the end.