My original patch was to just fix the NULL dereference, but Trond noticed
some other problems in _nfs4_opendata_reclaim_to_nfs4_state, so I went ahead
and fixed them.
I separated this into multiple patches for increased bisectability and made
sure the NULL dereference patch was first so that stable@ / distros can choose
to just fix the crash if they wish.
I went a bit further than the issues Trond raised with the last patch, but I
think it's good for consistency and catching reference counting bugs as errors
and not BUG()s.
-dros
Weston Andros Adamson (5):
NFSv4: fix NULL dereference in open recover
NFSv4: don't fail on missing fattr in open recover
NFSv4: clean up state ref counting in open recover
NFSv4: don't reprocess cached open CLAIM_PREVIOUS
NFSv4: clean up nfs4_state reference counting
fs/nfs/nfs4proc.c | 39 +++++++++++++++++++++++++--------------
fs/nfs/nfs4state.c | 3 ++-
2 files changed, 27 insertions(+), 15 deletions(-)
--
1.7.12.4 (Apple Git-37)
On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
+AD4- There's already a valid state (the one being recovered), so just
+AD4- reference it. Also clean up error paths to avoid ref leaks.
+AD4-
+AD4- Signed-off-by: Weston Andros Adamson +ADw-dros+AEA-netapp.com+AD4-
+AD4- ---
+AD4- fs/nfs/nfs4proc.c +AHw- 13 +-+-+-+-+-+-+-+------
+AD4- 1 file changed, 8 insertions(+-), 5 deletions(-)
+AD4-
+AD4- diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
+AD4- index 8140366..8ae1589 100644
+AD4- --- a/fs/nfs/nfs4proc.c
+AD4- +-+-+- b/fs/nfs/nfs4proc.c
+AD4- +AEAAQA- -1323,14 +-1323,14 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- goto err+ADs-
+AD4- +AH0-
+AD4-
+AD4- - ret +AD0- -ENOMEM+ADs-
+AD4- - state +AD0- nfs4+AF8-get+AF8-open+AF8-state(inode, data-+AD4-owner)+ADs-
+AD4- - if (state +AD0APQ- NULL)
+AD4- +- /+ACo- referenced the passed state +ACo-/
+AD4- +- ret +AD0- -EINVAL+ADs-
+AD4- +- if (state +AD0APQ- NULL +AHwAfA- +ACE-atomic+AF8-inc+AF8-not+AF8-zero(+ACY-state-+AD4-count))
+AD4- goto err+ADs-
We already know that state +ACEAPQ- NULL, and that state-+AD4-count +ACEAPQ- 0 here, so
I applied a simplified version of this patch that just does an
atomic+AF8-inc(+ACY-state-+AD4-count) just before the function return.
+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- +- goto err+AF8-put+ADs-
+AD4-
+AD4- nfs+AF8-setsecurity(inode, +ACY-data-+AD4-f+AF8-attr, data-+AD4-f+AF8-label)+ADs-
+AD4-
+AD4- +AEAAQA- -1340,9 +-1340,12 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- data-+AD4-o+AF8-arg.fmode)+ADs-
+AD4-
+AD4- return state+ADs-
+AD4- +-
+AD4- +-err+AF8-put:
+AD4- +- nfs4+AF8-put+AF8-open+AF8-state(state)+ADs-
+AD4- +-
+AD4- err:
+AD4- return ERR+AF8-PTR(ret)+ADs-
+AD4- -
+AD4- +AH0-
+AD4-
+AD4- static struct nfs4+AF8-state +ACo-
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
Cached opens have already been handled by _nfs4_opendata_reclaim_to_nfs4_state
and can safely skip being reprocessed, but must still call update_open_stateid
to make sure that all active fmodes are recovered.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8ae1589..a3b78df 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1315,12 +1315,15 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
{
struct inode *inode = data->state->inode;
struct nfs4_state *state = data->state;
+ bool cached_open = false;
int ret;
- /* allow cached opens (!rpc_done && !rpc_status) */
- if (!data->rpc_done && data->rpc_status) {
- ret = data->rpc_status;
- goto err;
+ if (!data->rpc_done) {
+ if (data->rpc_status) {
+ ret = data->rpc_status;
+ goto err;
+ } else
+ cached_open = true;
}
/* referenced the passed state */
@@ -1328,6 +1331,10 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
if (state == NULL || !atomic_inc_not_zero(&state->count))
goto err;
+ /* cached opens have already been processed */
+ if (cached_open)
+ goto update;
+
ret = nfs_refresh_inode(inode, &data->f_attr);
if (ret)
goto err_put;
@@ -1336,6 +1343,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
if (data->o_res.delegation_type != 0)
nfs4_opendata_check_deleg(data, state);
+
+update:
update_open_stateid(state, &data->o_res.stateid, NULL,
data->o_arg.fmode);
--
1.7.12.4 (Apple Git-37)
On Oct 28, 2013, at 2:45 PM, Myklebust, Trond <[email protected]> wrote:
> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> Use atomic_inc_not_zero() to avoid referencing a state that is currently
>> being freed.
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 8 ++++++--
>> fs/nfs/nfs4state.c | 3 ++-
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index a3b78df..005543d 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1275,7 +1275,8 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
>> out:
>> return ERR_PTR(ret);
>> out_return_state:
>> - atomic_inc(&state->count);
>> + if (!atomic_inc_not_zero(&state->count))
>> + return ERR_PTR(-EINVAL);
>> return state;
>> }
>>
>> @@ -1429,7 +1430,10 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
>> if (opendata == NULL)
>> return ERR_PTR(-ENOMEM);
>> opendata->state = state;
>> - atomic_inc(&state->count);
>> + if (!atomic_inc_not_zero(&state->count)) {
>> + nfs4_opendata_put(opendata);
>> + return ERR_PTR(-EINVAL);
>> + }
>> return opendata;
>> }
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index cc14cbb..1c71907 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1416,7 +1416,8 @@ restart:
>> continue;
>> if (state->state == 0)
>> continue;
>> - atomic_inc(&state->count);
>> + if (!atomic_inc_not_zero(&state->count))
>> + continue;
>> spin_unlock(&sp->so_lock);
>> status = ops->recover_open(sp, state);
>> if (status >= 0) {
>
> Is this patch needed? We should already be holding a reference to the
> open_context, so AFAICS we know that the state->count is non-zero.
I have no evidence that this is needed, it was purely for consistency. Feel free to drop this patch.
-dros
>
> Cheers
> Trond
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
Use atomic_inc_not_zero() to avoid referencing a state that is currently
being freed.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 8 ++++++--
fs/nfs/nfs4state.c | 3 ++-
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a3b78df..005543d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1275,7 +1275,8 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
out:
return ERR_PTR(ret);
out_return_state:
- atomic_inc(&state->count);
+ if (!atomic_inc_not_zero(&state->count))
+ return ERR_PTR(-EINVAL);
return state;
}
@@ -1429,7 +1430,10 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
if (opendata == NULL)
return ERR_PTR(-ENOMEM);
opendata->state = state;
- atomic_inc(&state->count);
+ if (!atomic_inc_not_zero(&state->count)) {
+ nfs4_opendata_put(opendata);
+ return ERR_PTR(-EINVAL);
+ }
return opendata;
}
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index cc14cbb..1c71907 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1416,7 +1416,8 @@ restart:
continue;
if (state->state == 0)
continue;
- atomic_inc(&state->count);
+ if (!atomic_inc_not_zero(&state->count))
+ continue;
spin_unlock(&sp->so_lock);
status = ops->recover_open(sp, state);
if (status >= 0) {
--
1.7.12.4 (Apple Git-37)
_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 RDWR and once as RDONLY 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 ]---
Cc: <[email protected]> #3.7+
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d2b4845..000063e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1317,7 +1317,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
struct nfs4_state *state = data->state;
int ret;
- if (!data->rpc_done) {
+ /* allow cached opens (!rpc_done && !rpc_status) */
+ if (!data->rpc_done && data->rpc_status) {
ret = data->rpc_status;
goto err;
}
--
1.7.12.4 (Apple Git-37)
On Oct 29, 2013, at 8:41 AM, Myklebust, Trond <[email protected]> wrote:
>
> On Oct 29, 2013, at 8:33 AM, Weston Andros Adamson <[email protected]> wrote:
>
>>
>> On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <[email protected]> wrote:
>>
>>> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <[email protected]> wrote:
>>>
>>>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>>>> There's already a valid state (the one being recovered), so just
>>>>> reference it. Also clean up error paths to avoid ref leaks.
>>>>>
>>>>> Signed-off-by: Weston Andros Adamson <[email protected]>
>>>>> ---
>>>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index 8140366..8ae1589 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>>>> goto err;
>>>>> }
>>>>>
>>>>> - ret = -ENOMEM;
>>>>> - state = nfs4_get_open_state(inode, data->owner);
>>>>> - if (state == NULL)
>>>>> + /* referenced the passed state */
>>>>> + ret = -EINVAL;
>>>>> + if (state == NULL || !atomic_inc_not_zero(&state->count))
>>>>> goto err;
>>>>
>>>> We already know that state != NULL, and that state->count != 0 here, so
>>>> I applied a simplified version of this patch that just does an
>>>> atomic_inc(&state->count) just before the function return.
>>>
>>> I just checked out the simplified patch - it looks good.
>>>
>>> Acked-by: Weston Andros Adamson <[email protected]>
>>
>> Oh, besides redundant CC lines?
>>
>> Cc: [email protected] # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
>> Cc: [email protected] # 3.7.x
>
> Those are there to indicate a dependency on the other patch. I believe this is in accordance with Documentation/stable-patches.txt...
Ah! Thanks,
-dros
This is an unneeded check that could cause the client to fail to recover
opens.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 000063e..8140366 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1323,12 +1323,6 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
goto err;
}
- 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))
- goto err;
-
ret = -ENOMEM;
state = nfs4_get_open_state(inode, data->owner);
if (state == NULL)
--
1.7.12.4 (Apple Git-37)
On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <[email protected]> wrote:
> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> There's already a valid state (the one being recovered), so just
>> reference it. Also clean up error paths to avoid ref leaks.
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 8140366..8ae1589 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> goto err;
>> }
>>
>> - ret = -ENOMEM;
>> - state = nfs4_get_open_state(inode, data->owner);
>> - if (state == NULL)
>> + /* referenced the passed state */
>> + ret = -EINVAL;
>> + if (state == NULL || !atomic_inc_not_zero(&state->count))
>> goto err;
>
> We already know that state != NULL, and that state->count != 0 here, so
> I applied a simplified version of this patch that just does an
> atomic_inc(&state->count) just before the function return.
I just checked out the simplified patch - it looks good.
Acked-by: Weston Andros Adamson <[email protected]>
-dros
>
>>
>> ret = nfs_refresh_inode(inode, &data->f_attr);
>> if (ret)
>> - goto err;
>> + goto err_put;
>>
>> nfs_setsecurity(inode, &data->f_attr, data->f_label);
>>
>> @@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> data->o_arg.fmode);
>>
>> return state;
>> +
>> +err_put:
>> + nfs4_put_open_state(state);
>> +
>> err:
>> return ERR_PTR(ret);
>> -
>> }
>>
>> static struct nfs4_state *
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
On Oct 29, 2013, at 8:33 AM, Weston Andros Adamson <[email protected]> wrote:
>
> On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <[email protected]> wrote:
>
>> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <[email protected]> wrote:
>>
>>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>>> There's already a valid state (the one being recovered), so just
>>>> reference it. Also clean up error paths to avoid ref leaks.
>>>>
>>>> Signed-off-by: Weston Andros Adamson <[email protected]>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 8140366..8ae1589 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>>> goto err;
>>>> }
>>>>
>>>> - ret = -ENOMEM;
>>>> - state = nfs4_get_open_state(inode, data->owner);
>>>> - if (state == NULL)
>>>> + /* referenced the passed state */
>>>> + ret = -EINVAL;
>>>> + if (state == NULL || !atomic_inc_not_zero(&state->count))
>>>> goto err;
>>>
>>> We already know that state != NULL, and that state->count != 0 here, so
>>> I applied a simplified version of this patch that just does an
>>> atomic_inc(&state->count) just before the function return.
>>
>> I just checked out the simplified patch - it looks good.
>>
>> Acked-by: Weston Andros Adamson <[email protected]>
>
> Oh, besides redundant CC lines?
>
> Cc: [email protected] # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
> Cc: [email protected] # 3.7.x
Those are there to indicate a dependency on the other patch. I believe this is in accordance with Documentation/stable-patches.txt...
Cheers
Trond
On Oct 28, 2013, at 5:08 PM, Myklebust, Trond <[email protected]> wrote:
> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> Cached opens have already been handled by _nfs4_opendata_reclaim_to_nfs4_state
>> and can safely skip being reprocessed, but must still call update_open_stateid
>> to make sure that all active fmodes are recovered.
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 8ae1589..a3b78df 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1315,12 +1315,15 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> {
>> struct inode *inode = data->state->inode;
>> struct nfs4_state *state = data->state;
>> + bool cached_open = false;
>> int ret;
>>
>> - /* allow cached opens (!rpc_done && !rpc_status) */
>> - if (!data->rpc_done && data->rpc_status) {
>> - ret = data->rpc_status;
>> - goto err;
>> + if (!data->rpc_done) {
>> + if (data->rpc_status) {
>> + ret = data->rpc_status;
>> + goto err;
>> + } else
>> + cached_open = true;
>
> This too was replaced by a simplified version. Once we get rid of the
> state == NULL check, we can just replace the above with a 'goto update?
Ok - to both updates. I guess I got a little too fancy.
-dros
>
>> }
>>
>> /* referenced the passed state */
>> @@ -1328,6 +1331,10 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> if (state == NULL || !atomic_inc_not_zero(&state->count))
>> goto err;
>>
>> + /* cached opens have already been processed */
>> + if (cached_open)
>> + goto update;
>> +
>> ret = nfs_refresh_inode(inode, &data->f_attr);
>> if (ret)
>> goto err_put;
>> @@ -1336,6 +1343,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>
>> if (data->o_res.delegation_type != 0)
>> nfs4_opendata_check_deleg(data, state);
>> +
>> +update:
>> update_open_stateid(state, &data->o_res.stateid, NULL,
>> data->o_arg.fmode);
>>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
+AD4- Use atomic+AF8-inc+AF8-not+AF8-zero() to avoid referencing a state that is currently
+AD4- being freed.
+AD4-
+AD4- Signed-off-by: Weston Andros Adamson +ADw-dros+AEA-netapp.com+AD4-
+AD4- ---
+AD4- fs/nfs/nfs4proc.c +AHw- 8 +-+-+-+-+-+---
+AD4- fs/nfs/nfs4state.c +AHw- 3 +-+--
+AD4- 2 files changed, 8 insertions(+-), 3 deletions(-)
+AD4-
+AD4- diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
+AD4- index a3b78df..005543d 100644
+AD4- --- a/fs/nfs/nfs4proc.c
+AD4- +-+-+- b/fs/nfs/nfs4proc.c
+AD4- +AEAAQA- -1275,7 +-1275,8 +AEAAQA- static struct nfs4+AF8-state +ACo-nfs4+AF8-try+AF8-open+AF8-cached(struct nfs4+AF8-opendata +ACo-opendata)
+AD4- out:
+AD4- return ERR+AF8-PTR(ret)+ADs-
+AD4- out+AF8-return+AF8-state:
+AD4- - atomic+AF8-inc(+ACY-state-+AD4-count)+ADs-
+AD4- +- if (+ACE-atomic+AF8-inc+AF8-not+AF8-zero(+ACY-state-+AD4-count))
+AD4- +- return ERR+AF8-PTR(-EINVAL)+ADs-
+AD4- return state+ADs-
+AD4- +AH0-
+AD4-
+AD4- +AEAAQA- -1429,7 +-1430,10 +AEAAQA- static struct nfs4+AF8-opendata +ACo-nfs4+AF8-open+AF8-recoverdata+AF8-alloc(struct nfs+AF8-open+AF8-context
+AD4- if (opendata +AD0APQ- NULL)
+AD4- return ERR+AF8-PTR(-ENOMEM)+ADs-
+AD4- opendata-+AD4-state +AD0- state+ADs-
+AD4- - atomic+AF8-inc(+ACY-state-+AD4-count)+ADs-
+AD4- +- if (+ACE-atomic+AF8-inc+AF8-not+AF8-zero(+ACY-state-+AD4-count)) +AHs-
+AD4- +- nfs4+AF8-opendata+AF8-put(opendata)+ADs-
+AD4- +- return ERR+AF8-PTR(-EINVAL)+ADs-
+AD4- +- +AH0-
+AD4- return opendata+ADs-
+AD4- +AH0-
+AD4-
+AD4- diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
+AD4- index cc14cbb..1c71907 100644
+AD4- --- a/fs/nfs/nfs4state.c
+AD4- +-+-+- b/fs/nfs/nfs4state.c
+AD4- +AEAAQA- -1416,7 +-1416,8 +AEAAQA- restart:
+AD4- continue+ADs-
+AD4- if (state-+AD4-state +AD0APQ- 0)
+AD4- continue+ADs-
+AD4- - atomic+AF8-inc(+ACY-state-+AD4-count)+ADs-
+AD4- +- if (+ACE-atomic+AF8-inc+AF8-not+AF8-zero(+ACY-state-+AD4-count))
+AD4- +- continue+ADs-
+AD4- spin+AF8-unlock(+ACY-sp-+AD4-so+AF8-lock)+ADs-
+AD4- status +AD0- ops-+AD4-recover+AF8-open(sp, state)+ADs-
+AD4- if (status +AD4APQ- 0) +AHs-
Is this patch needed? We should already be holding a reference to the
open+AF8-context, so AFAICS we know that the state-+AD4-count is non-zero.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Mon, Oct 21, 2013 at 7:10 PM, Weston Andros Adamson <[email protected]> wrote:
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d2b4845..000063e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1317,7 +1317,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
> struct nfs4_state *state = data->state;
> int ret;
>
> - if (!data->rpc_done) {
> + /* allow cached opens (!rpc_done && !rpc_status) */
> + if (!data->rpc_done && data->rpc_status) {
> ret = data->rpc_status;
> goto err;
> }
I'm also getting this trace on a 3.10.x; Trond, could you validate this patch?
Thanks,
--
William
There's already a valid state (the one being recovered), so just
reference it. Also clean up error paths to avoid ref leaks.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/nfs4proc.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8140366..8ae1589 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
goto err;
}
- ret = -ENOMEM;
- state = nfs4_get_open_state(inode, data->owner);
- if (state == NULL)
+ /* referenced the passed state */
+ ret = -EINVAL;
+ if (state == NULL || !atomic_inc_not_zero(&state->count))
goto err;
ret = nfs_refresh_inode(inode, &data->f_attr);
if (ret)
- goto err;
+ goto err_put;
nfs_setsecurity(inode, &data->f_attr, data->f_label);
@@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
data->o_arg.fmode);
return state;
+
+err_put:
+ nfs4_put_open_state(state);
+
err:
return ERR_PTR(ret);
-
}
static struct nfs4_state *
--
1.7.12.4 (Apple Git-37)
On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <[email protected]> wrote:
> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <[email protected]> wrote:
>
>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>> There's already a valid state (the one being recovered), so just
>>> reference it. Also clean up error paths to avoid ref leaks.
>>>
>>> Signed-off-by: Weston Andros Adamson <[email protected]>
>>> ---
>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 8140366..8ae1589 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>> goto err;
>>> }
>>>
>>> - ret = -ENOMEM;
>>> - state = nfs4_get_open_state(inode, data->owner);
>>> - if (state == NULL)
>>> + /* referenced the passed state */
>>> + ret = -EINVAL;
>>> + if (state == NULL || !atomic_inc_not_zero(&state->count))
>>> goto err;
>>
>> We already know that state != NULL, and that state->count != 0 here, so
>> I applied a simplified version of this patch that just does an
>> atomic_inc(&state->count) just before the function return.
>
> I just checked out the simplified patch - it looks good.
>
> Acked-by: Weston Andros Adamson <[email protected]>
Oh, besides redundant CC lines?
Cc: [email protected] # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
Cc: [email protected] # 3.7.x
-dros
>
> -dros
>
>
>>
>>>
>>> ret = nfs_refresh_inode(inode, &data->f_attr);
>>> if (ret)
>>> - goto err;
>>> + goto err_put;
>>>
>>> nfs_setsecurity(inode, &data->f_attr, data->f_label);
>>>
>>> @@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>> data->o_arg.fmode);
>>>
>>> return state;
>>> +
>>> +err_put:
>>> + nfs4_put_open_state(state);
>>> +
>>> err:
>>> return ERR_PTR(ret);
>>> -
>>> }
>>>
>>> static struct nfs4_state *
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer
>>
>> NetApp
>> [email protected]
>> http://www.netapp.com
>
> --
> 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
On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
+AD4- Cached opens have already been handled by +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state
+AD4- and can safely skip being reprocessed, but must still call update+AF8-open+AF8-stateid
+AD4- to make sure that all active fmodes are recovered.
+AD4-
+AD4- Signed-off-by: Weston Andros Adamson +ADw-dros+AEA-netapp.com+AD4-
+AD4- ---
+AD4- fs/nfs/nfs4proc.c +AHw- 17 +-+-+-+-+-+-+-+-+-+-+-+-+-----
+AD4- 1 file changed, 13 insertions(+-), 4 deletions(-)
+AD4-
+AD4- diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
+AD4- index 8ae1589..a3b78df 100644
+AD4- --- a/fs/nfs/nfs4proc.c
+AD4- +-+-+- b/fs/nfs/nfs4proc.c
+AD4- +AEAAQA- -1315,12 +-1315,15 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- +AHs-
+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- +- bool cached+AF8-open +AD0- false+ADs-
+AD4- int ret+ADs-
+AD4-
+AD4- - /+ACo- allow cached opens (+ACE-rpc+AF8-done +ACYAJg- +ACE-rpc+AF8-status) +ACo-/
+AD4- - if (+ACE-data-+AD4-rpc+AF8-done +ACYAJg- data-+AD4-rpc+AF8-status) +AHs-
+AD4- - ret +AD0- data-+AD4-rpc+AF8-status+ADs-
+AD4- - goto err+ADs-
+AD4- +- if (+ACE-data-+AD4-rpc+AF8-done) +AHs-
+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- +- cached+AF8-open +AD0- true+ADs-
This too was replaced by a simplified version. Once we get rid of the
state +AD0APQ- NULL check, we can just replace the above with a 'goto update'
+AD4- +AH0-
+AD4-
+AD4- /+ACo- referenced the passed state +ACo-/
+AD4- +AEAAQA- -1328,6 +-1331,10 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4- if (state +AD0APQ- NULL +AHwAfA- +ACE-atomic+AF8-inc+AF8-not+AF8-zero(+ACY-state-+AD4-count))
+AD4- goto err+ADs-
+AD4-
+AD4- +- /+ACo- cached opens have already been processed +ACo-/
+AD4- +- if (cached+AF8-open)
+AD4- +- goto update+ADs-
+AD4- +-
+AD4- ret +AD0- nfs+AF8-refresh+AF8-inode(inode, +ACY-data-+AD4-f+AF8-attr)+ADs-
+AD4- if (ret)
+AD4- goto err+AF8-put+ADs-
+AD4- +AEAAQA- -1336,6 +-1343,8 +AEAAQA- +AF8-nfs4+AF8-opendata+AF8-reclaim+AF8-to+AF8-nfs4+AF8-state(struct nfs4+AF8-opendata +ACo-data)
+AD4-
+AD4- if (data-+AD4-o+AF8-res.delegation+AF8-type +ACEAPQ- 0)
+AD4- nfs4+AF8-opendata+AF8-check+AF8-deleg(data, state)+ADs-
+AD4- +-
+AD4- +-update:
+AD4- update+AF8-open+AF8-stateid(state, +ACY-data-+AD4-o+AF8-res.stateid, NULL,
+AD4- data-+AD4-o+AF8-arg.fmode)+ADs-
+AD4-
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com