_nfs4_opendata_reclaim_to_nfs4_state doesn't expect to see a cached
open CLAIM_PREVIOUS, but this can happen. An example is when there are
RDWR openers and RDONLY openers on a delegation stateid. The recovery
path will first try an open CLAIM_PREVIOUS for the RDWR openers, this
marks the delegation as not needing RECLAIM anymore, so the open
CLAIM_PREVIOUS for the RDONLY openers will not actually send an rpc.
The NULL dereference is due to _nfs4_opendata_reclaim_to_nfs4_state
returning PTR_ERR(rpc_status) when !rpc_done. When the open is
cached, rpc_done == 0 and rpc_status == 0, thus
_nfs4_opendata_reclaim_to_nfs4_state returns NULL - this is unexpected
by callers of nfs4_opendata_to_nfs4_state().
This can be reproduced easily by opening the same file two times on an
NFSv4.0 mount with delegations enabled, once as RDONLY and once as RDWR then
sleeping for a long time. While the files are held open, kick off state
recovery and this NULL dereference will be hit every time.
An example OOPS:
[ 65.003602] BUG: unable to handle kernel NULL pointer dereference at 00000000
00000030
[ 65.005312] IP: [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[ 65.006820] PGD 7b0ea067 PUD 791ff067 PMD 0
[ 65.008075] Oops: 0000 [#1] SMP
[ 65.008802] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
snd_ens1371 gameport nfsd snd_rawmidi snd_ac97_codec ac97_bus btusb snd_seq snd
_seq_device snd_pcm ppdev bluetooth auth_rpcgss coretemp snd_page_alloc crc32_pc
lmul crc32c_intel ghash_clmulni_intel microcode rfkill nfs_acl vmw_balloon serio
_raw snd_timer lockd parport_pc e1000 snd soundcore parport i2c_piix4 shpchp vmw
_vmci sunrpc ata_generic mperf pata_acpi mptspi vmwgfx ttm scsi_transport_spi dr
m mptscsih mptbase i2c_core
[ 65.018684] CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
.x86_64 #1
[ 65.020113] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
Reference Platform, BIOS 6.00 07/31/2013
[ 65.022012] task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
[ 65.023414] RIP: 0010:[<ffffffffa037d6ee>] [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[ 65.025079] RSP: 0018:ffff88007b907d10 EFLAGS: 00010246
[ 65.026042] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[ 65.027321] RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
[ 65.028691] RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
[ 65.029990] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
[ 65.031295] R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
[ 65.032527] FS: 0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
[ 65.033981] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 65.035177] CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
[ 65.036568] Stack:
[ 65.037011] 0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
[ 65.038472] ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
[ 65.039935] ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
[ 65.041468] Call Trace:
[ 65.042050] [<ffffffffa037e4a5>] nfs4_close_state+0x15/0x20 [nfsv4]
[ 65.043209] [<ffffffffa036a6c8>] nfs4_open_recover_helper+0x148/0x1f0 [nfsv4]
[ 65.044529] [<ffffffffa036a886>] nfs4_open_recover+0x116/0x150 [nfsv4]
[ 65.045730] [<ffffffffa036d98d>] nfs4_open_reclaim+0xad/0x150 [nfsv4]
[ 65.046905] [<ffffffffa037d979>] nfs4_do_reclaim+0x149/0x5f0 [nfsv4]
[ 65.048071] [<ffffffffa037e1dc>] nfs4_run_state_manager+0x3bc/0x670 [nfsv4]
[ 65.049436] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
[ 65.050686] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
[ 65.051943] [<ffffffff81088640>] kthread+0xc0/0xd0
[ 65.052831] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
[ 65.054697] [<ffffffff8165686c>] ret_from_fork+0x7c/0xb0
[ 65.056396] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
[ 65.058208] Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb <4c> 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
[ 65.065225] RIP [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[ 65.067175] RSP <ffff88007b907d10>
[ 65.068570] CR2: 0000000000000030
[ 65.070098] ---[ end trace 0d1fe4f5c7dd6f8b ]---
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d2b4845..58658db 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1316,16 +1316,27 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
struct inode *inode = data->state->inode;
struct nfs4_state *state = data->state;
int ret;
+ bool cached_open = false;
if (!data->rpc_done) {
- ret = data->rpc_status;
- goto err;
+ if (data->rpc_status) {
+ ret = data->rpc_status;
+ goto err;
+ } else
+ /*
+ * This is a cached open from an earlier operation
+ * of the current state recovery event so there is
+ * no need to update or check anything, just lookup
+ * the open state.
+ */
+ cached_open = true;
}
ret = -ESTALE;
- if (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
- !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
- !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE))
+ if (!cached_open &&
+ (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
+ !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
+ !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE)))
goto err;
ret = -ENOMEM;
@@ -1333,6 +1344,9 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
if (state == NULL)
goto err;
+ if (cached_open)
+ goto out;
+
ret = nfs_refresh_inode(inode, &data->f_attr);
if (ret)
goto err;
@@ -1344,6 +1358,7 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
update_open_stateid(state, &data->o_res.stateid, NULL,
data->o_arg.fmode);
+out:
return state;
err:
return ERR_PTR(ret);
--
1.7.12.4 (Apple Git-37)
On Tue, Oct 8, 2013 at 1:40 AM, Weston Andros Adamson <[email protected]> wrote:
> This is the bug I found at the bakeathon. I can finally reproduce it quickly! And in a shockingly easy way!
> Tomorrow I'll figure out how long this regression has been present and if we need to CC stable.
Any news on that? Does it apply on older version?
Thanks,
--
William
This is the bug I found at the bakeathon. I can finally reproduce it quickly! And in a shockingly easy way!
Tomorrow I'll figure out how long this regression has been present and if we need to CC stable.
-dros
On Oct 7, 2013, at 7:33 PM, Weston Andros Adamson <[email protected]>
wrote:
> _nfs4_opendata_reclaim_to_nfs4_state doesn't expect to see a cached
> open CLAIM_PREVIOUS, but this can happen. An example is when there are
> RDWR openers and RDONLY openers on a delegation stateid. The recovery
> path will first try an open CLAIM_PREVIOUS for the RDWR openers, this
> marks the delegation as not needing RECLAIM anymore, so the open
> CLAIM_PREVIOUS for the RDONLY openers will not actually send an rpc.
>
> The NULL dereference is due to _nfs4_opendata_reclaim_to_nfs4_state
> returning PTR_ERR(rpc_status) when !rpc_done. When the open is
> cached, rpc_done == 0 and rpc_status == 0, thus
> _nfs4_opendata_reclaim_to_nfs4_state returns NULL - this is unexpected
> by callers of nfs4_opendata_to_nfs4_state().
>
> This can be reproduced easily by opening the same file two times on an
> NFSv4.0 mount with delegations enabled, once as RDONLY and once as RDWR then
> sleeping for a long time. While the files are held open, kick off state
> recovery and this NULL dereference will be hit every time.
>
> An example OOPS:
>
> [ 65.003602] BUG: unable to handle kernel NULL pointer dereference at 00000000
> 00000030
> [ 65.005312] IP: [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
> [ 65.006820] PGD 7b0ea067 PUD 791ff067 PMD 0
> [ 65.008075] Oops: 0000 [#1] SMP
> [ 65.008802] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
> snd_ens1371 gameport nfsd snd_rawmidi snd_ac97_codec ac97_bus btusb snd_seq snd
> _seq_device snd_pcm ppdev bluetooth auth_rpcgss coretemp snd_page_alloc crc32_pc
> lmul crc32c_intel ghash_clmulni_intel microcode rfkill nfs_acl vmw_balloon serio
> _raw snd_timer lockd parport_pc e1000 snd soundcore parport i2c_piix4 shpchp vmw
> _vmci sunrpc ata_generic mperf pata_acpi mptspi vmwgfx ttm scsi_transport_spi dr
> m mptscsih mptbase i2c_core
> [ 65.018684] CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
> .x86_64 #1
> [ 65.020113] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
> Reference Platform, BIOS 6.00 07/31/2013
> [ 65.022012] task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
> [ 65.023414] RIP: 0010:[<ffffffffa037d6ee>] [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
> [ 65.025079] RSP: 0018:ffff88007b907d10 EFLAGS: 00010246
> [ 65.026042] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [ 65.027321] RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
> [ 65.028691] RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
> [ 65.029990] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
> [ 65.031295] R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
> [ 65.032527] FS: 0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
> [ 65.033981] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 65.035177] CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
> [ 65.036568] Stack:
> [ 65.037011] 0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
> [ 65.038472] ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
> [ 65.039935] ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
> [ 65.041468] Call Trace:
> [ 65.042050] [<ffffffffa037e4a5>] nfs4_close_state+0x15/0x20 [nfsv4]
> [ 65.043209] [<ffffffffa036a6c8>] nfs4_open_recover_helper+0x148/0x1f0 [nfsv4]
> [ 65.044529] [<ffffffffa036a886>] nfs4_open_recover+0x116/0x150 [nfsv4]
> [ 65.045730] [<ffffffffa036d98d>] nfs4_open_reclaim+0xad/0x150 [nfsv4]
> [ 65.046905] [<ffffffffa037d979>] nfs4_do_reclaim+0x149/0x5f0 [nfsv4]
> [ 65.048071] [<ffffffffa037e1dc>] nfs4_run_state_manager+0x3bc/0x670 [nfsv4]
> [ 65.049436] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
> [ 65.050686] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
> [ 65.051943] [<ffffffff81088640>] kthread+0xc0/0xd0
> [ 65.052831] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
> [ 65.054697] [<ffffffff8165686c>] ret_from_fork+0x7c/0xb0
> [ 65.056396] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
> [ 65.058208] Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb <4c> 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
> [ 65.065225] RIP [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
> [ 65.067175] RSP <ffff88007b907d10>
> [ 65.068570] CR2: 0000000000000030
> [ 65.070098] ---[ end trace 0d1fe4f5c7dd6f8b ]---
>
> Signed-off-by: Weston Andros Adamson <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d2b4845..58658db 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1316,16 +1316,27 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
> struct inode *inode = data->state->inode;
> struct nfs4_state *state = data->state;
> int ret;
> + bool cached_open = false;
>
> if (!data->rpc_done) {
> - ret = data->rpc_status;
> - goto err;
> + if (data->rpc_status) {
> + ret = data->rpc_status;
> + goto err;
> + } else
> + /*
> + * This is a cached open from an earlier operation
> + * of the current state recovery event so there is
> + * no need to update or check anything, just lookup
> + * the open state.
> + */
> + cached_open = true;
> }
>
> ret = -ESTALE;
> - if (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
> - !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
> - !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE))
> + if (!cached_open &&
> + (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
> + !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
> + !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE)))
> goto err;
>
> ret = -ENOMEM;
> @@ -1333,6 +1344,9 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
> if (state == NULL)
> goto err;
>
> + if (cached_open)
> + goto out;
> +
> ret = nfs_refresh_inode(inode, &data->f_attr);
> if (ret)
> goto err;
> @@ -1344,6 +1358,7 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
> update_open_stateid(state, &data->o_res.stateid, NULL,
> data->o_arg.fmode);
>
> +out:
> return state;
> err:
> return ERR_PTR(ret);
> --
> 1.7.12.4 (Apple Git-37)
>
Ok, I figured this out.
Bisecting took me to 5f65753033d8c5a53e65810bff3832e8282c68d1, which seems unrelated, but blocks open (and open recover) code paths from being run, so the real culprit is two commits back e23008ec81ef37b7b271669ce5d2de2643b2dc75. I changed the order of these patches and verified that e23008ec81ef37b7b271669ce5d2de2643b2dc75 introduces the NULL dereference.
e23008ec81ef37b7b271669ce5d2de2643b2dc75 is in all tags since v3.7.
Trond:
Please CC stable with a note that this is for v3.7+. I'll add the CC if I need to repost upon review.
-dros
On Oct 15, 2013, at 1:43 PM, Weston Andros Adamson <[email protected]> wrote:
> I plan on bisecting this next time I get a few spare cycles.
>
> -dros
>
> On Oct 14, 2013, at 11:51 AM, William Dauchy <[email protected]> wrote:
>
>> On Tue, Oct 8, 2013 at 1:40 AM, Weston Andros Adamson <[email protected]> wrote:
>>> This is the bug I found at the bakeathon. I can finally reproduce it quickly! And in a shockingly easy way!
>>> Tomorrow I'll figure out how long this regression has been present and if we need to CC stable.
>>
>> Any news on that? Does it apply on older version?
>>
>> Thanks,
>>
>> --
>> William
>
> --
> 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
I plan on bisecting this next time I get a few spare cycles.
-dros
On Oct 14, 2013, at 11:51 AM, William Dauchy <[email protected]> wrote:
> On Tue, Oct 8, 2013 at 1:40 AM, Weston Andros Adamson <[email protected]> wrote:
>> This is the bug I found at the bakeathon. I can finally reproduce it quickly! And in a shockingly easy way!
>> Tomorrow I'll figure out how long this regression has been present and if we need to CC stable.
>
> Any news on that? Does it apply on older version?
>
> Thanks,
>
> --
> William
-dros
On Oct 16, 2013, at 4:18 PM, "Myklebust, Trond" <[email protected]> wrote:
> On Mon, 2013-10-07 at 19:33 -0400, Weston Andros Adamson wrote:
>> _nfs4_opendata_reclaim_to_nfs4_state doesn't expect to see a cached
>> open CLAIM_PREVIOUS, but this can happen. An example is when there are
>> RDWR openers and RDONLY openers on a delegation stateid. The recovery
>> path will first try an open CLAIM_PREVIOUS for the RDWR openers, this
>> marks the delegation as not needing RECLAIM anymore, so the open
>> CLAIM_PREVIOUS for the RDONLY openers will not actually send an rpc.
>>
>> The NULL dereference is due to _nfs4_opendata_reclaim_to_nfs4_state
>> returning PTR_ERR(rpc_status) when !rpc_done. When the open is
>> cached, rpc_done == 0 and rpc_status == 0, thus
>> _nfs4_opendata_reclaim_to_nfs4_state returns NULL - this is unexpected
>> by callers of nfs4_opendata_to_nfs4_state().
>>
>> This can be reproduced easily by opening the same file two times on an
>> NFSv4.0 mount with delegations enabled, once as RDONLY and once as RDWR then
>> sleeping for a long time. While the files are held open, kick off state
>> recovery and this NULL dereference will be hit every time.
>>
>> An example OOPS:
>>
>> [ 65.003602] BUG: unable to handle kernel NULL pointer dereference at 00000000
>> 00000030
>> [ 65.005312] IP: [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.006820] PGD 7b0ea067 PUD 791ff067 PMD 0
>> [ 65.008075] Oops: 0000 [#1] SMP
>> [ 65.008802] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
>> snd_ens1371 gameport nfsd snd_rawmidi snd_ac97_codec ac97_bus btusb snd_seq snd
>> _seq_device snd_pcm ppdev bluetooth auth_rpcgss coretemp snd_page_alloc crc32_pc
>> lmul crc32c_intel ghash_clmulni_intel microcode rfkill nfs_acl vmw_balloon serio
>> _raw snd_timer lockd parport_pc e1000 snd soundcore parport i2c_piix4 shpchp vmw
>> _vmci sunrpc ata_generic mperf pata_acpi mptspi vmwgfx ttm scsi_transport_spi dr
>> m mptscsih mptbase i2c_core
>> [ 65.018684] CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
>> .x86_64 #1
>> [ 65.020113] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
>> Reference Platform, BIOS 6.00 07/31/2013
>> [ 65.022012] task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
>> [ 65.023414] RIP: 0010:[<ffffffffa037d6ee>] [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.025079] RSP: 0018:ffff88007b907d10 EFLAGS: 00010246
>> [ 65.026042] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
>> [ 65.027321] RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
>> [ 65.028691] RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
>> [ 65.029990] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
>> [ 65.031295] R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
>> [ 65.032527] FS: 0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
>> [ 65.033981] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 65.035177] CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
>> [ 65.036568] Stack:
>> [ 65.037011] 0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
>> [ 65.038472] ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
>> [ 65.039935] ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
>> [ 65.041468] Call Trace:
>> [ 65.042050] [<ffffffffa037e4a5>] nfs4_close_state+0x15/0x20 [nfsv4]
>> [ 65.043209] [<ffffffffa036a6c8>] nfs4_open_recover_helper+0x148/0x1f0 [nfsv4]
>> [ 65.044529] [<ffffffffa036a886>] nfs4_open_recover+0x116/0x150 [nfsv4]
>> [ 65.045730] [<ffffffffa036d98d>] nfs4_open_reclaim+0xad/0x150 [nfsv4]
>> [ 65.046905] [<ffffffffa037d979>] nfs4_do_reclaim+0x149/0x5f0 [nfsv4]
>> [ 65.048071] [<ffffffffa037e1dc>] nfs4_run_state_manager+0x3bc/0x670 [nfsv4]
>> [ 65.049436] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
>> [ 65.050686] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
>> [ 65.051943] [<ffffffff81088640>] kthread+0xc0/0xd0
>> [ 65.052831] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
>> [ 65.054697] [<ffffffff8165686c>] ret_from_fork+0x7c/0xb0
>> [ 65.056396] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
>> [ 65.058208] Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb <4c> 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
>> [ 65.065225] RIP [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.067175] RSP <ffff88007b907d10>
>> [ 65.068570] CR2: 0000000000000030
>> [ 65.070098] ---[ end trace 0d1fe4f5c7dd6f8b ]---
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index d2b4845..58658db 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1316,16 +1316,27 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> struct inode *inode = data->state->inode;
>> struct nfs4_state *state = data->state;
>> int ret;
>> + bool cached_open = false;
>>
>> if (!data->rpc_done) {
>> - ret = data->rpc_status;
>> - goto err;
>> + if (data->rpc_status) {
>> + ret = data->rpc_status;
>> + goto err;
>> + } else
>> + /*
>> + * This is a cached open from an earlier operation
>> + * of the current state recovery event so there is
>> + * no need to update or check anything, just lookup
>> + * the open state.
>> + */
>> + cached_open = true;
>> }
>>
>> ret = -ESTALE;
>> - if (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
>> - !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
>> - !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE))
>> + if (!cached_open &&
>> + (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
>> + !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
>> + !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE)))
>> goto err;
>>
>> ret = -ENOMEM;
>> @@ -1333,6 +1344,9 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> if (state == NULL)
>> goto err;
>>
>> + if (cached_open)
>> + goto out;
>> +
>> ret = nfs_refresh_inode(inode, &data->f_attr);
>> if (ret)
>> goto err;
>> @@ -1344,6 +1358,7 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> update_open_stateid(state, &data->o_res.stateid, NULL,
>> data->o_arg.fmode);
>>
>> +out:
>> return state;
>> err:
>> return ERR_PTR(ret);
>
> OK. Looking at what _nfs4_opendata_reclaim_to_nfs4_state is supposed to
> do, there appear to be several more problems.
>
> 1. Why are we calling nfs4_get_open_state? We already have a value
> for state, and all we want to do is to update it (and bump the
> reference counter).
> 2. Why do we care about whether or not the GETATTR succeeded? We
> just did an open by filehandle.
> 3. Don't we have a state reference leak when nfs_refresh_inode
> fails?
Interesting questions, I'll need to look at this code path a bit more and get back to you.
-dros
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
On Mon, 2013-10-07 at 19:33 -0400, Weston Andros Adamson wrote:
+AD4- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state doesn't expect to see a cached
+AD4- open CLAIM+AF8-PREVIOUS, but this can happen. An example is when there are
+AD4- RDWR openers and RDONLY openers on a delegation stateid. The recovery
+AD4- path will first try an open CLAIM+AF8-PREVIOUS for the RDWR openers, this
+AD4- marks the delegation as not needing RECLAIM anymore, so the open
+AD4- CLAIM+AF8-PREVIOUS for the RDONLY openers will not actually send an rpc.
+AD4-
+AD4- The NULL dereference is due to +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state
+AD4- returning PTR+AF8-ERR(rpc+AF8-status) when +ACE-rpc+AF8-done. When the open is
+AD4- cached, rpc+AF8-done +AD0APQ- 0 and rpc+AF8-status +AD0APQ- 0, thus
+AD4- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state returns NULL - this is unexpected
+AD4- by callers of nfs4+AF8-opendata+AF8-to+AF8-nfs4+AF8-state().
+AD4-
+AD4- This can be reproduced easily by opening the same file two times on an
+AD4- NFSv4.0 mount with delegations enabled, once as RDONLY and once as RDWR then
+AD4- sleeping for a long time. While the files are held open, kick off state
+AD4- recovery and this NULL dereference will be hit every time.
+AD4-
+AD4- An example OOPS:
+AD4-
+AD4- +AFs- 65.003602+AF0- BUG: unable to handle kernel NULL pointer dereference at 00000000
+AD4- 00000030
+AD4- +AFs- 65.005312+AF0- IP: +AFsAPA-ffffffffa037d6ee+AD4AXQ- +AF8AXw-nfs4+AF8-close+-0x1e/0x160 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.006820+AF0- PGD 7b0ea067 PUD 791ff067 PMD 0
+AD4- +AFs- 65.008075+AF0- Oops: 0000 +AFsAIw-1+AF0- SMP
+AD4- +AFs- 65.008802+AF0- Modules linked in: rpcsec+AF8-gss+AF8-krb5 nfsv4 dns+AF8-resolver nfs fscache
+AD4- snd+AF8-ens1371 gameport nfsd snd+AF8-rawmidi snd+AF8-ac97+AF8-codec ac97+AF8-bus btusb snd+AF8-seq snd
+AD4- +AF8-seq+AF8-device snd+AF8-pcm ppdev bluetooth auth+AF8-rpcgss coretemp snd+AF8-page+AF8-alloc crc32+AF8-pc
+AD4- lmul crc32c+AF8-intel ghash+AF8-clmulni+AF8-intel microcode rfkill nfs+AF8-acl vmw+AF8-balloon serio
+AD4- +AF8-raw snd+AF8-timer lockd parport+AF8-pc e1000 snd soundcore parport i2c+AF8-piix4 shpchp vmw
+AD4- +AF8-vmci sunrpc ata+AF8-generic mperf pata+AF8-acpi mptspi vmwgfx ttm scsi+AF8-transport+AF8-spi dr
+AD4- m mptscsih mptbase i2c+AF8-core
+AD4- +AFs- 65.018684+AF0- CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
+AD4- .x86+AF8-64 +ACM-1
+AD4- +AFs- 65.020113+AF0- Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
+AD4- Reference Platform, BIOS 6.00 07/31/2013
+AD4- +AFs- 65.022012+AF0- task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
+AD4- +AFs- 65.023414+AF0- RIP: 0010:+AFsAPA-ffffffffa037d6ee+AD4AXQ- +AFsAPA-ffffffffa037d6ee+AD4AXQ- +AF8AXw-nfs4+AF8-close+-0x1e/0x160 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.025079+AF0- RSP: 0018:ffff88007b907d10 EFLAGS: 00010246
+AD4- +AFs- 65.026042+AF0- RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
+AD4- +AFs- 65.027321+AF0- RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
+AD4- +AFs- 65.028691+AF0- RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
+AD4- +AFs- 65.029990+AF0- R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
+AD4- +AFs- 65.031295+AF0- R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
+AD4- +AFs- 65.032527+AF0- FS: 0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
+AD4- +AFs- 65.033981+AF0- CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
+AD4- +AFs- 65.035177+AF0- CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
+AD4- +AFs- 65.036568+AF0- Stack:
+AD4- +AFs- 65.037011+AF0- 0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
+AD4- +AFs- 65.038472+AF0- ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
+AD4- +AFs- 65.039935+AF0- ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
+AD4- +AFs- 65.041468+AF0- Call Trace:
+AD4- +AFs- 65.042050+AF0- +AFsAPA-ffffffffa037e4a5+AD4AXQ- nfs4+AF8-close+AF8-state+-0x15/0x20 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.043209+AF0- +AFsAPA-ffffffffa036a6c8+AD4AXQ- nfs4+AF8-open+AF8-recover+AF8-helper+-0x148/0x1f0 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.044529+AF0- +AFsAPA-ffffffffa036a886+AD4AXQ- nfs4+AF8-open+AF8-recover+-0x116/0x150 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.045730+AF0- +AFsAPA-ffffffffa036d98d+AD4AXQ- nfs4+AF8-open+AF8-reclaim+-0xad/0x150 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.046905+AF0- +AFsAPA-ffffffffa037d979+AD4AXQ- nfs4+AF8-do+AF8-reclaim+-0x149/0x5f0 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.048071+AF0- +AFsAPA-ffffffffa037e1dc+AD4AXQ- nfs4+AF8-run+AF8-state+AF8-manager+-0x3bc/0x670 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.049436+AF0- +AFsAPA-ffffffffa037de20+AD4AXQ- ? nfs4+AF8-do+AF8-reclaim+-0x5f0/0x5f0 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.050686+AF0- +AFsAPA-ffffffffa037de20+AD4AXQ- ? nfs4+AF8-do+AF8-reclaim+-0x5f0/0x5f0 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.051943+AF0- +AFsAPA-ffffffff81088640+AD4AXQ- kthread+-0xc0/0xd0
+AD4- +AFs- 65.052831+AF0- +AFsAPA-ffffffff81088580+AD4AXQ- ? insert+AF8-kthread+AF8-work+-0x40/0x40
+AD4- +AFs- 65.054697+AF0- +AFsAPA-ffffffff8165686c+AD4AXQ- ret+AF8-from+AF8-fork+-0x7c/0xb0
+AD4- +AFs- 65.056396+AF0- +AFsAPA-ffffffff81088580+AD4AXQ- ? insert+AF8-kthread+AF8-work+-0x40/0x40
+AD4- +AFs- 65.058208+AF0- Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb +ADw-4c+AD4- 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
+AD4- +AFs- 65.065225+AF0- RIP +AFsAPA-ffffffffa037d6ee+AD4AXQ- +AF8AXw-nfs4+AF8-close+-0x1e/0x160 +AFs-nfsv4+AF0-
+AD4- +AFs- 65.067175+AF0- RSP +ADw-ffff88007b907d10+AD4-
+AD4- +AFs- 65.068570+AF0- CR2: 0000000000000030
+AD4- +AFs- 65.070098+AF0- ---+AFs- end trace 0d1fe4f5c7dd6f8b +AF0----
+AD4-
+AD4- Signed-off-by: Weston Andros Adamson +ADw-dros+AEA-netapp.com+AD4-
+AD4- ---
+AD4- fs/nfs/nfs4proc.c +AHw- 25 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+------
+AD4- 1 file changed, 20 insertions(+-), 5 deletions(-)
+AD4-
+AD4- diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
+AD4- index d2b4845..58658db 100644
+AD4- --- a/fs/nfs/nfs4proc.c
+AD4- +-+-+- b/fs/nfs/nfs4proc.c
+AD4- +AEAAQA- -1316,16 +-1316,27 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- struct inode +ACo-inode +AD0- data-+AD4-state-+AD4-inode+ADs-
+AD4- struct nfs4+AF8-state +ACo-state +AD0- data-+AD4-state+ADs-
+AD4- int ret+ADs-
+AD4- +- bool cached+AF8-open +AD0- false+ADs-
+AD4-
+AD4- if (+ACE-data-+AD4-rpc+AF8-done) +AHs-
+AD4- - ret +AD0- data-+AD4-rpc+AF8-status+ADs-
+AD4- - goto err+ADs-
+AD4- +- if (data-+AD4-rpc+AF8-status) +AHs-
+AD4- +- ret +AD0- data-+AD4-rpc+AF8-status+ADs-
+AD4- +- goto err+ADs-
+AD4- +- +AH0- else
+AD4- +- /+ACo-
+AD4- +- +ACo- This is a cached open from an earlier operation
+AD4- +- +ACo- of the current state recovery event so there is
+AD4- +- +ACo- no need to update or check anything, just lookup
+AD4- +- +ACo- the open state.
+AD4- +- +ACo-/
+AD4- +- cached+AF8-open +AD0- true+ADs-
+AD4- +AH0-
+AD4-
+AD4- ret +AD0- -ESTALE+ADs-
+AD4- - if (+ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-TYPE) +AHwAfA-
+AD4- - +ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-FILEID) +AHwAfA-
+AD4- - +ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-CHANGE))
+AD4- +- if (+ACE-cached+AF8-open +ACYAJg-
+AD4- +- (+ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-TYPE) +AHwAfA-
+AD4- +- +ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-FILEID) +AHwAfA-
+AD4- +- +ACE-(data-+AD4-f+AF8-attr.valid +ACY- NFS+AF8-ATTR+AF8-FATTR+AF8-CHANGE)))
+AD4- goto err+ADs-
+AD4-
+AD4- ret +AD0- -ENOMEM+ADs-
+AD4- +AEAAQA- -1333,6 +-1344,9 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- if (state +AD0APQ- NULL)
+AD4- goto err+ADs-
+AD4-
+AD4- +- if (cached+AF8-open)
+AD4- +- goto out+ADs-
+AD4- +-
+AD4- ret +AD0- nfs+AF8-refresh+AF8-inode(inode, +ACY-data-+AD4-f+AF8-attr)+ADs-
+AD4- if (ret)
+AD4- goto err+ADs-
+AD4- +AEAAQA- -1344,6 +-1358,7 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- update+AF8-open+AF8-stateid(state, +ACY-data-+AD4-o+AF8-res.stateid, NULL,
+AD4- data-+AD4-o+AF8-arg.fmode)+ADs-
+AD4-
+AD4- +-out:
+AD4- return state+ADs-
+AD4- err:
+AD4- return ERR+AF8-PTR(ret)+ADs-
OK. Looking at what +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state is supposed to
do, there appear to be several more problems.
1. Why are we calling nfs4+AF8-get+AF8-open+AF8-state? We already have a value
for state, and all we want to do is to update it (and bump the
reference counter).
2. Why do we care about whether or not the GETATTR succeeded? We
just did an open by filehandle.
3. Don't we have a state reference leak when nfs+AF8-refresh+AF8-inode
fails?
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Oct 16, 2013, at 4:18 PM, "Myklebust, Trond" <[email protected]> wrote:
> On Mon, 2013-10-07 at 19:33 -0400, Weston Andros Adamson wrote:
>> _nfs4_opendata_reclaim_to_nfs4_state doesn't expect to see a cached
>> open CLAIM_PREVIOUS, but this can happen. An example is when there are
>> RDWR openers and RDONLY openers on a delegation stateid. The recovery
>> path will first try an open CLAIM_PREVIOUS for the RDWR openers, this
>> marks the delegation as not needing RECLAIM anymore, so the open
>> CLAIM_PREVIOUS for the RDONLY openers will not actually send an rpc.
>>
>> The NULL dereference is due to _nfs4_opendata_reclaim_to_nfs4_state
>> returning PTR_ERR(rpc_status) when !rpc_done. When the open is
>> cached, rpc_done == 0 and rpc_status == 0, thus
>> _nfs4_opendata_reclaim_to_nfs4_state returns NULL - this is unexpected
>> by callers of nfs4_opendata_to_nfs4_state().
>>
>> This can be reproduced easily by opening the same file two times on an
>> NFSv4.0 mount with delegations enabled, once as RDONLY and once as RDWR then
>> sleeping for a long time. While the files are held open, kick off state
>> recovery and this NULL dereference will be hit every time.
>>
>> An example OOPS:
>>
>> [ 65.003602] BUG: unable to handle kernel NULL pointer dereference at 00000000
>> 00000030
>> [ 65.005312] IP: [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.006820] PGD 7b0ea067 PUD 791ff067 PMD 0
>> [ 65.008075] Oops: 0000 [#1] SMP
>> [ 65.008802] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
>> snd_ens1371 gameport nfsd snd_rawmidi snd_ac97_codec ac97_bus btusb snd_seq snd
>> _seq_device snd_pcm ppdev bluetooth auth_rpcgss coretemp snd_page_alloc crc32_pc
>> lmul crc32c_intel ghash_clmulni_intel microcode rfkill nfs_acl vmw_balloon serio
>> _raw snd_timer lockd parport_pc e1000 snd soundcore parport i2c_piix4 shpchp vmw
>> _vmci sunrpc ata_generic mperf pata_acpi mptspi vmwgfx ttm scsi_transport_spi dr
>> m mptscsih mptbase i2c_core
>> [ 65.018684] CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
>> .x86_64 #1
>> [ 65.020113] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
>> Reference Platform, BIOS 6.00 07/31/2013
>> [ 65.022012] task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
>> [ 65.023414] RIP: 0010:[<ffffffffa037d6ee>] [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.025079] RSP: 0018:ffff88007b907d10 EFLAGS: 00010246
>> [ 65.026042] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
>> [ 65.027321] RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
>> [ 65.028691] RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
>> [ 65.029990] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
>> [ 65.031295] R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
>> [ 65.032527] FS: 0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
>> [ 65.033981] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 65.035177] CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
>> [ 65.036568] Stack:
>> [ 65.037011] 0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
>> [ 65.038472] ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
>> [ 65.039935] ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
>> [ 65.041468] Call Trace:
>> [ 65.042050] [<ffffffffa037e4a5>] nfs4_close_state+0x15/0x20 [nfsv4]
>> [ 65.043209] [<ffffffffa036a6c8>] nfs4_open_recover_helper+0x148/0x1f0 [nfsv4]
>> [ 65.044529] [<ffffffffa036a886>] nfs4_open_recover+0x116/0x150 [nfsv4]
>> [ 65.045730] [<ffffffffa036d98d>] nfs4_open_reclaim+0xad/0x150 [nfsv4]
>> [ 65.046905] [<ffffffffa037d979>] nfs4_do_reclaim+0x149/0x5f0 [nfsv4]
>> [ 65.048071] [<ffffffffa037e1dc>] nfs4_run_state_manager+0x3bc/0x670 [nfsv4]
>> [ 65.049436] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
>> [ 65.050686] [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
>> [ 65.051943] [<ffffffff81088640>] kthread+0xc0/0xd0
>> [ 65.052831] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
>> [ 65.054697] [<ffffffff8165686c>] ret_from_fork+0x7c/0xb0
>> [ 65.056396] [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
>> [ 65.058208] Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb <4c> 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
>> [ 65.065225] RIP [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
>> [ 65.067175] RSP <ffff88007b907d10>
>> [ 65.068570] CR2: 0000000000000030
>> [ 65.070098] ---[ end trace 0d1fe4f5c7dd6f8b ]---
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index d2b4845..58658db 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1316,16 +1316,27 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> struct inode *inode = data->state->inode;
>> struct nfs4_state *state = data->state;
>> int ret;
>> + bool cached_open = false;
>>
>> if (!data->rpc_done) {
>> - ret = data->rpc_status;
>> - goto err;
>> + if (data->rpc_status) {
>> + ret = data->rpc_status;
>> + goto err;
>> + } else
>> + /*
>> + * This is a cached open from an earlier operation
>> + * of the current state recovery event so there is
>> + * no need to update or check anything, just lookup
>> + * the open state.
>> + */
>> + cached_open = true;
>> }
>>
>> ret = -ESTALE;
>> - if (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
>> - !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
>> - !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE))
>> + if (!cached_open &&
>> + (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
>> + !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
>> + !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE)))
>> goto err;
>>
>> ret = -ENOMEM;
>> @@ -1333,6 +1344,9 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> if (state == NULL)
>> goto err;
>>
>> + if (cached_open)
>> + goto out;
>> +
>> ret = nfs_refresh_inode(inode, &data->f_attr);
>> if (ret)
>> goto err;
>> @@ -1344,6 +1358,7 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> update_open_stateid(state, &data->o_res.stateid, NULL,
>> data->o_arg.fmode);
>>
>> +out:
>> return state;
>> err:
>> return ERR_PTR(ret);
>
> OK. Looking at what _nfs4_opendata_reclaim_to_nfs4_state is supposed to
> do, there appear to be several more problems.
>
> 1. Why are we calling nfs4_get_open_state? We already have a value
> for state, and all we want to do is to update it (and bump the
> reference counter).
Yeah, this is clearly not right.
> 2. Why do we care about whether or not the GETATTR succeeded? We
> just did an open by file handle.
I'm not 100% about this one, but it seems like we can just remove those checks.
> 3. Don't we have a state reference leak when nfs_refresh_inode
> fails?
Looks like it.
I'll clean this function up. I'm not sure how I should separate the issues into patches? Maybe first I'll clean these things up then see where I'm at with handling cached opens (the NULL deref problem).
-dros
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com