From: Bryan Schumaker <[email protected]>
I was treating a NULL return value as an error, but this function will
instead return an ERR_PTR(). Now that I'm checking if the function
returns an error, I should also pass the error up the call stack.
Signed-off-by: Bryan Schumaker <[email protected]>
---
fs/nfs/nfs4namespace.c | 4 ++--
fs/nfs/nfs4proc.c | 15 ++++-----------
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 017b4b0..a8aafc6 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -205,9 +205,9 @@ struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *ino
return clone;
auth = rpcauth_create(flavor, clone);
- if (!auth) {
+ if (IS_ERR(auth)) {
rpc_shutdown_client(clone);
- clone = ERR_PTR(-EIO);
+ clone = ERR_CAST(auth);
}
return clone;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5a7b372..f817587 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2356,17 +2356,10 @@ out:
static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
struct nfs_fsinfo *info, rpc_authflavor_t flavor)
{
- struct rpc_auth *auth;
- int ret;
-
- auth = rpcauth_create(flavor, server->client);
- if (!auth) {
- ret = -EIO;
- goto out;
- }
- ret = nfs4_lookup_root(server, fhandle, info);
-out:
- return ret;
+ struct rpc_auth *auth = rpcauth_create(flavor, server->client);
+ if (IS_ERR(auth))
+ return PTR_ERR(auth);
+ return nfs4_lookup_root(server, fhandle, info);
}
static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
--
1.7.11.1
On 06/26/2012 02:23 PM, Chuck Lever wrote:
>
> On Jun 26, 2012, at 2:12 PM, [email protected] wrote:
>
>> From: Bryan Schumaker <[email protected]>
>>
>> I was treating a NULL return value as an error, but this function will
>> instead return an ERR_PTR(). Now that I'm checking if the function
>> returns an error, I should also pass the error up the call stack.
>
>
>> Signed-off-by: Bryan Schumaker <[email protected]>
>> ---
>> fs/nfs/nfs4namespace.c | 4 ++--
>> fs/nfs/nfs4proc.c | 15 ++++-----------
>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>> index 017b4b0..a8aafc6 100644
>> --- a/fs/nfs/nfs4namespace.c
>> +++ b/fs/nfs/nfs4namespace.c
>> @@ -205,9 +205,9 @@ struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *ino
>> return clone;
>>
>> auth = rpcauth_create(flavor, clone);
>> - if (!auth) {
>> + if (IS_ERR(auth)) {
>> rpc_shutdown_client(clone);
>> - clone = ERR_PTR(-EIO);
>> + clone = ERR_CAST(auth);
>> }
>
> This echoes nfs_init_server_rpcclient(), so it should be adequate.
>
>>
>> return clone;
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 5a7b372..f817587 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2356,17 +2356,10 @@ out:
>> static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>> struct nfs_fsinfo *info, rpc_authflavor_t flavor)
>> {
>> - struct rpc_auth *auth;
>> - int ret;
>> -
>> - auth = rpcauth_create(flavor, server->client);
>> - if (!auth) {
>> - ret = -EIO;
>> - goto out;
>> - }
>> - ret = nfs4_lookup_root(server, fhandle, info);
>> -out:
>> - return ret;
>> + struct rpc_auth *auth = rpcauth_create(flavor, server->client);
>> + if (IS_ERR(auth))
>> + return PTR_ERR(auth);
>> + return nfs4_lookup_root(server, fhandle, info);
>> }
>>
>> static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>
> This logic calls rpcauth_create() in a loop, correct? The expectation is that rpcauth_create() should simply replace the nfs_client's rpc_auth with the next flavor each time it is called?
>
Yeah, that's what I'm expecting to happen.
> With the current code in nfs4_find_root_sec(), rpcauth_create() with a GSS flavor fails the second time you call it because (I suspect) there's a pipe dentry left over from the previous call. So there's no way to walk an entire list returned from gss_mech_list_flavors(), it will fail on the second GSS flavor every time.
rpcauth_create() has this bit of code in it:
if (clnt->cl_auth)
rpcauth_release(clnt->cl_auth);
clnt->cl_auth = auth;
I guess I figured rpcauth_release() was already doing the necessary cleanup.
- Bryan
>
> We could:
>
> a) have gss_pipes_dentries_create_net() ignore -EEXIST and return zero instead, or
>
> b) make sure rpcauth_create() releases the old rpc_auth before calling op->create()
>
> c) ??
>
On Jun 26, 2012, at 3:00 PM, Bryan Schumaker wrote:
> On 06/26/2012 02:23 PM, Chuck Lever wrote:
>>
>> On Jun 26, 2012, at 2:12 PM, [email protected] wrote:
>>
>>> From: Bryan Schumaker <[email protected]>
>>>
>>> I was treating a NULL return value as an error, but this function will
>>> instead return an ERR_PTR(). Now that I'm checking if the function
>>> returns an error, I should also pass the error up the call stack.
>>
>>
>>> Signed-off-by: Bryan Schumaker <[email protected]>
>>> ---
>>> fs/nfs/nfs4namespace.c | 4 ++--
>>> fs/nfs/nfs4proc.c | 15 ++++-----------
>>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>>> index 017b4b0..a8aafc6 100644
>>> --- a/fs/nfs/nfs4namespace.c
>>> +++ b/fs/nfs/nfs4namespace.c
>>> @@ -205,9 +205,9 @@ struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *ino
>>> return clone;
>>>
>>> auth = rpcauth_create(flavor, clone);
>>> - if (!auth) {
>>> + if (IS_ERR(auth)) {
>>> rpc_shutdown_client(clone);
>>> - clone = ERR_PTR(-EIO);
>>> + clone = ERR_CAST(auth);
>>> }
>>
>> This echoes nfs_init_server_rpcclient(), so it should be adequate.
>>
>>>
>>> return clone;
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 5a7b372..f817587 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2356,17 +2356,10 @@ out:
>>> static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>>> struct nfs_fsinfo *info, rpc_authflavor_t flavor)
>>> {
>>> - struct rpc_auth *auth;
>>> - int ret;
>>> -
>>> - auth = rpcauth_create(flavor, server->client);
>>> - if (!auth) {
>>> - ret = -EIO;
>>> - goto out;
>>> - }
>>> - ret = nfs4_lookup_root(server, fhandle, info);
>>> -out:
>>> - return ret;
>>> + struct rpc_auth *auth = rpcauth_create(flavor, server->client);
>>> + if (IS_ERR(auth))
>>> + return PTR_ERR(auth);
>>> + return nfs4_lookup_root(server, fhandle, info);
>>> }
>>>
>>> static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>>
>> This logic calls rpcauth_create() in a loop, correct? The expectation is that rpcauth_create() should simply replace the nfs_client's rpc_auth with the next flavor each time it is called?
>>
>
> Yeah, that's what I'm expecting to happen.
>
>> With the current code in nfs4_find_root_sec(), rpcauth_create() with a GSS flavor fails the second time you call it because (I suspect) there's a pipe dentry left over from the previous call. So there's no way to walk an entire list returned from gss_mech_list_flavors(), it will fail on the second GSS flavor every time.
>
> rpcauth_create() has this bit of code in it:
>
> if (clnt->cl_auth)
> rpcauth_release(clnt->cl_auth);
> clnt->cl_auth = auth;
>
>
> I guess I figured rpcauth_release() was already doing the necessary cleanup.
The problem is that all GSS pseudoflavors use the same "gssd" upcall pipe, but the RPC client does not appear to be prepared to share that pipe amongst pseudoflavors that exist concurrently.
So if rpcauth_create() is called to substitute one GSS pseudoflavor for another, it will always fail because the "gssd" pipe already exists.
It appears this bug has been around since late 2008 when the new GSS upcall mechanism was added. We've never hit it because we've never tried to use rpcauth_create() to replace an rpc_auth before. Any caller who is setting up a fresh rpc_clnt can handle a failure from rpcauth_create() reasonably well.
But a caller who is attempting to retry an rpcauth_create() with a list of different flavors is SOL; at least the current code leaves the rpc_clnt with the original, working rpc_auth, but it won't allow a caller to replace a GSS flavor with another.
How hard would it be to enable the gssd upcall pipe to be shared?
> - Bryan
>>
>> We could:
>>
>> a) have gss_pipes_dentries_create_net() ignore -EEXIST and return zero instead, or
>>
>> b) make sure rpcauth_create() releases the old rpc_auth before calling op->create()
>>
>> c) ??
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
T24gVHVlLCAyMDEyLTA2LTI2IGF0IDE3OjM0IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gSnVuIDI2LCAyMDEyLCBhdCAzOjAwIFBNLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6DQo+IA0K
PiA+IE9uIDA2LzI2LzIwMTIgMDI6MjMgUE0sIENodWNrIExldmVyIHdyb3RlOg0KPiA+PiANCj4g
Pj4gT24gSnVuIDI2LCAyMDEyLCBhdCAyOjEyIFBNLCBianNjaHVtYUBuZXRhcHAuY29tIHdyb3Rl
Og0KPiA+PiANCj4gPj4+IEZyb206IEJyeWFuIFNjaHVtYWtlciA8YmpzY2h1bWFAbmV0YXBwLmNv
bT4NCj4gPj4+IA0KPiA+Pj4gSSB3YXMgdHJlYXRpbmcgYSBOVUxMIHJldHVybiB2YWx1ZSBhcyBh
biBlcnJvciwgYnV0IHRoaXMgZnVuY3Rpb24gd2lsbA0KPiA+Pj4gaW5zdGVhZCByZXR1cm4gYW4g
RVJSX1BUUigpLiAgTm93IHRoYXQgSSdtIGNoZWNraW5nIGlmIHRoZSBmdW5jdGlvbg0KPiA+Pj4g
cmV0dXJucyBhbiBlcnJvciwgSSBzaG91bGQgYWxzbyBwYXNzIHRoZSBlcnJvciB1cCB0aGUgY2Fs
bCBzdGFjay4NCj4gPj4gDQo+ID4+IA0KPiA+Pj4gU2lnbmVkLW9mZi1ieTogQnJ5YW4gU2NodW1h
a2VyIDxianNjaHVtYUBuZXRhcHAuY29tPg0KPiA+Pj4gLS0tDQo+ID4+PiBmcy9uZnMvbmZzNG5h
bWVzcGFjZS5jIHwgIDQgKystLQ0KPiA+Pj4gZnMvbmZzL25mczRwcm9jLmMgICAgICB8IDE1ICsr
KystLS0tLS0tLS0tLQ0KPiA+Pj4gMiBmaWxlcyBjaGFuZ2VkLCA2IGluc2VydGlvbnMoKyksIDEz
IGRlbGV0aW9ucygtKQ0KPiA+Pj4gDQo+ID4+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRuYW1l
c3BhY2UuYyBiL2ZzL25mcy9uZnM0bmFtZXNwYWNlLmMNCj4gPj4+IGluZGV4IDAxN2I0YjAuLmE4
YWFmYzYgMTAwNjQ0DQo+ID4+PiAtLS0gYS9mcy9uZnMvbmZzNG5hbWVzcGFjZS5jDQo+ID4+PiAr
KysgYi9mcy9uZnMvbmZzNG5hbWVzcGFjZS5jDQo+ID4+PiBAQCAtMjA1LDkgKzIwNSw5IEBAIHN0
cnVjdCBycGNfY2xudCAqbmZzNF9jcmVhdGVfc2VjX2NsaWVudChzdHJ1Y3QgcnBjX2NsbnQgKmNs
bnQsIHN0cnVjdCBpbm9kZSAqaW5vDQo+ID4+PiAJCXJldHVybiBjbG9uZTsNCj4gPj4+IA0KPiA+
Pj4gCWF1dGggPSBycGNhdXRoX2NyZWF0ZShmbGF2b3IsIGNsb25lKTsNCj4gPj4+IC0JaWYgKCFh
dXRoKSB7DQo+ID4+PiArCWlmIChJU19FUlIoYXV0aCkpIHsNCj4gPj4+IAkJcnBjX3NodXRkb3du
X2NsaWVudChjbG9uZSk7DQo+ID4+PiAtCQljbG9uZSA9IEVSUl9QVFIoLUVJTyk7DQo+ID4+PiAr
CQljbG9uZSA9IEVSUl9DQVNUKGF1dGgpOw0KPiA+Pj4gCX0NCj4gPj4gDQo+ID4+IFRoaXMgZWNo
b2VzIG5mc19pbml0X3NlcnZlcl9ycGNjbGllbnQoKSwgc28gaXQgc2hvdWxkIGJlIGFkZXF1YXRl
Lg0KPiA+PiANCj4gPj4+IA0KPiA+Pj4gCXJldHVybiBjbG9uZTsNCj4gPj4+IGRpZmYgLS1naXQg
YS9mcy9uZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4+PiBpbmRleCA1YTdi
MzcyLi5mODE3NTg3IDEwMDY0NA0KPiA+Pj4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCj4gPj4+
ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4+PiBAQCAtMjM1NiwxNyArMjM1NiwxMCBAQCBv
dXQ6DQo+ID4+PiBzdGF0aWMgaW50IG5mczRfbG9va3VwX3Jvb3Rfc2VjKHN0cnVjdCBuZnNfc2Vy
dmVyICpzZXJ2ZXIsIHN0cnVjdCBuZnNfZmggKmZoYW5kbGUsDQo+ID4+PiAJCQkJc3RydWN0IG5m
c19mc2luZm8gKmluZm8sIHJwY19hdXRoZmxhdm9yX3QgZmxhdm9yKQ0KPiA+Pj4gew0KPiA+Pj4g
LQlzdHJ1Y3QgcnBjX2F1dGggKmF1dGg7DQo+ID4+PiAtCWludCByZXQ7DQo+ID4+PiAtDQo+ID4+
PiAtCWF1dGggPSBycGNhdXRoX2NyZWF0ZShmbGF2b3IsIHNlcnZlci0+Y2xpZW50KTsNCj4gPj4+
IC0JaWYgKCFhdXRoKSB7DQo+ID4+PiAtCQlyZXQgPSAtRUlPOw0KPiA+Pj4gLQkJZ290byBvdXQ7
DQo+ID4+PiAtCX0NCj4gPj4+IC0JcmV0ID0gbmZzNF9sb29rdXBfcm9vdChzZXJ2ZXIsIGZoYW5k
bGUsIGluZm8pOw0KPiA+Pj4gLW91dDoNCj4gPj4+IC0JcmV0dXJuIHJldDsNCj4gPj4+ICsJc3Ry
dWN0IHJwY19hdXRoICphdXRoID0gcnBjYXV0aF9jcmVhdGUoZmxhdm9yLCBzZXJ2ZXItPmNsaWVu
dCk7DQo+ID4+PiArCWlmIChJU19FUlIoYXV0aCkpDQo+ID4+PiArCQlyZXR1cm4gUFRSX0VSUihh
dXRoKTsNCj4gPj4+ICsJcmV0dXJuIG5mczRfbG9va3VwX3Jvb3Qoc2VydmVyLCBmaGFuZGxlLCBp
bmZvKTsNCj4gPj4+IH0NCj4gPj4+IA0KPiA+Pj4gc3RhdGljIGludCBuZnM0X2ZpbmRfcm9vdF9z
ZWMoc3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZlciwgc3RydWN0IG5mc19maCAqZmhhbmRsZSwNCj4g
Pj4gDQo+ID4+IFRoaXMgbG9naWMgY2FsbHMgcnBjYXV0aF9jcmVhdGUoKSBpbiBhIGxvb3AsIGNv
cnJlY3Q/ICBUaGUgZXhwZWN0YXRpb24gaXMgdGhhdCBycGNhdXRoX2NyZWF0ZSgpIHNob3VsZCBz
aW1wbHkgcmVwbGFjZSB0aGUgbmZzX2NsaWVudCdzIHJwY19hdXRoIHdpdGggdGhlIG5leHQgZmxh
dm9yIGVhY2ggdGltZSBpdCBpcyBjYWxsZWQ/DQo+ID4+IA0KPiA+IA0KPiA+IFllYWgsIHRoYXQn
cyB3aGF0IEknbSBleHBlY3RpbmcgdG8gaGFwcGVuLg0KPiA+IA0KPiA+PiBXaXRoIHRoZSBjdXJy
ZW50IGNvZGUgaW4gbmZzNF9maW5kX3Jvb3Rfc2VjKCksIHJwY2F1dGhfY3JlYXRlKCkgd2l0aCBh
IEdTUyBmbGF2b3IgZmFpbHMgdGhlIHNlY29uZCB0aW1lIHlvdSBjYWxsIGl0IGJlY2F1c2UgKEkg
c3VzcGVjdCkgdGhlcmUncyBhIHBpcGUgZGVudHJ5IGxlZnQgb3ZlciBmcm9tIHRoZSBwcmV2aW91
cyBjYWxsLiAgU28gdGhlcmUncyBubyB3YXkgdG8gd2FsayBhbiBlbnRpcmUgbGlzdCByZXR1cm5l
ZCBmcm9tIGdzc19tZWNoX2xpc3RfZmxhdm9ycygpLCBpdCB3aWxsIGZhaWwgb24gdGhlIHNlY29u
ZCBHU1MgZmxhdm9yIGV2ZXJ5IHRpbWUuDQo+ID4gDQo+ID4gcnBjYXV0aF9jcmVhdGUoKSBoYXMg
dGhpcyBiaXQgb2YgY29kZSBpbiBpdDoNCj4gPiANCj4gPiAJaWYgKGNsbnQtPmNsX2F1dGgpDQo+
ID4gCQlycGNhdXRoX3JlbGVhc2UoY2xudC0+Y2xfYXV0aCk7DQo+ID4gCWNsbnQtPmNsX2F1dGgg
PSBhdXRoOw0KPiA+IA0KPiA+IA0KPiA+IEkgZ3Vlc3MgSSBmaWd1cmVkIHJwY2F1dGhfcmVsZWFz
ZSgpIHdhcyBhbHJlYWR5IGRvaW5nIHRoZSBuZWNlc3NhcnkgY2xlYW51cC4NCj4gDQo+IFRoZSBw
cm9ibGVtIGlzIHRoYXQgYWxsIEdTUyBwc2V1ZG9mbGF2b3JzIHVzZSB0aGUgc2FtZSAiZ3NzZCIg
dXBjYWxsIHBpcGUsIGJ1dCB0aGUgUlBDIGNsaWVudCBkb2VzIG5vdCBhcHBlYXIgdG8gYmUgcHJl
cGFyZWQgdG8gc2hhcmUgdGhhdCBwaXBlIGFtb25nc3QgcHNldWRvZmxhdm9ycyB0aGF0IGV4aXN0
IGNvbmN1cnJlbnRseS4NCj4gDQo+IFNvIGlmIHJwY2F1dGhfY3JlYXRlKCkgaXMgY2FsbGVkIHRv
IHN1YnN0aXR1dGUgb25lIEdTUyBwc2V1ZG9mbGF2b3IgZm9yIGFub3RoZXIsIGl0IHdpbGwgYWx3
YXlzIGZhaWwgYmVjYXVzZSB0aGUgImdzc2QiIHBpcGUgYWxyZWFkeSBleGlzdHMuDQo+IA0KPiBJ
dCBhcHBlYXJzIHRoaXMgYnVnIGhhcyBiZWVuIGFyb3VuZCBzaW5jZSBsYXRlIDIwMDggd2hlbiB0
aGUgbmV3IEdTUyB1cGNhbGwgbWVjaGFuaXNtIHdhcyBhZGRlZC4gIFdlJ3ZlIG5ldmVyIGhpdCBp
dCBiZWNhdXNlIHdlJ3ZlIG5ldmVyIHRyaWVkIHRvIHVzZSBycGNhdXRoX2NyZWF0ZSgpIHRvIHJl
cGxhY2UgYW4gcnBjX2F1dGggYmVmb3JlLiAgQW55IGNhbGxlciB3aG8gaXMgc2V0dGluZyB1cCBh
IGZyZXNoIHJwY19jbG50IGNhbiBoYW5kbGUgYSBmYWlsdXJlIGZyb20gcnBjYXV0aF9jcmVhdGUo
KSByZWFzb25hYmx5IHdlbGwuDQo+IA0KPiBCdXQgYSBjYWxsZXIgd2hvIGlzIGF0dGVtcHRpbmcg
dG8gcmV0cnkgYW4gcnBjYXV0aF9jcmVhdGUoKSB3aXRoIGEgbGlzdCBvZiBkaWZmZXJlbnQgZmxh
dm9ycyBpcyBTT0w7IGF0IGxlYXN0IHRoZSBjdXJyZW50IGNvZGUgbGVhdmVzIHRoZSBycGNfY2xu
dCB3aXRoIHRoZSBvcmlnaW5hbCwgd29ya2luZyBycGNfYXV0aCwgYnV0IGl0IHdvbid0IGFsbG93
IGEgY2FsbGVyIHRvIHJlcGxhY2UgYSBHU1MgZmxhdm9yIHdpdGggYW5vdGhlci4NCj4gDQo+IEhv
dyBoYXJkIHdvdWxkIGl0IGJlIHRvIGVuYWJsZSB0aGUgZ3NzZCB1cGNhbGwgcGlwZSB0byBiZSBz
aGFyZWQ/DQoNCk5vLiBUaGlzIGlzIGEgbmV3IHByb2JsZW0sIGFuZCBpcyBwcm9iYWJseSBhIGNv
bnNlcXVlbmNlIG9mIHRoZSBuZXQNCm5hbWVzcGFjZSBjb2RlLiBQcmlvciB0byB0aGF0LCB0aGUg
cnBjYXV0aCBjb2RlIHdvdWxkIGp1c3QgdGFrZSBhDQpyZWZlcmVuY2UgdG8gdGhlIGV4aXN0aW5n
IHVwY2FsbCBwaXBlIGlmIGl0IGhhZCBhbHJlYWR5IGJlZW4gY3JlYXRlZC4NCg0KVGhlIHNvbHV0
aW9uIGhlcmUgd291bGQgYmUgdG8gY3JlYXRlIGEgcGVyLXJwY19jbGllbnQsIHBlci1waXBlbmFt
ZQ0Kc2hhcmVkICdycGNzZWNfZ3NzX3BpcGUnIG9iamVjdCB0aGF0IGhvbGRzIHRoZSB1cGNhbGwg
cGlwZSBkYXRhLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWlu
dGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAu
Y29tDQoNCg==
On Jun 26, 2012, at 3:00 PM, Bryan Schumaker wrote:
> On 06/26/2012 02:23 PM, Chuck Lever wrote:
>>
>> On Jun 26, 2012, at 2:12 PM, [email protected] wrote:
>>
>>> From: Bryan Schumaker <[email protected]>
>>>
>>> I was treating a NULL return value as an error, but this function will
>>> instead return an ERR_PTR(). Now that I'm checking if the function
>>> returns an error, I should also pass the error up the call stack.
>>
>>
>>> Signed-off-by: Bryan Schumaker <[email protected]>
>>> ---
>>> fs/nfs/nfs4namespace.c | 4 ++--
>>> fs/nfs/nfs4proc.c | 15 ++++-----------
>>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>>> index 017b4b0..a8aafc6 100644
>>> --- a/fs/nfs/nfs4namespace.c
>>> +++ b/fs/nfs/nfs4namespace.c
>>> @@ -205,9 +205,9 @@ struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *ino
>>> return clone;
>>>
>>> auth = rpcauth_create(flavor, clone);
>>> - if (!auth) {
>>> + if (IS_ERR(auth)) {
>>> rpc_shutdown_client(clone);
>>> - clone = ERR_PTR(-EIO);
>>> + clone = ERR_CAST(auth);
>>> }
>>
>> This echoes nfs_init_server_rpcclient(), so it should be adequate.
>>
>>>
>>> return clone;
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 5a7b372..f817587 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2356,17 +2356,10 @@ out:
>>> static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>>> struct nfs_fsinfo *info, rpc_authflavor_t flavor)
>>> {
>>> - struct rpc_auth *auth;
>>> - int ret;
>>> -
>>> - auth = rpcauth_create(flavor, server->client);
>>> - if (!auth) {
>>> - ret = -EIO;
>>> - goto out;
>>> - }
>>> - ret = nfs4_lookup_root(server, fhandle, info);
>>> -out:
>>> - return ret;
>>> + struct rpc_auth *auth = rpcauth_create(flavor, server->client);
>>> + if (IS_ERR(auth))
>>> + return PTR_ERR(auth);
>>> + return nfs4_lookup_root(server, fhandle, info);
>>> }
>>>
>>> static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
>>
>> This logic calls rpcauth_create() in a loop, correct? The expectation is that rpcauth_create() should simply replace the nfs_client's rpc_auth with the next flavor each time it is called?
>>
>
> Yeah, that's what I'm expecting to happen.
>
>> With the current code in nfs4_find_root_sec(), rpcauth_create() with a GSS flavor fails the second time you call it because (I suspect) there's a pipe dentry left over from the previous call. So there's no way to walk an entire list returned from gss_mech_list_flavors(), it will fail on the second GSS flavor every time.
>
> rpcauth_create() has this bit of code in it:
>
> if (clnt->cl_auth)
> rpcauth_release(clnt->cl_auth);
> clnt->cl_auth = auth;
>
>
> I guess I figured rpcauth_release() was already doing the necessary cleanup.
It's doing clean-up _after_ the op->create() call. That way if op->create() fails, clnt->cl_auth is unchanged, as rpcauth_create()'s caller expects.
But that means that any pipes created for clnt->cl_auth are still around while op->create() is working, and will cause rpc_mkpipe() to barf. I'm toying with these solutions:
>> We could:
>>
>> a) have gss_pipes_dentries_create_net() ignore -EEXIST and return zero instead, or
>>
>> b) make sure rpcauth_create() releases the old rpc_auth before calling op->create()
>>
>> c) ??
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Jun 26, 2012, at 2:12 PM, [email protected] wrote:
> From: Bryan Schumaker <[email protected]>
>
> I was treating a NULL return value as an error, but this function will
> instead return an ERR_PTR(). Now that I'm checking if the function
> returns an error, I should also pass the error up the call stack.
> Signed-off-by: Bryan Schumaker <[email protected]>
> ---
> fs/nfs/nfs4namespace.c | 4 ++--
> fs/nfs/nfs4proc.c | 15 ++++-----------
> 2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 017b4b0..a8aafc6 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -205,9 +205,9 @@ struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *ino
> return clone;
>
> auth = rpcauth_create(flavor, clone);
> - if (!auth) {
> + if (IS_ERR(auth)) {
> rpc_shutdown_client(clone);
> - clone = ERR_PTR(-EIO);
> + clone = ERR_CAST(auth);
> }
This echoes nfs_init_server_rpcclient(), so it should be adequate.
>
> return clone;
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 5a7b372..f817587 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2356,17 +2356,10 @@ out:
> static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
> struct nfs_fsinfo *info, rpc_authflavor_t flavor)
> {
> - struct rpc_auth *auth;
> - int ret;
> -
> - auth = rpcauth_create(flavor, server->client);
> - if (!auth) {
> - ret = -EIO;
> - goto out;
> - }
> - ret = nfs4_lookup_root(server, fhandle, info);
> -out:
> - return ret;
> + struct rpc_auth *auth = rpcauth_create(flavor, server->client);
> + if (IS_ERR(auth))
> + return PTR_ERR(auth);
> + return nfs4_lookup_root(server, fhandle, info);
> }
>
> static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
This logic calls rpcauth_create() in a loop, correct? The expectation is that rpcauth_create() should simply replace the nfs_client's rpc_auth with the next flavor each time it is called?
With the current code in nfs4_find_root_sec(), rpcauth_create() with a GSS flavor fails the second time you call it because (I suspect) there's a pipe dentry left over from the previous call. So there's no way to walk an entire list returned from gss_mech_list_flavors(), it will fail on the second GSS flavor every time.
We could:
a) have gss_pipes_dentries_create_net() ignore -EEXIST and return zero instead, or
b) make sure rpcauth_create() releases the old rpc_auth before calling op->create()
c) ??
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com