[PATCH] SUNRPC: return negative value in case rpcbind client creation error

[PATCH] SUNRPC: return negative value in case rpcbind client creation error

[Stanislav Kinsbursky]

Without this patch kernel will panic on LockD start, because lockd_up() checks
lockd_up_net() result for negative value.
>From my pow it's better to return negative value from rpcbind routines instead
of replacing all such checks like in lockd_up().

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 net/sunrpc/rpcb_clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 92509ff..a70acae 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -251,7 +251,7 @@ static int rpcb_create_local_unix(struct net *net)
 	if (IS_ERR(clnt)) {
 		dprintk("RPC:       failed to create AF_LOCAL rpcbind "
 				"client (errno %ld).\n", PTR_ERR(clnt));
-		result = -PTR_ERR(clnt);
+		result = PTR_ERR(clnt);
 		goto out;
 	}
 
@@ -298,7 +298,7 @@ static int rpcb_create_local_net(struct net *net)
 	if (IS_ERR(clnt)) {
 		dprintk("RPC:       failed to create local rpcbind "
 				"client (errno %ld).\n", PTR_ERR(clnt));
-		result = -PTR_ERR(clnt);
+		result = PTR_ERR(clnt);
 		goto out;
 	}
 

Re: [PATCH] SUNRPC: return negative value in case rpcbind client creation error

[Myklebust, Trond]

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

On Fri, 2012-07-20 at 15:57 +0400, Stanislav Kinsbursky wrote:
> Without this patch kernel will panic on LockD start, because lockd_up() checks
> lockd_up_net() result for negative value.
> >From my pow it's better to return negative value from rpcbind routines instead
> of replacing all such checks like in lockd_up().
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> ---
>  net/sunrpc/rpcb_clnt.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index 92509ff..a70acae 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -251,7 +251,7 @@ static int rpcb_create_local_unix(struct net *net)
>  	if (IS_ERR(clnt)) {
>  		dprintk("RPC:       failed to create AF_LOCAL rpcbind "
>  				"client (errno %ld).\n", PTR_ERR(clnt));
> -		result = -PTR_ERR(clnt);
> +		result = PTR_ERR(clnt);
>  		goto out;
>  	}
>  
> @@ -298,7 +298,7 @@ static int rpcb_create_local_net(struct net *net)
>  	if (IS_ERR(clnt)) {
>  		dprintk("RPC:       failed to create local rpcbind "
>  				"client (errno %ld).\n", PTR_ERR(clnt));
> -		result = -PTR_ERR(clnt);
> +		result = PTR_ERR(clnt);
>  		goto out;
>  	}

Who is supposed to carry this patch? Is it Bruce or is it me?

Cheers
  Trond

ÿôèº{.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] SUNRPC: return negative value in case rpcbind client creation error

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTA3LTIwIGF0IDE1OjU3ICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3
cm90ZToNCj4gV2l0aG91dCB0aGlzIHBhdGNoIGtlcm5lbCB3aWxsIHBhbmljIG9uIExvY2tEIHN0
YXJ0LCBiZWNhdXNlIGxvY2tkX3VwKCkgY2hlY2tzDQo+IGxvY2tkX3VwX25ldCgpIHJlc3VsdCBm
b3IgbmVnYXRpdmUgdmFsdWUuDQo+ID5Gcm9tIG15IHBvdyBpdCdzIGJldHRlciB0byByZXR1cm4g
bmVnYXRpdmUgdmFsdWUgZnJvbSBycGNiaW5kIHJvdXRpbmVzIGluc3RlYWQNCj4gb2YgcmVwbGFj
aW5nIGFsbCBzdWNoIGNoZWNrcyBsaWtlIGluIGxvY2tkX3VwKCkuDQo+IA0KPiBTaWduZWQtb2Zm
LWJ5OiBTdGFuaXNsYXYgS2luc2J1cnNreSA8c2tpbnNidXJza3lAcGFyYWxsZWxzLmNvbT4NCj4g
LS0tDQo+ICBuZXQvc3VucnBjL3JwY2JfY2xudC5jIHwgICAgNCArKy0tDQo+ICAxIGZpbGVzIGNo
YW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQg
YS9uZXQvc3VucnBjL3JwY2JfY2xudC5jIGIvbmV0L3N1bnJwYy9ycGNiX2NsbnQuYw0KPiBpbmRl
eCA5MjUwOWZmLi5hNzBhY2FlIDEwMDY0NA0KPiAtLS0gYS9uZXQvc3VucnBjL3JwY2JfY2xudC5j
DQo+ICsrKyBiL25ldC9zdW5ycGMvcnBjYl9jbG50LmMNCj4gQEAgLTI1MSw3ICsyNTEsNyBAQCBz
dGF0aWMgaW50IHJwY2JfY3JlYXRlX2xvY2FsX3VuaXgoc3RydWN0IG5ldCAqbmV0KQ0KPiAgCWlm
IChJU19FUlIoY2xudCkpIHsNCj4gIAkJZHByaW50aygiUlBDOiAgICAgICBmYWlsZWQgdG8gY3Jl
YXRlIEFGX0xPQ0FMIHJwY2JpbmQgIg0KPiAgCQkJCSJjbGllbnQgKGVycm5vICVsZCkuXG4iLCBQ
VFJfRVJSKGNsbnQpKTsNCj4gLQkJcmVzdWx0ID0gLVBUUl9FUlIoY2xudCk7DQo+ICsJCXJlc3Vs
dCA9IFBUUl9FUlIoY2xudCk7DQo+ICAJCWdvdG8gb3V0Ow0KPiAgCX0NCj4gIA0KPiBAQCAtMjk4
LDcgKzI5OCw3IEBAIHN0YXRpYyBpbnQgcnBjYl9jcmVhdGVfbG9jYWxfbmV0KHN0cnVjdCBuZXQg
Km5ldCkNCj4gIAlpZiAoSVNfRVJSKGNsbnQpKSB7DQo+ICAJCWRwcmludGsoIlJQQzogICAgICAg
ZmFpbGVkIHRvIGNyZWF0ZSBsb2NhbCBycGNiaW5kICINCj4gIAkJCQkiY2xpZW50IChlcnJubyAl
bGQpLlxuIiwgUFRSX0VSUihjbG50KSk7DQo+IC0JCXJlc3VsdCA9IC1QVFJfRVJSKGNsbnQpOw0K
PiArCQlyZXN1bHQgPSBQVFJfRVJSKGNsbnQpOw0KPiAgCQlnb3RvIG91dDsNCj4gIAl9DQoNCldo
byBpcyBzdXBwb3NlZCB0byBjYXJyeSB0aGlzIHBhdGNoPyBJcyBpdCBCcnVjZSBvciBpcyBpdCBt
ZT8NCg0KQ2hlZXJzDQogIFRyb25kDQoNCg==

