* [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
@ 2020-05-08 22:19 J. Bruce Fields
2020-05-11 12:10 ` Masayoshi Mizuma
0 siblings, 1 reply; 9+ messages in thread
From: J. Bruce Fields @ 2020-05-08 22:19 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs
From: "J. Bruce Fields" <bfields@redhat.com>
We add the new state to the nfsi->open_states list, making it
potentially visible to other threads, before we've finished initializing
it.
That wasn't a problem when all the readers were also taking the i_lock
(as we do here), but since we switched to RCU, there's now a possibility
that a reader could see the partially initialized state.
Symptoms observed were a crash when another thread called
nfs4_get_valid_delegation() on a NULL inode.
Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfs/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index ac93715c05a4..a8dc25ce48bb 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
state = new;
state->owner = owner;
atomic_inc(&owner->so_count);
- list_add_rcu(&state->inode_states, &nfsi->open_states);
ihold(inode);
state->inode = inode;
+ list_add_rcu(&state->inode_states, &nfsi->open_states);
spin_unlock(&inode->i_lock);
/* Note: The reclaim code dictates that we add stateless
* and read-only stateids to the end of the list */
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-08 22:19 [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation J. Bruce Fields
@ 2020-05-11 12:10 ` Masayoshi Mizuma
2020-05-11 13:16 ` J. Bruce Fields
0 siblings, 1 reply; 9+ messages in thread
From: Masayoshi Mizuma @ 2020-05-11 12:10 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Trond Myklebust, Anna Schumaker, linux-nfs
On Fri, May 08, 2020 at 06:19:35PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> We add the new state to the nfsi->open_states list, making it
> potentially visible to other threads, before we've finished initializing
> it.
>
> That wasn't a problem when all the readers were also taking the i_lock
> (as we do here), but since we switched to RCU, there's now a possibility
> that a reader could see the partially initialized state.
>
> Symptoms observed were a crash when another thread called
> nfs4_get_valid_delegation() on a NULL inode.
>
> Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/nfs/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index ac93715c05a4..a8dc25ce48bb 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
> state = new;
> state->owner = owner;
> atomic_inc(&owner->so_count);
> - list_add_rcu(&state->inode_states, &nfsi->open_states);
> ihold(inode);
> state->inode = inode;
> + list_add_rcu(&state->inode_states, &nfsi->open_states);
> spin_unlock(&inode->i_lock);
> /* Note: The reclaim code dictates that we add stateless
> * and read-only stateids to the end of the list */
> --
Thank you for posting the patch! It works for our box.
Please feel free to add:
Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Without the patch, the system which is a NFSv4 client has been
crashed randomly. The panic log is such as:
BUG: unable to handle page fault for address: ffffffffffffffb0
...
RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4]
...
Call Trace:
nfs4_open_prepare+0x80/0x1c0 [nfsv4]
__rpc_execute+0x75/0x390 [sunrpc]
? finish_task_switch+0x75/0x260
rpc_async_schedule+0x29/0x40 [sunrpc]
process_one_work+0x1ad/0x370
worker_thread+0x30/0x390
? create_worker+0x1a0/0x1a0
kthread+0x10c/0x130
? kthread_park+0x80/0x80
ret_from_fork+0x22/0x30
After applied the patch, the panic is gone.
Thanks!
Masa
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 12:10 ` Masayoshi Mizuma
@ 2020-05-11 13:16 ` J. Bruce Fields
2020-05-11 13:43 ` Trond Myklebust
0 siblings, 1 reply; 9+ messages in thread
From: J. Bruce Fields @ 2020-05-11 13:16 UTC (permalink / raw)
To: Masayoshi Mizuma; +Cc: Trond Myklebust, Anna Schumaker, linux-nfs
Thanks, applying.--b.
On Mon, May 11, 2020 at 08:10:54AM -0400, Masayoshi Mizuma wrote:
> On Fri, May 08, 2020 at 06:19:35PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > We add the new state to the nfsi->open_states list, making it
> > potentially visible to other threads, before we've finished initializing
> > it.
> >
> > That wasn't a problem when all the readers were also taking the i_lock
> > (as we do here), but since we switched to RCU, there's now a possibility
> > that a reader could see the partially initialized state.
> >
> > Symptoms observed were a crash when another thread called
> > nfs4_get_valid_delegation() on a NULL inode.
> >
> > Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> > fs/nfs/nfs4state.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index ac93715c05a4..a8dc25ce48bb 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
> > state = new;
> > state->owner = owner;
> > atomic_inc(&owner->so_count);
> > - list_add_rcu(&state->inode_states, &nfsi->open_states);
> > ihold(inode);
> > state->inode = inode;
> > + list_add_rcu(&state->inode_states, &nfsi->open_states);
> > spin_unlock(&inode->i_lock);
> > /* Note: The reclaim code dictates that we add stateless
> > * and read-only stateids to the end of the list */
> > --
>
> Thank you for posting the patch! It works for our box.
> Please feel free to add:
>
> Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
> Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>
> Without the patch, the system which is a NFSv4 client has been
> crashed randomly. The panic log is such as:
>
> BUG: unable to handle page fault for address: ffffffffffffffb0
> ...
> RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4]
> ...
> Call Trace:
> nfs4_open_prepare+0x80/0x1c0 [nfsv4]
> __rpc_execute+0x75/0x390 [sunrpc]
> ? finish_task_switch+0x75/0x260
> rpc_async_schedule+0x29/0x40 [sunrpc]
> process_one_work+0x1ad/0x370
> worker_thread+0x30/0x390
> ? create_worker+0x1a0/0x1a0
> kthread+0x10c/0x130
> ? kthread_park+0x80/0x80
> ret_from_fork+0x22/0x30
>
> After applied the patch, the panic is gone.
>
> Thanks!
> Masa
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 13:16 ` J. Bruce Fields
@ 2020-05-11 13:43 ` Trond Myklebust
2020-05-11 13:57 ` bfields
0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2020-05-11 13:43 UTC (permalink / raw)
To: bfields, msys.mizuma; +Cc: schumakeranna, linux-nfs
On Mon, 2020-05-11 at 09:16 -0400, J. Bruce Fields wrote:
> Thanks, applying.--b.
>
You're applying? So should I remove it from the NFS client bugfixes?
> On Mon, May 11, 2020 at 08:10:54AM -0400, Masayoshi Mizuma wrote:
> > On Fri, May 08, 2020 at 06:19:35PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > >
> > > We add the new state to the nfsi->open_states list, making it
> > > potentially visible to other threads, before we've finished
> > > initializing
> > > it.
> > >
> > > That wasn't a problem when all the readers were also taking the
> > > i_lock
> > > (as we do here), but since we switched to RCU, there's now a
> > > possibility
> > > that a reader could see the partially initialized state.
> > >
> > > Symptoms observed were a crash when another thread called
> > > nfs4_get_valid_delegation() on a NULL inode.
> > >
> > > Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > ---
> > > fs/nfs/nfs4state.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > index ac93715c05a4..a8dc25ce48bb 100644
> > > --- a/fs/nfs/nfs4state.c
> > > +++ b/fs/nfs/nfs4state.c
> > > @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode,
> > > struct nfs4_state_owner *owner)
> > > state = new;
> > > state->owner = owner;
> > > atomic_inc(&owner->so_count);
> > > - list_add_rcu(&state->inode_states, &nfsi->open_states);
> > > ihold(inode);
> > > state->inode = inode;
> > > + list_add_rcu(&state->inode_states, &nfsi->open_states);
> > > spin_unlock(&inode->i_lock);
> > > /* Note: The reclaim code dictates that we add
> > > stateless
> > > * and read-only stateids to the end of the list */
> > > --
> >
> > Thank you for posting the patch! It works for our box.
> > Please feel free to add:
> >
> > Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
> > Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> > Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> >
> > Without the patch, the system which is a NFSv4 client has been
> > crashed randomly. The panic log is such as:
> >
> > BUG: unable to handle page fault for address: ffffffffffffffb0
> > ...
> > RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4]
> > ...
> > Call Trace:
> > nfs4_open_prepare+0x80/0x1c0 [nfsv4]
> > __rpc_execute+0x75/0x390 [sunrpc]
> > ? finish_task_switch+0x75/0x260
> > rpc_async_schedule+0x29/0x40 [sunrpc]
> > process_one_work+0x1ad/0x370
> > worker_thread+0x30/0x390
> > ? create_worker+0x1a0/0x1a0
> > kthread+0x10c/0x130
> > ? kthread_park+0x80/0x80
> > ret_from_fork+0x22/0x30
> >
> > After applied the patch, the panic is gone.
> >
> > Thanks!
> > Masa
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 13:43 ` Trond Myklebust
@ 2020-05-11 13:57 ` bfields
2020-05-11 14:01 ` bfields
0 siblings, 1 reply; 9+ messages in thread
From: bfields @ 2020-05-11 13:57 UTC (permalink / raw)
To: Trond Myklebust; +Cc: msys.mizuma, schumakeranna, linux-nfs
On Mon, May 11, 2020 at 01:43:30PM +0000, Trond Myklebust wrote:
> On Mon, 2020-05-11 at 09:16 -0400, J. Bruce Fields wrote:
> > Thanks, applying.--b.
> >
>
> You're applying? So should I remove it from the NFS client bugfixes?
No. Sorry, I responded to the wrong email!
--b.
>
> > On Mon, May 11, 2020 at 08:10:54AM -0400, Masayoshi Mizuma wrote:
> > > On Fri, May 08, 2020 at 06:19:35PM -0400, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > >
> > > > We add the new state to the nfsi->open_states list, making it
> > > > potentially visible to other threads, before we've finished
> > > > initializing
> > > > it.
> > > >
> > > > That wasn't a problem when all the readers were also taking the
> > > > i_lock
> > > > (as we do here), but since we switched to RCU, there's now a
> > > > possibility
> > > > that a reader could see the partially initialized state.
> > > >
> > > > Symptoms observed were a crash when another thread called
> > > > nfs4_get_valid_delegation() on a NULL inode.
> > > >
> > > > Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > > ---
> > > > fs/nfs/nfs4state.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > > index ac93715c05a4..a8dc25ce48bb 100644
> > > > --- a/fs/nfs/nfs4state.c
> > > > +++ b/fs/nfs/nfs4state.c
> > > > @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode,
> > > > struct nfs4_state_owner *owner)
> > > > state = new;
> > > > state->owner = owner;
> > > > atomic_inc(&owner->so_count);
> > > > - list_add_rcu(&state->inode_states, &nfsi->open_states);
> > > > ihold(inode);
> > > > state->inode = inode;
> > > > + list_add_rcu(&state->inode_states, &nfsi->open_states);
> > > > spin_unlock(&inode->i_lock);
> > > > /* Note: The reclaim code dictates that we add
> > > > stateless
> > > > * and read-only stateids to the end of the list */
> > > > --
> > >
> > > Thank you for posting the patch! It works for our box.
> > > Please feel free to add:
> > >
> > > Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
> > > Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> > > Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > >
> > > Without the patch, the system which is a NFSv4 client has been
> > > crashed randomly. The panic log is such as:
> > >
> > > BUG: unable to handle page fault for address: ffffffffffffffb0
> > > ...
> > > RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4]
> > > ...
> > > Call Trace:
> > > nfs4_open_prepare+0x80/0x1c0 [nfsv4]
> > > __rpc_execute+0x75/0x390 [sunrpc]
> > > ? finish_task_switch+0x75/0x260
> > > rpc_async_schedule+0x29/0x40 [sunrpc]
> > > process_one_work+0x1ad/0x370
> > > worker_thread+0x30/0x390
> > > ? create_worker+0x1a0/0x1a0
> > > kthread+0x10c/0x130
> > > ? kthread_park+0x80/0x80
> > > ret_from_fork+0x22/0x30
> > >
> > > After applied the patch, the panic is gone.
> > >
> > > Thanks!
> > > Masa
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 13:57 ` bfields
@ 2020-05-11 14:01 ` bfields
2020-05-11 14:02 ` bfields
0 siblings, 1 reply; 9+ messages in thread
From: bfields @ 2020-05-11 14:01 UTC (permalink / raw)
To: Trond Myklebust; +Cc: msys.mizuma, schumakeranna, linux-nfs
On Mon, May 11, 2020 at 09:57:45AM -0400, bfields@fieldses.org wrote:
> On Mon, May 11, 2020 at 01:43:30PM +0000, Trond Myklebust wrote:
> > On Mon, 2020-05-11 at 09:16 -0400, J. Bruce Fields wrote:
> > > Thanks, applying.--b.
> > >
> >
> > You're applying? So should I remove it from the NFS client bugfixes?
>
> No. Sorry, I responded to the wrong email!
I do have a patch including the tags and oops provided by Masayoshi
Mizuma, if you'd like to take that instead. See followup.--b.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 14:01 ` bfields
@ 2020-05-11 14:02 ` bfields
2020-05-11 17:37 ` Masayoshi Mizuma
0 siblings, 1 reply; 9+ messages in thread
From: bfields @ 2020-05-11 14:02 UTC (permalink / raw)
To: Trond Myklebust; +Cc: msys.mizuma, schumakeranna, linux-nfs
From: "J. Bruce Fields" <bfields@redhat.com>
We add the new state to the nfsi->open_states list, making it
potentially visible to other threads, before we've finished initializing
it.
That wasn't a problem when all the readers were also taking the i_lock
(as we do here), but since we switched to RCU, there's now a possibility
that a reader could see the partially initialized state.
Symptoms observed were a crash when another thread called
nfs4_get_valid_delegation() on a NULL inode, resulting in an oops like:
BUG: unable to handle page fault for address: ffffffffffffffb0 ...
RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4] ...
Call Trace:
nfs4_open_prepare+0x80/0x1c0 [nfsv4]
__rpc_execute+0x75/0x390 [sunrpc]
? finish_task_switch+0x75/0x260
rpc_async_schedule+0x29/0x40 [sunrpc]
process_one_work+0x1ad/0x370
worker_thread+0x30/0x390
? create_worker+0x1a0/0x1a0
kthread+0x10c/0x130
? kthread_park+0x80/0x80
ret_from_fork+0x22/0x30
Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfs/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
> I do have a patch including the tags and oops provided by Masayoshi
> Mizuma, if you'd like to take that instead. See followup.--b.
Here you go.
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f7723d221945..459c7fb5d103 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
state = new;
state->owner = owner;
atomic_inc(&owner->so_count);
- list_add_rcu(&state->inode_states, &nfsi->open_states);
ihold(inode);
state->inode = inode;
+ list_add_rcu(&state->inode_states, &nfsi->open_states);
spin_unlock(&inode->i_lock);
/* Note: The reclaim code dictates that we add stateless
* and read-only stateids to the end of the list */
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 14:02 ` bfields
@ 2020-05-11 17:37 ` Masayoshi Mizuma
2020-05-11 17:39 ` bfields
0 siblings, 1 reply; 9+ messages in thread
From: Masayoshi Mizuma @ 2020-05-11 17:37 UTC (permalink / raw)
To: bfields, Trond Myklebust; +Cc: schumakeranna, linux-nfs
Hello,
How about adding Cc: stable@vger.kernel.org in the sign-off area?
9ae075fdd190 "NFSv4: Convert open state lookup to use RCU" was merged
to v4.20 and some stable tree have 9ae075fdd190.
On Mon, May 11, 2020 at 10:02:48AM -0400, bfields@fieldses.org wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> We add the new state to the nfsi->open_states list, making it
> potentially visible to other threads, before we've finished initializing
> it.
>
> That wasn't a problem when all the readers were also taking the i_lock
> (as we do here), but since we switched to RCU, there's now a possibility
> that a reader could see the partially initialized state.
>
> Symptoms observed were a crash when another thread called
> nfs4_get_valid_delegation() on a NULL inode, resulting in an oops like:
>
> BUG: unable to handle page fault for address: ffffffffffffffb0 ...
> RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4] ...
> Call Trace:
> nfs4_open_prepare+0x80/0x1c0 [nfsv4]
> __rpc_execute+0x75/0x390 [sunrpc]
> ? finish_task_switch+0x75/0x260
> rpc_async_schedule+0x29/0x40 [sunrpc]
> process_one_work+0x1ad/0x370
> worker_thread+0x30/0x390
> ? create_worker+0x1a0/0x1a0
> kthread+0x10c/0x130
> ? kthread_park+0x80/0x80
> ret_from_fork+0x22/0x30
>
> Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
Cc: stable@vger.kernel.org # 4.20.x-
Thanks!
Masa
> Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
> Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/nfs/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> > I do have a patch including the tags and oops provided by Masayoshi
> > Mizuma, if you'd like to take that instead. See followup.--b.
>
> Here you go.
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f7723d221945..459c7fb5d103 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
> state = new;
> state->owner = owner;
> atomic_inc(&owner->so_count);
> - list_add_rcu(&state->inode_states, &nfsi->open_states);
> ihold(inode);
> state->inode = inode;
> + list_add_rcu(&state->inode_states, &nfsi->open_states);
> spin_unlock(&inode->i_lock);
> /* Note: The reclaim code dictates that we add stateless
> * and read-only stateids to the end of the list */
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation
2020-05-11 17:37 ` Masayoshi Mizuma
@ 2020-05-11 17:39 ` bfields
0 siblings, 0 replies; 9+ messages in thread
From: bfields @ 2020-05-11 17:39 UTC (permalink / raw)
To: Masayoshi Mizuma; +Cc: Trond Myklebust, schumakeranna, linux-nfs
On Mon, May 11, 2020 at 01:37:23PM -0400, Masayoshi Mizuma wrote:
> Hello,
>
> How about adding Cc: stable@vger.kernel.org in the sign-off area?
> 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU" was merged
Makes sense to me. I'll leave it to Trond.--b.
> to v4.20 and some stable tree have 9ae075fdd190.
>
> On Mon, May 11, 2020 at 10:02:48AM -0400, bfields@fieldses.org wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > We add the new state to the nfsi->open_states list, making it
> > potentially visible to other threads, before we've finished initializing
> > it.
> >
> > That wasn't a problem when all the readers were also taking the i_lock
> > (as we do here), but since we switched to RCU, there's now a possibility
> > that a reader could see the partially initialized state.
> >
> > Symptoms observed were a crash when another thread called
> > nfs4_get_valid_delegation() on a NULL inode, resulting in an oops like:
> >
> > BUG: unable to handle page fault for address: ffffffffffffffb0 ...
> > RIP: 0010:nfs4_get_valid_delegation+0x6/0x30 [nfsv4] ...
> > Call Trace:
> > nfs4_open_prepare+0x80/0x1c0 [nfsv4]
> > __rpc_execute+0x75/0x390 [sunrpc]
> > ? finish_task_switch+0x75/0x260
> > rpc_async_schedule+0x29/0x40 [sunrpc]
> > process_one_work+0x1ad/0x370
> > worker_thread+0x30/0x390
> > ? create_worker+0x1a0/0x1a0
> > kthread+0x10c/0x130
> > ? kthread_park+0x80/0x80
> > ret_from_fork+0x22/0x30
> >
> > Fixes: 9ae075fdd190 "NFSv4: Convert open state lookup to use RCU"
>
> Cc: stable@vger.kernel.org # 4.20.x-
>
> Thanks!
> Masa
>
> > Reviewed-by: Seiichi Ikarashi <s.ikarashi@fujitsu.com>
> > Tested-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> > Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> > fs/nfs/nfs4state.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > > I do have a patch including the tags and oops provided by Masayoshi
> > > Mizuma, if you'd like to take that instead. See followup.--b.
> >
> > Here you go.
> >
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index f7723d221945..459c7fb5d103 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -734,9 +734,9 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
> > state = new;
> > state->owner = owner;
> > atomic_inc(&owner->so_count);
> > - list_add_rcu(&state->inode_states, &nfsi->open_states);
> > ihold(inode);
> > state->inode = inode;
> > + list_add_rcu(&state->inode_states, &nfsi->open_states);
> > spin_unlock(&inode->i_lock);
> > /* Note: The reclaim code dictates that we add stateless
> > * and read-only stateids to the end of the list */
> > --
> > 2.26.2
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-11 17:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 22:19 [PATCH] nfs: fix NULL deference in nfs4_get_valid_delegation J. Bruce Fields
2020-05-11 12:10 ` Masayoshi Mizuma
2020-05-11 13:16 ` J. Bruce Fields
2020-05-11 13:43 ` Trond Myklebust
2020-05-11 13:57 ` bfields
2020-05-11 14:01 ` bfields
2020-05-11 14:02 ` bfields
2020-05-11 17:37 ` Masayoshi Mizuma
2020-05-11 17:39 ` bfields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.