LinuxLists.cc - [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

2012-09-24 17:39:04

Subject: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

From: Bryan Schumaker <[email protected]>

f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP transport
regression) introduced the "alloc_slot" function for xprt operations,
but never created one for the backchannel operations. This patch fixes
a null pointer dereference when mounting NFS over v4.1.

Call Trace:
[<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
[<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
[<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
[<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
[<ffffffff81073589>] process_one_work+0x139/0x500
[<ffffffff81070e70>] ? alloc_worker+0x70/0x70
[<ffffffffa020e5f0>] ? __rpc_execute+0x400/0x400 [sunrpc]
[<ffffffff81073d1e>] worker_thread+0x15e/0x460
[<ffffffff8145c839>] ? preempt_schedule+0x49/0x70
[<ffffffff81073bc0>] ? rescuer_thread+0x230/0x230
[<ffffffff81079603>] kthread+0x93/0xa0
[<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
[<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
[<ffffffff81465d00>] ? gs_change+0x13/0x13

Signed-off-by: Bryan Schumaker <[email protected]>
---
net/sunrpc/xprtsock.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 86b7777..aaaadfb 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
static struct rpc_xprt_ops bc_tcp_ops = {
.reserve_xprt = xprt_reserve_xprt,
.release_xprt = xprt_release_xprt,
+ .alloc_slot = xprt_alloc_slot,
.rpcbind = xs_local_rpcbind,
.buf_alloc = bc_malloc,
.buf_free = bc_free,
--
1.7.12.1

2012-09-24 17:53:11

by Anna Schumaker

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On 09/24/2012 01:42 PM, J. Bruce Fields wrote:
> On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
>> From: Bryan Schumaker <[email protected]>
>>
>> f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP transport
>> regression) introduced the "alloc_slot" function for xprt operations,
>> but never created one for the backchannel operations. This patch fixes
>> a null pointer dereference when mounting NFS over v4.1.
>
> Thanks, I just rebased some of my work to 3.6 and ran across that! It
> crashes the 4.1 server very quickly....

That sounds like my story. It got my peer-to-peer server right away, too.

- Bryan

>
> --b.
>
>>
>> Call Trace:
>> [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
>> [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
>> [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
>> [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
>> [<ffffffff81073589>] process_one_work+0x139/0x500
>> [<ffffffff81070e70>] ? alloc_worker+0x70/0x70
>> [<ffffffffa020e5f0>] ? __rpc_execute+0x400/0x400 [sunrpc]
>> [<ffffffff81073d1e>] worker_thread+0x15e/0x460
>> [<ffffffff8145c839>] ? preempt_schedule+0x49/0x70
>> [<ffffffff81073bc0>] ? rescuer_thread+0x230/0x230
>> [<ffffffff81079603>] kthread+0x93/0xa0
>> [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
>> [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
>> [<ffffffff81465d00>] ? gs_change+0x13/0x13
>>
>> Signed-off-by: Bryan Schumaker <[email protected]>
>> ---
>> net/sunrpc/xprtsock.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 86b7777..aaaadfb 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
>> static struct rpc_xprt_ops bc_tcp_ops = {
>> .reserve_xprt = xprt_reserve_xprt,
>> .release_xprt = xprt_release_xprt,
>> + .alloc_slot = xprt_alloc_slot,
>> .rpcbind = xs_local_rpcbind,
>> .buf_alloc = bc_malloc,
>> .buf_free = bc_free,
>> --
>> 1.7.12.1
>>
>> --
>> 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

2012-09-24 17:42:49

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
> From: Bryan Schumaker <[email protected]>
>
> f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP transport
> regression) introduced the "alloc_slot" function for xprt operations,
> but never created one for the backchannel operations. This patch fixes
> a null pointer dereference when mounting NFS over v4.1.

Thanks, I just rebased some of my work to 3.6 and ran across that! It
crashes the 4.1 server very quickly....

--b.

>
> Call Trace:
> [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> [<ffffffff81073589>] process_one_work+0x139/0x500
> [<ffffffff81070e70>] ? alloc_worker+0x70/0x70
> [<ffffffffa020e5f0>] ? __rpc_execute+0x400/0x400 [sunrpc]
> [<ffffffff81073d1e>] worker_thread+0x15e/0x460
> [<ffffffff8145c839>] ? preempt_schedule+0x49/0x70
> [<ffffffff81073bc0>] ? rescuer_thread+0x230/0x230
> [<ffffffff81079603>] kthread+0x93/0xa0
> [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> [<ffffffff81465d00>] ? gs_change+0x13/0x13
>
> Signed-off-by: Bryan Schumaker <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 86b7777..aaaadfb 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> static struct rpc_xprt_ops bc_tcp_ops = {
> .reserve_xprt = xprt_reserve_xprt,
> .release_xprt = xprt_release_xprt,
> + .alloc_slot = xprt_alloc_slot,
> .rpcbind = xs_local_rpcbind,
> .buf_alloc = bc_malloc,
> .buf_free = bc_free,
> --
> 1.7.12.1
>
> --
> 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

2012-09-25 19:08:03

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On Mon, Sep 24, 2012 at 07:31:23PM +0000, Myklebust, Trond wrote:
> On Mon, 2012-09-24 at 13:52 -0400, Bryan Schumaker wrote:
> > On 09/24/2012 01:42 PM, J. Bruce Fields wrote:
> > > On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
> > >> From: Bryan Schumaker <[email protected]>
> > >>
> > >> f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP transport
> > >> regression) introduced the "alloc_slot" function for xprt operations,
> > >> but never created one for the backchannel operations. This patch fixes
> > >> a null pointer dereference when mounting NFS over v4.1.
> > >
> > > Thanks, I just rebased some of my work to 3.6 and ran across that! It
> > > crashes the 4.1 server very quickly....
> >
> > That sounds like my story. It got my peer-to-peer server right away, too.
> >
> > - Bryan
> >
> > >
> > > --b.
> > >
> > >>
> > >> Call Trace:
> > >> [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> > >> [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> > >> [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> > >> [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> > >> [<ffffffff81073589>] process_one_work+0x139/0x500
> > >> [<ffffffff81070e70>] ? alloc_worker+0x70/0x70
> > >> [<ffffffffa020e5f0>] ? __rpc_execute+0x400/0x400 [sunrpc]
> > >> [<ffffffff81073d1e>] worker_thread+0x15e/0x460
> > >> [<ffffffff8145c839>] ? preempt_schedule+0x49/0x70
> > >> [<ffffffff81073bc0>] ? rescuer_thread+0x230/0x230
> > >> [<ffffffff81079603>] kthread+0x93/0xa0
> > >> [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> > >> [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> > >> [<ffffffff81465d00>] ? gs_change+0x13/0x13
> > >>
> > >> Signed-off-by: Bryan Schumaker <[email protected]>
> > >> ---
> > >> net/sunrpc/xprtsock.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > >> index 86b7777..aaaadfb 100644
> > >> --- a/net/sunrpc/xprtsock.c
> > >> +++ b/net/sunrpc/xprtsock.c
> > >> @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> > >> static struct rpc_xprt_ops bc_tcp_ops = {
> > >> .reserve_xprt = xprt_reserve_xprt,
> > >> .release_xprt = xprt_release_xprt,
> > >> + .alloc_slot = xprt_alloc_slot,
> > >> .rpcbind = xs_local_rpcbind,
> > >> .buf_alloc = bc_malloc,
> > >> .buf_free = bc_free,
> > >> --
> > >> 1.7.12.1
> > >>
> > >> --
> > >> 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
> >
>
> Argh... Sorry, that was entirely my fault. I traced the client side
> backchannel code, and found it was allocating slots using its own
> mechanism, then thought that applied to bc_tcp_ops.
>
> I find the NFSv4.1 backchannel code to be even more confusing than
> lockd.

Patches very much welcomed.

> ...and BTW the .rpcbind hack above is a prime example. Bruce, why do you
> need that? The server back channel sets xprt_set_bound() in
> xs_setup_bc_tcp() and should never clear it.

Beats me; you're suggesting the below? Agreed, looks wrong.

It must be pointless in the AF_LOCAL case too, though I didn't try to
verify.

--b.

commit ad25de5558f702fa2c7ececedf4d61975dababa8
Author: J. Bruce Fields <[email protected]>
Date: Mon Sep 24 15:53:29 2012 -0400

sunrpc: server back channel needs no rpcbind method

XPRT_BOUND is set on server backchannel xprts by xs_setup_bc_tcp()
(using xprt_set_bound()), and is never cleared, so ->rpcbind() will
never need to be called.

Reported-by: "Myklebust, Trond" <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index cd59a80..3a8663e6 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2529,7 +2529,6 @@ static struct rpc_xprt_ops bc_tcp_ops = {
.reserve_xprt = xprt_reserve_xprt,
.release_xprt = xprt_release_xprt,
.alloc_slot = xprt_alloc_slot,
- .rpcbind = xs_local_rpcbind,
.buf_alloc = bc_malloc,
.buf_free = bc_free,
.send_request = bc_send_request,

2012-09-24 19:31:41

by Myklebust, Trond

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

T24gTW9uLCAyMDEyLTA5LTI0IGF0IDEzOjUyIC0wNDAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDA5LzI0LzIwMTIgMDE6NDIgUE0sIEouIEJydWNlIEZpZWxkcyB3cm90ZToNCj4gPiBP
biBNb24sIFNlcCAyNCwgMjAxMiBhdCAwMTozOTowMVBNIC0wNDAwLCBianNjaHVtYUBuZXRhcHAu
Y29tIHdyb3RlOg0KPiA+PiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5j
b20+DQo+ID4+DQo+ID4+IGYzOWMxYmZiNWEwM2UyZDI1NTQ1MWJmZjA1YmUwZDcyNTUyOThmYTQg
KFNVTlJQQzogRml4IGEgVURQIHRyYW5zcG9ydA0KPiA+PiByZWdyZXNzaW9uKSBpbnRyb2R1Y2Vk
IHRoZSAiYWxsb2Nfc2xvdCIgZnVuY3Rpb24gZm9yIHhwcnQgb3BlcmF0aW9ucywNCj4gPj4gYnV0
IG5ldmVyIGNyZWF0ZWQgb25lIGZvciB0aGUgYmFja2NoYW5uZWwgb3BlcmF0aW9ucy4gIFRoaXMg
cGF0Y2ggZml4ZXMNCj4gPj4gYSBudWxsIHBvaW50ZXIgZGVyZWZlcmVuY2Ugd2hlbiBtb3VudGlu
ZyBORlMgb3ZlciB2NC4xLg0KPiA+IA0KPiA+IFRoYW5rcywgSSBqdXN0IHJlYmFzZWQgc29tZSBv
ZiBteSB3b3JrIHRvIDMuNiBhbmQgcmFuIGFjcm9zcyB0aGF0ISAgSXQNCj4gPiBjcmFzaGVzIHRo
ZSA0LjEgc2VydmVyIHZlcnkgcXVpY2tseS4uLi4NCj4gDQo+IFRoYXQgc291bmRzIGxpa2UgbXkg
c3RvcnkuICBJdCBnb3QgbXkgcGVlci10by1wZWVyIHNlcnZlciByaWdodCBhd2F5LCB0b28uDQo+
IA0KPiAtIEJyeWFuDQo+IA0KPiA+IA0KPiA+IC0tYi4NCj4gPiANCj4gPj4NCj4gPj4gQ2FsbCBU
cmFjZToNCj4gPj4gIFs8ZmZmZmZmZmZhMDIwNzk1Nz5dID8geHBydF9yZXNlcnZlKzB4NDcvMHg1
MCBbc3VucnBjXQ0KPiA+PiAgWzxmZmZmZmZmZmEwMjAyM2E0Pl0gY2FsbF9yZXNlcnZlKzB4MzQv
MHg2MCBbc3VucnBjXQ0KPiA+PiAgWzxmZmZmZmZmZmEwMjBlMjgwPl0gX19ycGNfZXhlY3V0ZSsw
eDkwLzB4NDAwIFtzdW5ycGNdDQo+ID4+ICBbPGZmZmZmZmZmYTAyMGU2MWE+XSBycGNfYXN5bmNf
c2NoZWR1bGUrMHgyYS8weDQwIFtzdW5ycGNdDQo+ID4+ICBbPGZmZmZmZmZmODEwNzM1ODk+XSBw
cm9jZXNzX29uZV93b3JrKzB4MTM5LzB4NTAwDQo+ID4+ICBbPGZmZmZmZmZmODEwNzBlNzA+XSA/
IGFsbG9jX3dvcmtlcisweDcwLzB4NzANCj4gPj4gIFs8ZmZmZmZmZmZhMDIwZTVmMD5dID8gX19y
cGNfZXhlY3V0ZSsweDQwMC8weDQwMCBbc3VucnBjXQ0KPiA+PiAgWzxmZmZmZmZmZjgxMDczZDFl
Pl0gd29ya2VyX3RocmVhZCsweDE1ZS8weDQ2MA0KPiA+PiAgWzxmZmZmZmZmZjgxNDVjODM5Pl0g
PyBwcmVlbXB0X3NjaGVkdWxlKzB4NDkvMHg3MA0KPiA+PiAgWzxmZmZmZmZmZjgxMDczYmMwPl0g
PyByZXNjdWVyX3RocmVhZCsweDIzMC8weDIzMA0KPiA+PiAgWzxmZmZmZmZmZjgxMDc5NjAzPl0g
a3RocmVhZCsweDkzLzB4YTANCj4gPj4gIFs8ZmZmZmZmZmY4MTQ2NWQwND5dIGtlcm5lbF90aHJl
YWRfaGVscGVyKzB4NC8weDEwDQo+ID4+ICBbPGZmZmZmZmZmODEwNzk1NzA+XSA/IGt0aHJlYWRf
ZnJlZXphYmxlX3Nob3VsZF9zdG9wKzB4NzAvMHg3MA0KPiA+PiAgWzxmZmZmZmZmZjgxNDY1ZDAw
Pl0gPyBnc19jaGFuZ2UrMHgxMy8weDEzDQo+ID4+DQo+ID4+IFNpZ25lZC1vZmYtYnk6IEJyeWFu
IFNjaHVtYWtlciA8YmpzY2h1bWFAbmV0YXBwLmNvbT4NCj4gPj4gLS0tDQo+ID4+ICBuZXQvc3Vu
cnBjL3hwcnRzb2NrLmMgfCAxICsNCj4gPj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigr
KQ0KPiA+Pg0KPiA+PiBkaWZmIC0tZ2l0IGEvbmV0L3N1bnJwYy94cHJ0c29jay5jIGIvbmV0L3N1
bnJwYy94cHJ0c29jay5jDQo+ID4+IGluZGV4IDg2Yjc3NzcuLmFhYWFkZmIgMTAwNjQ0DQo+ID4+
IC0tLSBhL25ldC9zdW5ycGMveHBydHNvY2suYw0KPiA+PiArKysgYi9uZXQvc3VucnBjL3hwcnRz
b2NrLmMNCj4gPj4gQEAgLTI1MjEsNiArMjUyMSw3IEBAIHN0YXRpYyBzdHJ1Y3QgcnBjX3hwcnRf
b3BzIHhzX3RjcF9vcHMgPSB7DQo+ID4+ICBzdGF0aWMgc3RydWN0IHJwY194cHJ0X29wcyBiY190
Y3Bfb3BzID0gew0KPiA+PiAgCS5yZXNlcnZlX3hwcnQJCT0geHBydF9yZXNlcnZlX3hwcnQsDQo+
ID4+ICAJLnJlbGVhc2VfeHBydAkJPSB4cHJ0X3JlbGVhc2VfeHBydCwNCj4gPj4gKwkuYWxsb2Nf
c2xvdAkJPSB4cHJ0X2FsbG9jX3Nsb3QsDQo+ID4+ICAJLnJwY2JpbmQJCT0geHNfbG9jYWxfcnBj
YmluZCwNCj4gPj4gIAkuYnVmX2FsbG9jCQk9IGJjX21hbGxvYywNCj4gPj4gIAkuYnVmX2ZyZWUJ
CT0gYmNfZnJlZSwNCj4gPj4gLS0gDQo+ID4+IDEuNy4xMi4xDQo+ID4+DQo+ID4+IC0tDQo+ID4+
IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmli
ZSBsaW51eC1uZnMiIGluDQo+ID4+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9A
dmdlci5rZXJuZWwub3JnDQo+ID4+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2Vy
Lmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KPiANCg0KQXJnaC4uLiBTb3JyeSwgdGhh
dCB3YXMgZW50aXJlbHkgbXkgZmF1bHQuIEkgdHJhY2VkIHRoZSBjbGllbnQgc2lkZQ0KYmFja2No
YW5uZWwgY29kZSwgYW5kIGZvdW5kIGl0IHdhcyBhbGxvY2F0aW5nIHNsb3RzIHVzaW5nIGl0cyBv
d24NCm1lY2hhbmlzbSwgdGhlbiB0aG91Z2h0IHRoYXQgYXBwbGllZCB0byBiY190Y3Bfb3BzLg0K
DQpJIGZpbmQgdGhlIE5GU3Y0LjEgYmFja2NoYW5uZWwgY29kZSB0byBiZSBldmVuIG1vcmUgY29u
ZnVzaW5nIHRoYW4NCmxvY2tkLg0KDQouLi5hbmQgQlRXIHRoZSAucnBjYmluZCBoYWNrIGFib3Zl
IGlzIGEgcHJpbWUgZXhhbXBsZS4gQnJ1Y2UsIHdoeSBkbyB5b3UNCm5lZWQgdGhhdD8gVGhlIHNl
cnZlciBiYWNrIGNoYW5uZWwgc2V0cyB4cHJ0X3NldF9ib3VuZCgpIGluDQp4c19zZXR1cF9iY190
Y3AoKSBhbmQgc2hvdWxkIG5ldmVyIGNsZWFyIGl0Lg0KDQotLSANClRyb25kIE15a2xlYnVzdA0K
TGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5l
dGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

2012-10-19 19:39:54

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

Did this not get sent to stable? (Now upstream as
84e28a307e376f271505af65a7b7e212dd6f61f4.)

--b.

On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
> From: Bryan Schumaker <[email protected]>
>
> f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP transport
> regression) introduced the "alloc_slot" function for xprt operations,
> but never created one for the backchannel operations. This patch fixes
> a null pointer dereference when mounting NFS over v4.1.
>
> Call Trace:
> [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> [<ffffffff81073589>] process_one_work+0x139/0x500
> [<ffffffff81070e70>] ? alloc_worker+0x70/0x70
> [<ffffffffa020e5f0>] ? __rpc_execute+0x400/0x400 [sunrpc]
> [<ffffffff81073d1e>] worker_thread+0x15e/0x460
> [<ffffffff8145c839>] ? preempt_schedule+0x49/0x70
> [<ffffffff81073bc0>] ? rescuer_thread+0x230/0x230
> [<ffffffff81079603>] kthread+0x93/0xa0
> [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> [<ffffffff81465d00>] ? gs_change+0x13/0x13
>
> Signed-off-by: Bryan Schumaker <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 86b7777..aaaadfb 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> static struct rpc_xprt_ops bc_tcp_ops = {
> .reserve_xprt = xprt_reserve_xprt,
> .release_xprt = xprt_release_xprt,
> + .alloc_slot = xprt_alloc_slot,
> .rpcbind = xs_local_rpcbind,
> .buf_alloc = bc_malloc,
> .buf_free = bc_free,
> --
> 1.7.12.1
>
> --
> 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

2012-10-19 21:01:29

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On Fri, Oct 19, 2012 at 08:23:05PM +0000, Myklebust, Trond wrote:
>
> Et tu Bruce?
>
> Not yet, because if you check, neither has commit f39c1bfb5a (SUNRPC: Fix a UDP transport regression). I figured it is better to send them both together as a single patch.

I'm confused, I thought I did check. Looking again... It's in 3.6:

$ git log v3.6..f39c1bfb5a
$

and cc'd to stable:

$ git show f39c1bfb5a|grep stable
Cc: [email protected] [>= 3.1]

so presumably it's already on its way to older stable branches too.

--b.

>
> Trond
>
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:[email protected]]
> > Sent: Friday, October 19, 2012 3:40 PM
> > To: Schumaker, Bryan
> > Cc: Myklebust, Trond; [email protected]; [email protected]
> > Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops
> >
> > Did this not get sent to stable? (Now upstream as
> > 84e28a307e376f271505af65a7b7e212dd6f61f4.)
> >
> > --b.
> >
> > On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
> > > From: Bryan Schumaker <[email protected]>
> > >
> > > f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP
> > transport
> > > regression) introduced the "alloc_slot" function for xprt operations,
> > > but never created one for the backchannel operations. This patch
> > > fixes a null pointer dereference when mounting NFS over v4.1.
> > >
> > > Call Trace:
> > > [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> > > [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> > > [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> > > [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> > > [<ffffffff81073589>] process_one_work+0x139/0x500
> > > [<ffffffff81070e70>] ? alloc_worker+0x70/0x70 [<ffffffffa020e5f0>] ?
> > > __rpc_execute+0x400/0x400 [sunrpc] [<ffffffff81073d1e>]
> > > worker_thread+0x15e/0x460 [<ffffffff8145c839>] ?
> > > preempt_schedule+0x49/0x70 [<ffffffff81073bc0>] ?
> > > rescuer_thread+0x230/0x230 [<ffffffff81079603>] kthread+0x93/0xa0
> > > [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> > > [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> > > [<ffffffff81465d00>] ? gs_change+0x13/0x13
> > >
> > > Signed-off-by: Bryan Schumaker <[email protected]>
> > > ---
> > > net/sunrpc/xprtsock.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index
> > > 86b7777..aaaadfb 100644
> > > --- a/net/sunrpc/xprtsock.c
> > > +++ b/net/sunrpc/xprtsock.c
> > > @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> > > static struct rpc_xprt_ops bc_tcp_ops = {
> > > .reserve_xprt = xprt_reserve_xprt,
> > > .release_xprt = xprt_release_xprt,
> > > + .alloc_slot = xprt_alloc_slot,
> > > .rpcbind = xs_local_rpcbind,
> > > .buf_alloc = bc_malloc,
> > > .buf_free = bc_free,
> > > --
> > > 1.7.12.1
> > >
> > > --
> > > 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

2012-10-19 21:05:10

by Myklebust, Trond

[permalink] [raw]

Subject: RE: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

> -----Original Message-----
> From: J. Bruce Fields [mailto:[email protected]]
> Sent: Friday, October 19, 2012 5:01 PM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; [email protected]; [email protected]
> Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops
>
> On Fri, Oct 19, 2012 at 08:23:05PM +0000, Myklebust, Trond wrote:
> >
> > Et tu Bruce?
> >
> > Not yet, because if you check, neither has commit f39c1bfb5a (SUNRPC: Fix
> a UDP transport regression). I figured it is better to send them both together
> as a single patch.
>
> I'm confused, I thought I did check. Looking again... It's in 3.6:
>
> $ git log v3.6..f39c1bfb5a
> $

Yes. 3.6 still has the original patch+problem.

> and cc'd to stable:
>
> $ git show f39c1bfb5a|grep stable
> Cc: [email protected] [>= 3.1]
>
> so presumably it's already on its way to older stable branches too.

No. See attachment...

> >
> > Trond
> >
> > > -----Original Message-----
> > > From: J. Bruce Fields [mailto:[email protected]]
> > > Sent: Friday, October 19, 2012 3:40 PM
> > > To: Schumaker, Bryan
> > > Cc: Myklebust, Trond; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops
> > >
> > > Did this not get sent to stable? (Now upstream as
> > > 84e28a307e376f271505af65a7b7e212dd6f61f4.)
> > >
> > > --b.
> > >
> > > On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected]
> wrote:
> > > > From: Bryan Schumaker <[email protected]>
> > > >
> > > > f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP
> > > transport
> > > > regression) introduced the "alloc_slot" function for xprt
> > > > operations, but never created one for the backchannel operations.
> > > > This patch fixes a null pointer dereference when mounting NFS over
> v4.1.
> > > >
> > > > Call Trace:
> > > > [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> > > > [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> > > > [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> > > > [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> > > > [<ffffffff81073589>] process_one_work+0x139/0x500
> > > > [<ffffffff81070e70>] ? alloc_worker+0x70/0x70 [<ffffffffa020e5f0>] ?
> > > > __rpc_execute+0x400/0x400 [sunrpc] [<ffffffff81073d1e>]
> > > > worker_thread+0x15e/0x460 [<ffffffff8145c839>] ?
> > > > preempt_schedule+0x49/0x70 [<ffffffff81073bc0>] ?
> > > > rescuer_thread+0x230/0x230 [<ffffffff81079603>] kthread+0x93/0xa0
> > > > [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> > > > [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> > > > [<ffffffff81465d00>] ? gs_change+0x13/0x13
> > > >
> > > > Signed-off-by: Bryan Schumaker <[email protected]>
> > > > ---
> > > > net/sunrpc/xprtsock.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index
> > > > 86b7777..aaaadfb 100644
> > > > --- a/net/sunrpc/xprtsock.c
> > > > +++ b/net/sunrpc/xprtsock.c
> > > > @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> > > > static struct rpc_xprt_ops bc_tcp_ops = {
> > > > .reserve_xprt = xprt_reserve_xprt,
> > > > .release_xprt = xprt_release_xprt,
> > > > + .alloc_slot = xprt_alloc_slot,
> > > > .rpcbind = xs_local_rpcbind,
> > > > .buf_alloc = bc_malloc,
> > > > .buf_free = bc_free,
> > > > --
> > > > 1.7.12.1
> > > >
> > > > --
> > > > 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

Attachments:

(No filename) (2.57 kB)

2012-10-19 20:23:08

by Myklebust, Trond

[permalink] [raw]

Subject: RE: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

Et tu Bruce?

Not yet, because if you check, neither has commit f39c1bfb5a (SUNRPC: Fix a UDP transport regression). I figured it is better to send them both together as a single patch.

Trond

> -----Original Message-----
> From: J. Bruce Fields [mailto:[email protected]]
> Sent: Friday, October 19, 2012 3:40 PM
> To: Schumaker, Bryan
> Cc: Myklebust, Trond; [email protected]; [email protected]
> Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops
>
> Did this not get sent to stable? (Now upstream as
> 84e28a307e376f271505af65a7b7e212dd6f61f4.)
>
> --b.
>
> On Mon, Sep 24, 2012 at 01:39:01PM -0400, [email protected] wrote:
> > From: Bryan Schumaker <[email protected]>
> >
> > f39c1bfb5a03e2d255451bff05be0d7255298fa4 (SUNRPC: Fix a UDP
> transport
> > regression) introduced the "alloc_slot" function for xprt operations,
> > but never created one for the backchannel operations. This patch
> > fixes a null pointer dereference when mounting NFS over v4.1.
> >
> > Call Trace:
> > [<ffffffffa0207957>] ? xprt_reserve+0x47/0x50 [sunrpc]
> > [<ffffffffa02023a4>] call_reserve+0x34/0x60 [sunrpc]
> > [<ffffffffa020e280>] __rpc_execute+0x90/0x400 [sunrpc]
> > [<ffffffffa020e61a>] rpc_async_schedule+0x2a/0x40 [sunrpc]
> > [<ffffffff81073589>] process_one_work+0x139/0x500
> > [<ffffffff81070e70>] ? alloc_worker+0x70/0x70 [<ffffffffa020e5f0>] ?
> > __rpc_execute+0x400/0x400 [sunrpc] [<ffffffff81073d1e>]
> > worker_thread+0x15e/0x460 [<ffffffff8145c839>] ?
> > preempt_schedule+0x49/0x70 [<ffffffff81073bc0>] ?
> > rescuer_thread+0x230/0x230 [<ffffffff81079603>] kthread+0x93/0xa0
> > [<ffffffff81465d04>] kernel_thread_helper+0x4/0x10
> > [<ffffffff81079570>] ? kthread_freezable_should_stop+0x70/0x70
> > [<ffffffff81465d00>] ? gs_change+0x13/0x13
> >
> > Signed-off-by: Bryan Schumaker <[email protected]>
> > ---
> > net/sunrpc/xprtsock.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index
> > 86b7777..aaaadfb 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2521,6 +2521,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> > static struct rpc_xprt_ops bc_tcp_ops = {
> > .reserve_xprt = xprt_reserve_xprt,
> > .release_xprt = xprt_release_xprt,
> > + .alloc_slot = xprt_alloc_slot,
> > .rpcbind = xs_local_rpcbind,
> > .buf_alloc = bc_malloc,
> > .buf_free = bc_free,
> > --
> > 1.7.12.1
> >
> > --
> > 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

2012-10-27 23:25:48

by Ben Hutchings

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On Fri, 2012-10-19 at 15:39 -0400, J. Bruce Fields wrote:
> Did this not get sent to stable? (Now upstream as
> 84e28a307e376f271505af65a7b7e212dd6f61f4.)
[...]

I've added this to the queue for 3.2.

Ben.

--
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2012-10-19 21:11:45

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops

On Fri, Oct 19, 2012 at 09:05:08PM +0000, Myklebust, Trond wrote:
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:[email protected]]
> > Sent: Friday, October 19, 2012 5:01 PM
> > To: Myklebust, Trond
> > Cc: Schumaker, Bryan; [email protected]; [email protected]
> > Subject: Re: [PATCH] SUNRPC: Set alloc_slot for backchannel tcp ops
> >
> > On Fri, Oct 19, 2012 at 08:23:05PM +0000, Myklebust, Trond wrote:
> > >
> > > Et tu Bruce?
> > >
> > > Not yet, because if you check, neither has commit f39c1bfb5a (SUNRPC: Fix
> > a UDP transport regression). I figured it is better to send them both together
> > as a single patch.
> >
> > I'm confused, I thought I did check. Looking again... It's in 3.6:
> >
> > $ git log v3.6..f39c1bfb5a
> > $
>
> Yes. 3.6 still has the original patch+problem.
>
> > and cc'd to stable:
> >
> > $ git show f39c1bfb5a|grep stable
> > Cc: [email protected] [>= 3.1]
> >
> > so presumably it's already on its way to older stable branches too.
>
> No. See attachment...

OK. As long as it makes it to 3.6.x one way or another, I'm happy....

--b.