Re: [PATCH] SUNRPC: return negative value in case rpcbind client creation error

[bfields]

On Mon, Jul 30, 2012 at 11:12:05PM +0000, Myklebust, Trond wrote:
> On Fri, 2012-07-20 at 15:57 +0400, Stanislav Kinsbursky wrote:
> > Without this patch kernel will panic on LockD start, because lockd_up() checks
> > lockd_up_net() result for negative value.
> > >From my pow it's better to return negative value from rpcbind routines instead
> > of replacing all such checks like in lockd_up().
> > 
> > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> > ---
> >  net/sunrpc/rpcb_clnt.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > index 92509ff..a70acae 100644
> > --- a/net/sunrpc/rpcb_clnt.c
> > +++ b/net/sunrpc/rpcb_clnt.c
> > @@ -251,7 +251,7 @@ static int rpcb_create_local_unix(struct net *net)
> >  	if (IS_ERR(clnt)) {
> >  		dprintk("RPC:       failed to create AF_LOCAL rpcbind "
> >  				"client (errno %ld).\n", PTR_ERR(clnt));
> > -		result = -PTR_ERR(clnt);
> > +		result = PTR_ERR(clnt);
> >  		goto out;
> >  	}
> >  
> > @@ -298,7 +298,7 @@ static int rpcb_create_local_net(struct net *net)
> >  	if (IS_ERR(clnt)) {
> >  		dprintk("RPC:       failed to create local rpcbind "
> >  				"client (errno %ld).\n", PTR_ERR(clnt));
> > -		result = -PTR_ERR(clnt);
> > +		result = PTR_ERR(clnt);
> >  		goto out;
> >  	}
> 
> Who is supposed to carry this patch? Is it Bruce or is it me?

Works either way.  Either way--it looks like the bug was introduced with

c526611dd631b2802b6b0221ffb306c5fa25c86c "SUNRPC: Use a cached RPC
client and transport for rpcbind upcalls" and
7402ab19cdd5943c7dd4f3399afe3abda8077ef5 "SUNRPC: Use AF_LOCAL for
rpcbind upcalls"

and should go to stable as well.

(Looks like I said that before but accidentally dropped everyone off the
cc.)

--b.

Re: [PATCH] SUNRPC: return negative value in case rpcbind client creation error

[Stanislav Kinsbursky]

31.07.2012 03:12, Myklebust, Trond пишет:
> On Fri, 2012-07-20 at 15:57 +0400, Stanislav Kinsbursky wrote:
>> Without this patch kernel will panic on LockD start, because lockd_up() checks
>> lockd_up_net() result for negative value.
>> >From my pow it's better to return negative value from rpcbind routines instead
>> of replacing all such checks like in lockd_up().
>>
>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>> ---
>>   net/sunrpc/rpcb_clnt.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>> index 92509ff..a70acae 100644
>> --- a/net/sunrpc/rpcb_clnt.c
>> +++ b/net/sunrpc/rpcb_clnt.c
>> @@ -251,7 +251,7 @@ static int rpcb_create_local_unix(struct net *net)
>>   	if (IS_ERR(clnt)) {
>>   		dprintk("RPC:       failed to create AF_LOCAL rpcbind "
>>   				"client (errno %ld).\n", PTR_ERR(clnt));
>> -		result = -PTR_ERR(clnt);
>> +		result = PTR_ERR(clnt);
>>   		goto out;
>>   	}
>>
>> @@ -298,7 +298,7 @@ static int rpcb_create_local_net(struct net *net)
>>   	if (IS_ERR(clnt)) {
>>   		dprintk("RPC:       failed to create local rpcbind "
>>   				"client (errno %ld).\n", PTR_ERR(clnt));
>> -		result = -PTR_ERR(clnt);
>> +		result = PTR_ERR(clnt);
>>   		goto out;
>>   	}
>
> Who is supposed to carry this patch? Is it Bruce or is it me?
>

I don't know, Trond. It's up to you and Bruce.
This is a bug fix and the bug is very old. The only reason, why it was found 
just now, is that all the callers of these functions were checking the result 
for zero.
And I agreed with Bruce, that is have to marked for stable branches (at least 
for 3.4-3.5 kernels).

-- 
Best regards,
Stanislav Kinsbursky