Oops in NFSv4 server in 2.6.23.17

Oops in NFSv4 server in 2.6.23.17

[Lukas Hejtmanek]

Hello,

I'm running NFSv4 server (nfs-utils 1.1.1) with vanila kernel 2.6.23.17.
It works while I use the server from standard ethernet lines.

However, I tried to mount the volume over GPRS network. For the first time, it
replies premission denied and the second try results in the following oops.

Anything I should try?

I guess that the last oops is induced from the first one due to some memory 
corruption.

WARNING: at lib/kref.c:33 kref_get()

Call Trace:
 [<ffffffff803a1315>] kref_get+0x35/0x40
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0
 [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
 [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
 [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
 [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
 [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
 [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
 [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
 [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
 [<ffffffff804e8032>] __down_read+0x12/0xb0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
 [<ffffffff8020d068>] child_rip+0xa/0x12
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff8020d05e>] child_rip+0x0/0x12

WARNING: at lib/kref.c:33 kref_get()

Call Trace:
 [<ffffffff803a1315>] kref_get+0x35/0x40
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0
 [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
 [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
 [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
 [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
 [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
 [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
 [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
 [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
 [<ffffffff804e8032>] __down_read+0x12/0xb0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
 [<ffffffff8020d068>] child_rip+0xa/0x12
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff8020d05e>] child_rip+0x0/0x12

WARNING: at lib/kref.c:33 kref_get()

Call Trace:
 [<ffffffff803a1315>] kref_get+0x35/0x40
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0
 [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
 [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
 [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
 [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
 [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
 [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
 [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
 [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
 [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
 [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
 [<ffffffff804e8032>] __down_read+0x12/0xb0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
 [<ffffffff8020d068>] child_rip+0xa/0x12
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff8020d05e>] child_rip+0x0/0x12

general protection fault: 0000 [1] SMP 
CPU 0 
Modules linked in: des cbc blkcipher nfsd exportfs rpcsec_gss_krb5 auth_rpcgss nfs lockd nfs_acl sunrpc ipv6 dm_snapshot dm_mirror loop dm_round_robin dm_multipath dm_mod ata_piix ata_generic thermal mptfc libata processor scsi_transport_fc ehci_hcd e1000 evdev serio_raw psmouse uhci_hcd i2c_i801 i2c_core
Pid: 2863, comm: nfsd Not tainted 2.6.23.17 #1
RIP: 0010:[<ffffffff8819b113>]  [<ffffffff8819b113>] :auth_rpcgss:gss_get_mic+0x3/0x10
RSP: 0018:ffff81025c1d1c78  EFLAGS: 00010286
RAX: 762aba3d00000004 RBX: ffff81025b19f240 RCX: ffff81025c05c000
RDX: ffff81025c1d1cd0 RSI: ffff81025c1d1c80 RDI: ffff81025effa600
RBP: ffff81025c05c014 R08: 0000000000000000 R09: 0000000000000004
R10: 0000000000000000 R11: 0000000000000000 R12: ffff81025effa600
R13: ffff81025b596000 R14: ffff81025c05c018 R15: ffff81025b596130
FS:  00002b540c07b6d0(0000) GS:ffffffff805fe000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007fffa57e00e8 CR3: 000000025bb40000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process nfsd (pid: 2863, threadinfo ffff81025c1d0000, task ffff81025fe0c0c0)
Stack:  ffffffff8819c175 ffff81025c1d1cec 0000000000000004 0000000000000000
 0000000000000000 0000000000000000 0000000000000000 0000000400000004
 0000000000000000 ffff81025c1d1cec 0000000000000004 ffff81025effa600
Call Trace:
 [<ffffffff8819c175>] :auth_rpcgss:gss_write_verf+0xa5/0x130
 [<ffffffff8819d408>] :auth_rpcgss:svcauth_gss_accept+0x8b8/0xc30
 [<ffffffff88120c7f>] :sunrpc:svc_sock_enqueue+0x19f/0x340
 [<ffffffff88122531>] :sunrpc:svc_tcp_recvfrom+0x81/0x980
 [<ffffffff80242250>] del_timer_sync+0x10/0x20
 [<ffffffff804e7057>] schedule_timeout+0x67/0xd0
 [<ffffffff80242060>] process_timeout+0x0/0x10
 [<ffffffff804e704a>] schedule_timeout+0x5a/0xd0
 [<ffffffff8811fd78>] :sunrpc:svc_process+0x378/0x7d0
 [<ffffffff80231d60>] default_wake_function+0x0/0x10
 [<ffffffff804e8032>] __down_read+0x12/0xb0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
 [<ffffffff8020d068>] child_rip+0xa/0x12
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
 [<ffffffff8020d05e>] child_rip+0x0/0x12


Code: 48 8b 40 30 4c 8b 58 08 41 ff e3 66 90 48 8b 07 48 8b 40 30 
RIP  [<ffffffff8819b113>] :auth_rpcgss:gss_get_mic+0x3/0x10
 RSP <ffff81025c1d1c78>


kernel: invalid opcode: 0000 [2] SMP 
CPU 1 
Modules linked in: des cbc blkcipher nfsd exportfs rpcsec_gss_krb5 auth_rpcgss nfs lockd nfs_acl sunrpc ipv6 dm_snapshot dm_mirror loop dm_round_robin dm_multipath dm_mod ata_piix serio_raw mptfc psmouse scsi_transport_fc evdev ata_generic libata i2c_i801 e1000 i2c_core ehci_hcd uhci_hcd thermal processor
Pid: 2748, comm: multipathd Tainted: G      D 2.6.23.17 #1
RIP: 0010:[<ffffffff80291e9d>]  [<ffffffff80291e9d>] cache_alloc_refill+0x1ad/0x540
RSP: 0018:ffff810259aafe68  EFLAGS: 00010046
RAX: ffff81025e494040 RBX: 0000000000000020 RCX: 000000000000001c
RDX: ffff81025e494040 RSI: 000000000000000f RDI: ffff81025e810040
RBP: ffff81025fc6ec00 R08: ffff81025e810070 R09: 0000000000000000
R10: 0000000000159849 R11: 0000000000000000 R12: ffff81025fc00680
R13: ffff81025fc023c0 R14: 0000000000000000 R15: ffff81025fc0d000
FS:  0000000040061960(0063) GS:ffff81025fc6ce40(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00002ba20d8f4e30 CR3: 0000000259b10000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process multipathd (pid: 2748, threadinfo ffff810259aae000, task ffff81025fd06040)
Stack:  000000d000000010 ffffffff80291d6a 000000d0000080d0 ffff810259b19000
 0000000000000296 00000000000080d0 ffff81025fc00680 00007fff9d909298
 0000000040060e10 ffffffff80291cd2 0000000040062000 ffff810259b19000
Call Trace:
 [<ffffffff80291d6a>] cache_alloc_refill+0x7a/0x540
 [<ffffffff80291cd2>] kmem_cache_alloc+0x82/0xa0
 [<ffffffff8029bcf6>] do_execve+0x46/0x230
 [<ffffffff8020ae44>] sys_execve+0x44/0xb0
 [<ffffffff8020c617>] stub_execve+0x67/0xb0


Code: 0f 0b eb fe 49 8b 45 10 49 8d 55 10 48 89 78 08 48 89 07 48 
 RSP <ffff810259aafe68>


-- 
Lukáš Hejtmánek

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Wed, Mar 12, 2008 at 01:25:50PM +0100, Lukas Hejtmanek wrote:
> Hello,
> 
> I'm running NFSv4 server (nfs-utils 1.1.1) with vanila kernel 2.6.23.17.
> It works while I use the server from standard ethernet lines.
> 
> However, I tried to mount the volume over GPRS network. For the first time, it
> replies premission denied and the second try results in the following oops.
> 
> Anything I should try?
> 
> I guess that the last oops is induced from the first one due to some memory 
> corruption.
> 
> WARNING: at lib/kref.c:33 kref_get()

Hm.  So looks like that means kref_get() was called on something with a
zero reference count.

> 
> Call Trace:
>  [<ffffffff803a1315>] kref_get+0x35/0x40
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160

So sunrpc_cache_lookup found something with a zero reference count,
in...

>  [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0

... the has table that keeps the kernel's cached view of the export
table.

>  [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
>  [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
>  [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
>  [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
>  [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
>  [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
>  [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
>  [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
>  [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
>  [<ffffffff804e8032>] __down_read+0x12/0xb0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
>  [<ffffffff8020d068>] child_rip+0xa/0x12
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff8020d05e>] child_rip+0x0/0x12
> 
> WARNING: at lib/kref.c:33 kref_get()
> 
> Call Trace:
>  [<ffffffff803a1315>] kref_get+0x35/0x40
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
>  [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0
>  [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
>  [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
>  [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
>  [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
>  [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
>  [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
>  [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
>  [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
>  [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
>  [<ffffffff804e8032>] __down_read+0x12/0xb0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
>  [<ffffffff8020d068>] child_rip+0xa/0x12
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff8020d05e>] child_rip+0x0/0x12
> 
> WARNING: at lib/kref.c:33 kref_get()
> 
> Call Trace:
>  [<ffffffff803a1315>] kref_get+0x35/0x40
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
>  [<ffffffff881b5910>] :nfsd:svc_export_lookup+0xc0/0xd0
>  [<ffffffff881b594f>] :nfsd:exp_get_by_name+0x2f/0x80
>  [<ffffffff88128f34>] :sunrpc:sunrpc_cache_lookup+0x64/0x160
>  [<ffffffff88127b89>] :sunrpc:cache_check+0x49/0x490
>  [<ffffffff881b5fb7>] :nfsd:exp_find_key+0x57/0xc0
>  [<ffffffff881b6064>] :nfsd:exp_find+0x44/0xa0
>  [<ffffffff881b615e>] :nfsd:rqst_exp_find+0x9e/0xf0
>  [<ffffffff881b0c45>] :nfsd:fh_verify+0x425/0x540
>  [<ffffffff881bf25e>] :nfsd:nfsd4_proc_compound+0x1de/0x330
>  [<ffffffff881ad271>] :nfsd:nfsd_dispatch+0xb1/0x250
>  [<ffffffff8811fe91>] :sunrpc:svc_process+0x491/0x7d0
>  [<ffffffff804e8032>] __down_read+0x12/0xb0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
>  [<ffffffff8020d068>] child_rip+0xa/0x12
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff8020d05e>] child_rip+0x0/0x12
> 
> general protection fault: 0000 [1] SMP 
> CPU 0 
> Modules linked in: des cbc blkcipher nfsd exportfs rpcsec_gss_krb5 auth_rpcgss nfs lockd nfs_acl sunrpc ipv6 dm_snapshot dm_mirror loop dm_round_robin dm_multipath dm_mod ata_piix ata_generic thermal mptfc libata processor scsi_transport_fc ehci_hcd e1000 evdev serio_raw psmouse uhci_hcd i2c_i801 i2c_core
> Pid: 2863, comm: nfsd Not tainted 2.6.23.17 #1
> RIP: 0010:[<ffffffff8819b113>]  [<ffffffff8819b113>] :auth_rpcgss:gss_get_mic+0x3/0x10
> RSP: 0018:ffff81025c1d1c78  EFLAGS: 00010286
> RAX: 762aba3d00000004 RBX: ffff81025b19f240 RCX: ffff81025c05c000
> RDX: ffff81025c1d1cd0 RSI: ffff81025c1d1c80 RDI: ffff81025effa600
> RBP: ffff81025c05c014 R08: 0000000000000000 R09: 0000000000000004
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff81025effa600
> R13: ffff81025b596000 R14: ffff81025c05c018 R15: ffff81025b596130
> FS:  00002b540c07b6d0(0000) GS:ffffffff805fe000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00007fffa57e00e8 CR3: 000000025bb40000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process nfsd (pid: 2863, threadinfo ffff81025c1d0000, task ffff81025fe0c0c0)
> Stack:  ffffffff8819c175 ffff81025c1d1cec 0000000000000004 0000000000000000
>  0000000000000000 0000000000000000 0000000000000000 0000000400000004
>  0000000000000000 ffff81025c1d1cec 0000000000000004 ffff81025effa600
> Call Trace:
>  [<ffffffff8819c175>] :auth_rpcgss:gss_write_verf+0xa5/0x130

In this case it looks like something in the gss context cache is bad.

I'm not sure what's going on.

>From a quick

	gitk v2.6.23..origin fs/nfsd net/sunrpc/cache.c include/linux/sunrpc/cache.h

I don't see any recent commits that would address a problem like this.
But it might be worth trying to reproduce this in v2.6.25-rc5.

It'd also be interesting to know whether this is reproduceable with
auth_unix (as opposed to auth_gss) access, and/or with nfsv3 as opposed
to nfsv4.

--b.


>  [<ffffffff8819d408>] :auth_rpcgss:svcauth_gss_accept+0x8b8/0xc30
>  [<ffffffff88120c7f>] :sunrpc:svc_sock_enqueue+0x19f/0x340
>  [<ffffffff88122531>] :sunrpc:svc_tcp_recvfrom+0x81/0x980
>  [<ffffffff80242250>] del_timer_sync+0x10/0x20
>  [<ffffffff804e7057>] schedule_timeout+0x67/0xd0
>  [<ffffffff80242060>] process_timeout+0x0/0x10
>  [<ffffffff804e704a>] schedule_timeout+0x5a/0xd0
>  [<ffffffff8811fd78>] :sunrpc:svc_process+0x378/0x7d0
>  [<ffffffff80231d60>] default_wake_function+0x0/0x10
>  [<ffffffff804e8032>] __down_read+0x12/0xb0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad9b0>] :nfsd:nfsd+0x1a0/0x2f0
>  [<ffffffff8020d068>] child_rip+0xa/0x12
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff881ad810>] :nfsd:nfsd+0x0/0x2f0
>  [<ffffffff8020d05e>] child_rip+0x0/0x12
> 
> 
> Code: 48 8b 40 30 4c 8b 58 08 41 ff e3 66 90 48 8b 07 48 8b 40 30 
> RIP  [<ffffffff8819b113>] :auth_rpcgss:gss_get_mic+0x3/0x10
>  RSP <ffff81025c1d1c78>
> 
> 
> kernel: invalid opcode: 0000 [2] SMP 
> CPU 1 
> Modules linked in: des cbc blkcipher nfsd exportfs rpcsec_gss_krb5 auth_rpcgss nfs lockd nfs_acl sunrpc ipv6 dm_snapshot dm_mirror loop dm_round_robin dm_multipath dm_mod ata_piix serio_raw mptfc psmouse scsi_transport_fc evdev ata_generic libata i2c_i801 e1000 i2c_core ehci_hcd uhci_hcd thermal processor
> Pid: 2748, comm: multipathd Tainted: G      D 2.6.23.17 #1
> RIP: 0010:[<ffffffff80291e9d>]  [<ffffffff80291e9d>] cache_alloc_refill+0x1ad/0x540
> RSP: 0018:ffff810259aafe68  EFLAGS: 00010046
> RAX: ffff81025e494040 RBX: 0000000000000020 RCX: 000000000000001c
> RDX: ffff81025e494040 RSI: 000000000000000f RDI: ffff81025e810040
> RBP: ffff81025fc6ec00 R08: ffff81025e810070 R09: 0000000000000000
> R10: 0000000000159849 R11: 0000000000000000 R12: ffff81025fc00680
> R13: ffff81025fc023c0 R14: 0000000000000000 R15: ffff81025fc0d000
> FS:  0000000040061960(0063) GS:ffff81025fc6ce40(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00002ba20d8f4e30 CR3: 0000000259b10000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process multipathd (pid: 2748, threadinfo ffff810259aae000, task ffff81025fd06040)
> Stack:  000000d000000010 ffffffff80291d6a 000000d0000080d0 ffff810259b19000
>  0000000000000296 00000000000080d0 ffff81025fc00680 00007fff9d909298
>  0000000040060e10 ffffffff80291cd2 0000000040062000 ffff810259b19000
> Call Trace:
>  [<ffffffff80291d6a>] cache_alloc_refill+0x7a/0x540
>  [<ffffffff80291cd2>] kmem_cache_alloc+0x82/0xa0
>  [<ffffffff8029bcf6>] do_execve+0x46/0x230
>  [<ffffffff8020ae44>] sys_execve+0x44/0xb0
>  [<ffffffff8020c617>] stub_execve+0x67/0xb0
> 
> 
> Code: 0f 0b eb fe 49 8b 45 10 49 8d 55 10 48 89 78 08 48 89 07 48 
>  RSP <ffff810259aafe68>
> 
> 
> -- 
> Lukáš Hejtmánek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Oops in NFSv4 server in 2.6.23.17

[Lukas Hejtmanek]

Hello,

On Wed, Mar 12, 2008 at 12:00:07PM -0400, J. Bruce Fields wrote:
> In this case it looks like something in the gss context cache is bad.
> 
> I'm not sure what's going on.

I discovered what triggers this bug. If I have default export with secure
option and I try to connect from insecure port, it produces the oops.
If I add insecure option to the export, no oops at all and mount succeeds.

Also, the bug is not triggered by the first attempt but by the second one.

So I guess that from the first attempt there is something inserted into gss
context cache - error occurs (insecure attempt with the secure option), gss
context is removed from the cache as invalid but not completely. And the
second attempt does oops. Sounds reasonable for me. 

Hope this helps to find the bug.

-- 
Lukáš Hejtmánek

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Thu, Mar 13, 2008 at 03:36:31PM +0100, Lukas Hejtmanek wrote:
> Hello,
> 
> On Wed, Mar 12, 2008 at 12:00:07PM -0400, J. Bruce Fields wrote:
> > In this case it looks like something in the gss context cache is bad.
> > 
> > I'm not sure what's going on.
> 
> I discovered what triggers this bug. If I have default export with secure
> option and I try to connect from insecure port, it produces the oops.
> If I add insecure option to the export, no oops at all and mount succeeds.
> 
> Also, the bug is not triggered by the first attempt but by the second one.
> 
> So I guess that from the first attempt there is something inserted into gss
> context cache - error occurs (insecure attempt with the secure option), gss
> context is removed from the cache as invalid but not completely. And the
> second attempt does oops. Sounds reasonable for me. 
> 
> Hope this helps to find the bug.

That's an excellent clue, thanks.  I wonder it would be explainable by a
reference count imbalance of some kind in fh_verify?  I don't quite see
it yet, and unfortunately can't yet reproduce the bug here.

--b.

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Fri, Mar 14, 2008 at 02:14:13PM -0400, bfields wrote:
> On Thu, Mar 13, 2008 at 03:36:31PM +0100, Lukas Hejtmanek wrote:
> > Hello,
> > 
> > On Wed, Mar 12, 2008 at 12:00:07PM -0400, J. Bruce Fields wrote:
> > > In this case it looks like something in the gss context cache is bad.
> > > 
> > > I'm not sure what's going on.
> > 
> > I discovered what triggers this bug. If I have default export with secure
> > option and I try to connect from insecure port, it produces the oops.
> > If I add insecure option to the export, no oops at all and mount succeeds.
> > 
> > Also, the bug is not triggered by the first attempt but by the second one.
> > 
> > So I guess that from the first attempt there is something inserted into gss
> > context cache - error occurs (insecure attempt with the secure option), gss
> > context is removed from the cache as invalid but not completely. And the
> > second attempt does oops. Sounds reasonable for me. 
> > 
> > Hope this helps to find the bug.
> 
> That's an excellent clue, thanks.  I wonder it would be explainable by a
> reference count imbalance of some kind in fh_verify?

OK, yes, I think so.  Could you confirm whether this fixes it?

--b.

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 1eb771d..3e6b3f4 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -232,6 +232,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		fhp->fh_dentry = dentry;
 		fhp->fh_export = exp;
 		nfsd_nr_verified++;
+		cache_get(&exp->h);
 	} else {
 		/*
 		 * just rechecking permissions
@@ -241,6 +242,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		dprintk("nfsd: fh_verify - just checking\n");
 		dentry = fhp->fh_dentry;
 		exp = fhp->fh_export;
+		cache_get(&exp->h);
 		/*
 		 * Set user creds for this exportpoint; necessary even
 		 * in the "just checking" case because this may be a
@@ -252,8 +254,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		if (error)
 			goto out;
 	}
-	cache_get(&exp->h);
-
 
 	error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type);
 	if (error)

Re: Oops in NFSv4 server in 2.6.23.17

[Lukas Hejtmanek]

On Fri, Mar 14, 2008 at 03:33:50PM -0400, J. Bruce Fields wrote:
> OK, yes, I think so.  Could you confirm whether this fixes it?

I will test it on Monday as I cannot reboot the machine right now.

Just a quick review - isn't the cache_get() call needed also on the line 185 in
the same file?
 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 1eb771d..3e6b3f4 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -232,6 +232,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
>  		fhp->fh_dentry = dentry;
>  		fhp->fh_export = exp;
>  		nfsd_nr_verified++;
> +		cache_get(&exp->h);
>  	} else {
>  		/*
>  		 * just rechecking permissions
> @@ -241,6 +242,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
>  		dprintk("nfsd: fh_verify - just checking\n");
>  		dentry = fhp->fh_dentry;
>  		exp = fhp->fh_export;
> +		cache_get(&exp->h);
>  		/*
>  		 * Set user creds for this exportpoint; necessary even
>  		 * in the "just checking" case because this may be a
> @@ -252,8 +254,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
>  		if (error)
>  			goto out;
>  	}
> -	cache_get(&exp->h);
> -
>  
>  	error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type);
>  	if (error)

-- 
Lukáš Hejtmánek

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Fri, Mar 14, 2008 at 08:53:03PM +0100, Lukas Hejtmanek wrote:
> On Fri, Mar 14, 2008 at 03:33:50PM -0400, J. Bruce Fields wrote:
> > OK, yes, I think so.  Could you confirm whether this fixes it?
> 
> I will test it on Monday as I cannot reboot the machine right now.
> 
> Just a quick review - isn't the cache_get() call needed also on the line 185 in
> the same file?

Right before the first nfsd_setuser_and_check_port()?  I don't believe
so.

The way it works is: the rqst_exp_find() calls bump the reference count
on the export they return to us, as expected; so if we bail out after
that line 185, the exp_put() at "out:" drops that reference, as it
should, making fh_verify a no-op with respect to the reference count.

The only time we need a new reference is when we store that pointer in
the filehandle, around line 233, as that's what creates a long-lived
reference that will outlive the function.

The other cache_get() (in the "just rechecking" case) is there just to
balance out the final exp_put() so every code path can share the same
code at "out:".

I find that a little contorted.  So I'll go ahead and submit this small
patch to 2.6.25 and stable now (I have since managed to reproduce what I
believe is your bug, though my symptoms were a little different), and
then submit to 2.6.26 some cleanup which makes this more understandable,
and brings fh_verify() a little closer to the kernel's aesthetic of
small, minimally-indented functions.

That said, I'd definitely still appreciate your confirmation that this
fixes your bug, so thanks for offering to retest that Monday.

--b.

>  
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 1eb771d..3e6b3f4 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -232,6 +232,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
> >  		fhp->fh_dentry = dentry;
> >  		fhp->fh_export = exp;
> >  		nfsd_nr_verified++;
> > +		cache_get(&exp->h);
> >  	} else {
> >  		/*
> >  		 * just rechecking permissions
> > @@ -241,6 +242,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
> >  		dprintk("nfsd: fh_verify - just checking\n");
> >  		dentry = fhp->fh_dentry;
> >  		exp = fhp->fh_export;
> > +		cache_get(&exp->h);
> >  		/*
> >  		 * Set user creds for this exportpoint; necessary even
> >  		 * in the "just checking" case because this may be a
> > @@ -252,8 +254,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
> >  		if (error)
> >  			goto out;
> >  	}
> > -	cache_get(&exp->h);
> > -
> >  
> >  	error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type);
> >  	if (error)
> 
> -- 
> Lukáš Hejtmánek

[PATCH] nfsd: fix oops on access from high-numbered ports

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

This bug was always here, but before my commit 6fa02839bf9412e18e77
("recheck for secure ports in fh_verify"), it could only be triggered by
failure of a kmalloc().  After that commit it could be triggered by a
client making a request from a non-reserved port for access to an export
marked "secure".  (Exports are "secure" by default.)

The result is a struct svc_export with a reference count one to low,
resulting in likely oopses next time the export is accessed.

The reference counting here is not straightforward; a later patch will
clean up fh_verify().

Thanks to Lukas Hejtmanek for the bug report and followup.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Cc: Lukas Hejtmanek <xhejtman@ics.muni.cz>
---
 fs/nfsd/nfsfh.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

This is appropriate for 2.6.25, 2.6.24.y, and 2.6.23.y (if it's still
maintained).--b.

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 1eb771d..3e6b3f4 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -232,6 +232,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		fhp->fh_dentry = dentry;
 		fhp->fh_export = exp;
 		nfsd_nr_verified++;
+		cache_get(&exp->h);
 	} else {
 		/*
 		 * just rechecking permissions
@@ -241,6 +242,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		dprintk("nfsd: fh_verify - just checking\n");
 		dentry = fhp->fh_dentry;
 		exp = fhp->fh_export;
+		cache_get(&exp->h);
 		/*
 		 * Set user creds for this exportpoint; necessary even
 		 * in the "just checking" case because this may be a
@@ -252,8 +254,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		if (error)
 			goto out;
 	}
-	cache_get(&exp->h);
-
 
 	error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type);
 	if (error)
-- 
1.5.4.rc2.60.gb2e62

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Fri, Mar 14, 2008 at 04:05:10PM -0400, bfields wrote:
> I find that a little contorted.  So I'll go ahead and submit this small
> patch to 2.6.25 and stable now (I have since managed to reproduce what I
> believe is your bug, though my symptoms were a little different), and
> then submit to 2.6.26 some cleanup which makes this more understandable,

Here's an attempt.  We could break up fh_verify even more, though.--b.

>From ce83cbd34a40153aed8bf559f24576a06e3e378a Mon Sep 17 00:00:00 2001
From: J. Bruce Fields <bfields@citi.umich.edu>
Date: Fri, 14 Mar 2008 17:51:12 -0400
Subject: [PATCH] nfsd: move most of fh_verify to separate function

Move the code that actually parses the filehandle and looks up the
dentry and export to a separate function.  This simplifies the reference
counting a little and moves fh_verify() a little closer to the kernel
ideal of small, minimally-indentended functions.  Clean up a few other
minor style sins along the way.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfsfh.c |  223 +++++++++++++++++++++++++++++--------------------------
 1 files changed, 118 insertions(+), 105 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 3e6b3f4..fc1d98b 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -112,6 +112,119 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
 	return nfserrno(nfsd_setuser(rqstp, exp));
 }
 
+static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
+{
+	struct knfsd_fh	*fh = &fhp->fh_handle;
+	struct fid *fid = NULL, sfid;
+	struct svc_export *exp;
+	struct dentry *dentry;
+	int fileid_type;
+	int data_left = fh->fh_size/4;
+	__be32 error;
+
+	error = nfserr_stale;
+	if (rqstp->rq_vers > 2)
+		error = nfserr_badhandle;
+	if (rqstp->rq_vers == 4 && fh->fh_size == 0)
+		return nfserr_nofilehandle;
+
+	if (fh->fh_version == 1) {
+		int len;
+
+		if (--data_left < 0)
+			return error;
+		if (fh->fh_auth_type != 0)
+			return error;
+		len = key_len(fh->fh_fsid_type) / 4;
+		if (len == 0)
+			return error;
+		if  (fh->fh_fsid_type == FSID_MAJOR_MINOR) {
+			/* deprecated, convert to type 3 */
+			len = key_len(FSID_ENCODE_DEV)/4;
+			fh->fh_fsid_type = FSID_ENCODE_DEV;
+			fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl(fh->fh_fsid[0]), ntohl(fh->fh_fsid[1])));
+			fh->fh_fsid[1] = fh->fh_fsid[2];
+		}
+		data_left -= len;
+		if (data_left < 0)
+			return error;
+		exp = rqst_exp_find(rqstp, fh->fh_fsid_type, fh->fh_auth);
+		fid = (struct fid *)(fh->fh_auth + len);
+	} else {
+		__u32 tfh[2];
+		dev_t xdev;
+		ino_t xino;
+
+		if (fh->fh_size != NFS_FHSIZE)
+			return error;
+		/* assume old filehandle format */
+		xdev = old_decode_dev(fh->ofh_xdev);
+		xino = u32_to_ino_t(fh->ofh_xino);
+		mk_fsid(FSID_DEV, tfh, xdev, xino, 0, NULL);
+		exp = rqst_exp_find(rqstp, FSID_DEV, tfh);
+	}
+
+	error = nfserr_stale;
+	if (PTR_ERR(exp) == -ENOENT)
+		return error;
+
+	if (IS_ERR(exp))
+		return nfserrno(PTR_ERR(exp));
+
+	error = nfsd_setuser_and_check_port(rqstp, exp);
+	if (error)
+		goto out;
+
+	/*
+	 * Look up the dentry using the NFS file handle.
+	 */
+	error = nfserr_stale;
+	if (rqstp->rq_vers > 2)
+		error = nfserr_badhandle;
+
+	if (fh->fh_version != 1) {
+		sfid.i32.ino = fh->ofh_ino;
+		sfid.i32.gen = fh->ofh_generation;
+		sfid.i32.parent_ino = fh->ofh_dirino;
+		fid = &sfid;
+		data_left = 3;
+		if (fh->ofh_dirino == 0)
+			fileid_type = FILEID_INO32_GEN;
+		else
+			fileid_type = FILEID_INO32_GEN_PARENT;
+	} else
+		fileid_type = fh->fh_fileid_type;
+
+	if (fileid_type == FILEID_ROOT)
+		dentry = dget(exp->ex_path.dentry);
+	else {
+		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
+				data_left, fileid_type,
+				nfsd_acceptable, exp);
+	}
+	if (dentry == NULL)
+		goto out;
+	if (IS_ERR(dentry)) {
+		if (PTR_ERR(dentry) != -EINVAL)
+			error = nfserrno(PTR_ERR(dentry));
+		goto out;
+	}
+
+	if (S_ISDIR(dentry->d_inode->i_mode) &&
+			(dentry->d_flags & DCACHE_DISCONNECTED)) {
+		printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n",
+				dentry->d_parent->d_name.name, dentry->d_name.name);
+	}
+
+	fhp->fh_dentry = dentry;
+	fhp->fh_export = exp;
+	nfsd_nr_verified++;
+	return 0;
+out:
+	exp_put(exp);
+	return error;
+}
+
 /*
  * Perform sanity checks on the dentry in a client's file handle.
  *
@@ -124,115 +237,18 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
 __be32
 fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 {
-	struct knfsd_fh	*fh = &fhp->fh_handle;
-	struct svc_export *exp = NULL;
+	struct svc_export *exp;
 	struct dentry	*dentry;
-	__be32		error = 0;
+	__be32		error;
 
 	dprintk("nfsd: fh_verify(%s)\n", SVCFH_fmt(fhp));
 
 	if (!fhp->fh_dentry) {
-		struct fid *fid = NULL, sfid;
-		int fileid_type;
-		int data_left = fh->fh_size/4;
-
-		error = nfserr_stale;
-		if (rqstp->rq_vers > 2)
-			error = nfserr_badhandle;
-		if (rqstp->rq_vers == 4 && fh->fh_size == 0)
-			return nfserr_nofilehandle;
-
-		if (fh->fh_version == 1) {
-			int len;
-			if (--data_left<0) goto out;
-			switch (fh->fh_auth_type) {
-			case 0: break;
-			default: goto out;
-			}
-			len = key_len(fh->fh_fsid_type) / 4;
-			if (len == 0) goto out;
-			if  (fh->fh_fsid_type == FSID_MAJOR_MINOR) {
-				/* deprecated, convert to type 3 */
-				len = key_len(FSID_ENCODE_DEV)/4;
-				fh->fh_fsid_type = FSID_ENCODE_DEV;
-				fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl(fh->fh_fsid[0]), ntohl(fh->fh_fsid[1])));
-				fh->fh_fsid[1] = fh->fh_fsid[2];
-			}
-			if ((data_left -= len)<0) goto out;
-			exp = rqst_exp_find(rqstp, fh->fh_fsid_type,
-					    fh->fh_auth);
-			fid = (struct fid *)(fh->fh_auth + len);
-		} else {
-			__u32 tfh[2];
-			dev_t xdev;
-			ino_t xino;
-			if (fh->fh_size != NFS_FHSIZE)
-				goto out;
-			/* assume old filehandle format */
-			xdev = old_decode_dev(fh->ofh_xdev);
-			xino = u32_to_ino_t(fh->ofh_xino);
-			mk_fsid(FSID_DEV, tfh, xdev, xino, 0, NULL);
-			exp = rqst_exp_find(rqstp, FSID_DEV, tfh);
-		}
-
-		error = nfserr_stale;
-		if (PTR_ERR(exp) == -ENOENT)
-			goto out;
-
-		if (IS_ERR(exp)) {
-			error = nfserrno(PTR_ERR(exp));
-			goto out;
-		}
-
-		error = nfsd_setuser_and_check_port(rqstp, exp);
+		error = nfsd_set_fh_dentry(rqstp, fhp);
 		if (error)
 			goto out;
-
-		/*
-		 * Look up the dentry using the NFS file handle.
-		 */
-		error = nfserr_stale;
-		if (rqstp->rq_vers > 2)
-			error = nfserr_badhandle;
-
-		if (fh->fh_version != 1) {
-			sfid.i32.ino = fh->ofh_ino;
-			sfid.i32.gen = fh->ofh_generation;
-			sfid.i32.parent_ino = fh->ofh_dirino;
-			fid = &sfid;
-			data_left = 3;
-			if (fh->ofh_dirino == 0)
-				fileid_type = FILEID_INO32_GEN;
-			else
-				fileid_type = FILEID_INO32_GEN_PARENT;
-		} else
-			fileid_type = fh->fh_fileid_type;
-
-		if (fileid_type == FILEID_ROOT)
-			dentry = dget(exp->ex_path.dentry);
-		else {
-			dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
-					data_left, fileid_type,
-					nfsd_acceptable, exp);
-		}
-		if (dentry == NULL)
-			goto out;
-		if (IS_ERR(dentry)) {
-			if (PTR_ERR(dentry) != -EINVAL)
-				error = nfserrno(PTR_ERR(dentry));
-			goto out;
-		}
-
-		if (S_ISDIR(dentry->d_inode->i_mode) &&
-		    (dentry->d_flags & DCACHE_DISCONNECTED)) {
-			printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n",
-			       dentry->d_parent->d_name.name, dentry->d_name.name);
-		}
-
-		fhp->fh_dentry = dentry;
-		fhp->fh_export = exp;
-		nfsd_nr_verified++;
-		cache_get(&exp->h);
+		dentry = fhp->fh_dentry;
+		exp = fhp->fh_export;
 	} else {
 		/*
 		 * just rechecking permissions
@@ -242,7 +258,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		dprintk("nfsd: fh_verify - just checking\n");
 		dentry = fhp->fh_dentry;
 		exp = fhp->fh_export;
-		cache_get(&exp->h);
 		/*
 		 * Set user creds for this exportpoint; necessary even
 		 * in the "just checking" case because this may be a
@@ -281,8 +296,6 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 			access, ntohl(error));
 	}
 out:
-	if (exp && !IS_ERR(exp))
-		exp_put(exp);
 	if (error == nfserr_stale)
 		nfsdstats.fh_stale++;
 	return error;
-- 
1.5.4.rc2.60.gb2e62

Re: Oops in NFSv4 server in 2.6.23.17

[Neil Brown]

On Friday March 14, bfields@fieldses.org wrote:
> On Fri, Mar 14, 2008 at 04:05:10PM -0400, bfields wrote:
> > I find that a little contorted.  So I'll go ahead and submit this small
> > patch to 2.6.25 and stable now (I have since managed to reproduce what I
> > believe is your bug, though my symptoms were a little different), and
> > then submit to 2.6.26 some cleanup which makes this more understandable,
> 
> Here's an attempt.  We could break up fh_verify even more, though.--b.

Looks like a good attempt.

My only suggestion would be to put a comment at the top of
nfsd_set_fh_dentry explaining what it does and who calls it.

It's long past time that code had some spring cleaning !!

Thanks,
NeilBrown

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Mon, Mar 17, 2008 at 10:19:20AM +1100, Neil Brown wrote:
> On Friday March 14, bfields@fieldses.org wrote:
> > On Fri, Mar 14, 2008 at 04:05:10PM -0400, bfields wrote:
> > > I find that a little contorted.  So I'll go ahead and submit this small
> > > patch to 2.6.25 and stable now (I have since managed to reproduce what I
> > > believe is your bug, though my symptoms were a little different), and
> > > then submit to 2.6.26 some cleanup which makes this more understandable,
> > 
> > Here's an attempt.  We could break up fh_verify even more, though.--b.
> 
> Looks like a good attempt.
> 
> My only suggestion would be to put a comment at the top of
> nfsd_set_fh_dentry explaining what it does and who calls it.

OK!  I'm planning to just add:

+/*
+ * Use the given filehandle to look up the corresponding export and
+ * dentry.  On success, the results are used to set fh_export and
+ * fh_dentry.
+ */
 static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 {
 	struct knfsd_fh	*fh = &fhp->fh_handle;

(Nothing about "who calls it", but it's static and its only caller is
fh_verify, so that seemed uninteresting.)  Anything else you were
looking for?

> 
> It's long past time that code had some spring cleaning !!

If I had a little more time I think it might be clearer to make this:

	fhp->fh_export = nfsd_fh_get_export(rqstp, &fhp->fh_handle);
	if (IS_ERR(fhp->fh_export))
		return ERR_PTR(fhp->fh_export);
	error = nfsd_setuser_and_check_port(rqstp, exp);
	if (error)
		goto out;
	fhp->fh_export = nfsd_fh_get_dentry(rqstp, &fhp->fh_handle);
	if (IS_ERR(...))

etc., though that's probably not quite right.

--b.

Re: Oops in NFSv4 server in 2.6.23.17

[NeilBrown]

On Thu, March 20, 2008 9:32 am, J. Bruce Fields wrote:
> On Mon, Mar 17, 2008 at 10:19:20AM +1100, Neil Brown wrote:
>> On Friday March 14, bfields@fieldses.org wrote:
>> > On Fri, Mar 14, 2008 at 04:05:10PM -0400, bfields wrote:
>> > > I find that a little contorted.  So I'll go ahead and submit this
>> small
>> > > patch to 2.6.25 and stable now (I have since managed to reproduce
>> what I
>> > > believe is your bug, though my symptoms were a little different),
>> and
>> > > then submit to 2.6.26 some cleanup which makes this more
>> understandable,
>> >
>> > Here's an attempt.  We could break up fh_verify even more, though.--b.
>>
>> Looks like a good attempt.
>>
>> My only suggestion would be to put a comment at the top of
>> nfsd_set_fh_dentry explaining what it does and who calls it.
>
> OK!  I'm planning to just add:
>
> +/*
> + * Use the given filehandle to look up the corresponding export and
> + * dentry.  On success, the results are used to set fh_export and
> + * fh_dentry.
> + */
>  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh
> *fhp)
>  {
>  	struct knfsd_fh	*fh = &fhp->fh_handle;
>
> (Nothing about "who calls it", but it's static and its only caller is
> fh_verify, so that seemed uninteresting.)  Anything else you were
> looking for?

No, that's adequate.
The function name has one verb and 2 (or 3) nouns,  and it isn't clear
how the verb relates to the nouns.  The comment you gave makes that
clear.

Thanks,
NeilBrown

Re: Oops in NFSv4 server in 2.6.23.17

[Lukas Hejtmanek]

Hello,

On Fri, Mar 14, 2008 at 04:05:10PM -0400, J. Bruce Fields wrote:
> That said, I'd definitely still appreciate your confirmation that this
> fixes your bug, so thanks for offering to retest that Monday.

seems to be OK even from the insecure port. Thanks.

-- 
Lukáš Hejtmánek

Re: Oops in NFSv4 server in 2.6.23.17

[J. Bruce Fields]

On Mon, Mar 17, 2008 at 09:12:59AM +0100, Lukas Hejtmanek wrote:
> Hello,
> 
> On Fri, Mar 14, 2008 at 04:05:10PM -0400, J. Bruce Fields wrote:
> > That said, I'd definitely still appreciate your confirmation that this
> > fixes your bug, so thanks for offering to retest that Monday.
> 
> seems to be OK even from the insecure port. Thanks.

Thanks for the confirmation.--b.