[pnfs] [PATCH] nfsd: use list_del_init in unhash_stateowner

[J. Bruce Fields]

On Thu, Jan 24, 2008 at 02:47:37PM +0200, Benny Halevy wrote:
> 
> If release_stateowner is called more than once
> on the same nfs4_stateowner, unhash_stateowner crashes
> when calling list_del on a previously poisoned list_head.

I'm not necessarily opposed to changing these to list_del_init()'s just
out of general paranoia, and I'm not doubting the bug at all, but I
don't see how it's happening--I've stared at the code and just can't see
how release_stateowner() could be getting called twice on the same
stateowner.  As far as I can tell, every caller of release_stateowner()
gets the state lock, either creates the stateowner or finds it using
some data structure, then calls release_stateowner(), which removes the
stateowner from every structure that any such callers could use to find
it again.

--b.

> 
> For example:
> 
> Jan 24 14:17:17 bh-testlin1 kernel: Unable to handle kernel paging request at 0000000000200200 RIP:
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff810fb71b>] list_del+0xb/0x5b
> Jan 24 14:17:17 bh-testlin1 kernel: PGD 79879067 PUD 7995a067 PMD 0
> Jan 24 14:17:17 bh-testlin1 kernel: Oops: 0000 [1] SMP
> Jan 24 14:17:17 bh-testlin1 kernel: CPU 0
> Jan 24 14:17:17 bh-testlin1 kernel: Modules linked in: panfs(P) panlayoutdriver vmnet(P) parport_pc parport vmmon(P) nfsd auth_rpcgss exportfs autofs4 nfs lockd nfs_acl sunrpc ipv6 video output sbs sbshc battery ac forcedeth k8temp hwmoni2c_nforce2 pcspkr i2c_core button sr_mod cdrom pata_amd ata_generic sata_nv libata sd_mod scsi_mod ext3 jbd mbcache ehci_hcd ohci_hcd uhci_hcd
> Jan 24 14:17:17 bh-testlin1 kernel: Pid: 2144, comm: nfsd4 Tainted: P        2.6.24-rc8-panlayout #14
> Jan 24 14:17:17 bh-testlin1 kernel: RIP: 0010:[<ffffffff810fb71b>]  [<ffffffff810fb71b>] list_del+0xb/0x5b
> Jan 24 14:17:17 bh-testlin1 kernel: RSP: 0018:ffff81007c5e1e70  EFLAGS: 00010282
> Jan 24 14:17:17 bh-testlin1 kernel: RAX: ffff81004b9b7058 RBX: ffff81004b9b7000 RCX: 0000000000200200
> Jan 24 14:17:17 bh-testlin1 kernel: RDX: ffff81004b9b7000 RSI: ffff81004b9b7008 RDI: ffff81004b9b7008
> Jan 24 14:17:17 bh-testlin1 kernel: RBP: ffffffff8826b9f0 R08: ffff81007c5e0000 R09: ffff81007d44cbe8
> Jan 24 14:17:17 bh-testlin1 kernel: R10: ffffffffffffffff R11: 0000000100000000 R12: 000000000000000d
> Jan 24 14:17:17 bh-testlin1 kernel: R13: 00000000479881b9 R14: ffffffff81401240 R15: 0000000000000000
> Jan 24 14:17:17 bh-testlin1 kernel: FS:  00002ad035e7f6f0(0000) GS:ffffffff81371000(0000) knlGS:00000000f39d7b90
> Jan 24 14:17:17 bh-testlin1 kernel: CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> Jan 24 14:17:17 bh-testlin1 kernel: CR2: 0000000000200200 CR3: 0000000079947000 CR4: 00000000000006e0
> Jan 24 14:17:17 bh-testlin1 kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> Jan 24 14:17:17 bh-testlin1 kernel: DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Jan 24 14:17:17 bh-testlin1 kernel: Process nfsd4 (pid: 2144, threadinfo ffff81007c5e0000, task ffff81007d44c8c0)
> Jan 24 14:17:17 bh-testlin1 kernel: Stack:  ffff81007e438100 ffffffff88244db2 ffff81004b9b7000 ffffffff882458b6
> Jan 24 14:17:17 bh-testlin1 kernel:  ffff81007c5e1e90 ffff81007c5e1e90 0000000000000000 ffff81007e438100
> Jan 24 14:17:17 bh-testlin1 kernel:  ffffffff882456fa ffff81007d445ca8 ffffffffffffffff ffffffff81047f75
> Jan 24 14:17:17 bh-testlin1 kernel: Call Trace:
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff88244db2>] :nfsd:release_stateowner+0xd/0x82
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff882458b6>] :nfsd:laundromat_main+0x1bc/0x21c
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff882456fa>] :nfsd:laundromat_main+0x0/0x21c
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff81047f75>] run_workqueue+0x7f/0x10b
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8104887d>] worker_thread+0x0/0xe4
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff81048957>] worker_thread+0xda/0xe4
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8104b8a9>] autoremove_wake_function+0x0/0x2e
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8104b77a>] kthread+0x47/0x75
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8100ccd8>] child_rip+0xa/0x12
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8104b733>] kthread+0x0/0x75
> Jan 24 14:17:17 bh-testlin1 kernel:  [<ffffffff8100ccce>] child_rip+0x0/0x12
> Jan 24 14:17:17 bh-testlin1 kernel:
> Jan 24 14:17:17 bh-testlin1 kernel:
> Jan 24 14:17:17 bh-testlin1 kernel: Code: 48 8b 11 48 39 fa 74 12 48 c7 c7 d9 36 2f 81 31 c0 e8 f9 e5
> Jan 24 14:17:17 bh-testlin1 kernel: RIP  [<ffffffff810fb71b>] list_del+0xb/0x5b
> Jan 24 14:17:17 bh-testlin1 kernel:  RSP <ffff81007c5e1e70>
> Jan 24 14:17:17 bh-testlin1 kernel: CR2: 0000000000200200
> Jan 24 14:17:17 bh-testlin1 kernel: ---[ end trace a1a049631f2c4c32 ]---
> 
> Signed-off-by: Benny Halevy <bhalevy at panasas.com>
> ---
>  fs/nfsd/nfs4state.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 48a5322..df97a9e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1658,11 +1658,11 @@ unhash_stateowner(struct nfs4_stateowner *sop)
>  {
>  	struct nfs4_stateid *stp;
>  
> -	list_del(&sop->so_idhash);
> -	list_del(&sop->so_strhash);
> +	list_del_init(&sop->so_idhash);
> +	list_del_init(&sop->so_strhash);
>  	if (sop->so_is_open_owner)
> -		list_del(&sop->so_perclient);
> -	list_del(&sop->so_perstateid);
> +		list_del_init(&sop->so_perclient);
> +	list_del_init(&sop->so_perstateid);
>  	while (!list_empty(&sop->so_stateids)) {
>  		stp = list_entry(sop->so_stateids.next,
>  			struct nfs4_stateid, st_perstateowner);