From: Chuck Lever <hidden> Date: 2022-07-07 15:58:38
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
@@ -180,6 +180,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,nf->nf_cred=get_current_cred();nf->nf_net=net;nf->nf_flags=0;+ihold(inode);nf->nf_inode=inode;nf->nf_hashval=hashval;refcount_set(&nf->nf_ref,1);
From: Jeff Layton <hidden> Date: 2022-07-07 16:55:07
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
@@ -180,6 +180,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,nf->nf_cred=get_current_cred();nf->nf_net=net;nf->nf_flags=0;+ihold(inode);nf->nf_inode=inode;nf->nf_hashval=hashval;refcount_set(&nf->nf_ref,1);
From: Chuck Lever III <hidden> Date: 2022-07-07 16:58:30
On Jul 7, 2022, at 12:55 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
Thanks for the look. I will treat this as a clean-up, then, and see
what can be done about nfsd_file_mark_find_or_create() and
nfs4_show_superblock().
/*
* A representation of a file that has been opened by knfsd. These are hashed
- * in the hashtable by inode pointer value. Note that this object doesn't
- * hold a reference to the inode by itself, so the nf_inode pointer should
- * never be dereferenced, only used for comparison.
+ * in the hashtable by inode pointer value.
*/
struct nfsd_file {
struct hlist_node nf_node;
From: Jeff Layton <hidden> Date: 2022-07-07 16:59:15
On Thu, 2022-07-07 at 12:55 -0400, Jeff Layton wrote:
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
One other spot. We also dereference it in nfsd_file_mark_find_or_create,
but I think that specific instance is OK. We know that we still hold a
reference to the inode at that point since it comes from fhp->fh_dentry,
so we shouldn't need to worry about it disappearing out from under us.
What did the crash look like?
@@ -180,6 +180,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,nf->nf_cred=get_current_cred();nf->nf_net=net;nf->nf_flags=0;+ihold(inode);nf->nf_inode=inode;nf->nf_hashval=hashval;refcount_set(&nf->nf_ref,1);
From: Chuck Lever III <hidden> Date: 2022-07-07 17:02:01
On Jul 7, 2022, at 12:59 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 12:55 -0400, Jeff Layton wrote:
quoted
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
One other spot. We also dereference it in nfsd_file_mark_find_or_create,
but I think that specific instance is OK. We know that we still hold a
reference to the inode at that point since it comes from fhp->fh_dentry,
so we shouldn't need to worry about it disappearing out from under us.
Needs some annotation. I would prefer not to get that pointer from
nf_inode, then. As your comment says: compare only, never deref.
/*
* A representation of a file that has been opened by knfsd. These are hashed
- * in the hashtable by inode pointer value. Note that this object doesn't
- * hold a reference to the inode by itself, so the nf_inode pointer should
- * never be dereferenced, only used for comparison.
+ * in the hashtable by inode pointer value.
*/
struct nfsd_file {
struct hlist_node nf_node;
From: Jeff Layton <hidden> Date: 2022-07-07 17:11:16
On Thu, 2022-07-07 at 16:58 +0000, Chuck Lever III wrote:
quoted
On Jul 7, 2022, at 12:55 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
Thanks for the look. I will treat this as a clean-up, then, and see
what can be done about nfsd_file_mark_find_or_create() and
nfs4_show_superblock().
I don't see a real need to hold a separate inode reference in the
nfsd_file as you should have one already by virtue of the open file
itself. It probably won't hurt anything to hold one though if you decide
that's safer.
/*
* A representation of a file that has been opened by knfsd. These are hashed
- * in the hashtable by inode pointer value. Note that this object doesn't
- * hold a reference to the inode by itself, so the nf_inode pointer should
- * never be dereferenced, only used for comparison.
+ * in the hashtable by inode pointer value.
*/
struct nfsd_file {
struct hlist_node nf_node;
From: Jeff Layton <hidden> Date: 2022-07-07 17:12:55
On Thu, 2022-07-07 at 17:01 +0000, Chuck Lever III wrote:
quoted
On Jul 7, 2022, at 12:59 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 12:55 -0400, Jeff Layton wrote:
quoted
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.
One other spot. We also dereference it in nfsd_file_mark_find_or_create,
but I think that specific instance is OK. We know that we still hold a
reference to the inode at that point since it comes from fhp->fh_dentry,
so we shouldn't need to worry about it disappearing out from under us.
Needs some annotation. I would prefer not to get that pointer from
nf_inode, then. As your comment says: compare only, never deref.
Yeah, maybe we should pass in the inode as a separate parameter to that
function?
/*
* A representation of a file that has been opened by knfsd. These are hashed
- * in the hashtable by inode pointer value. Note that this object doesn't
- * hold a reference to the inode by itself, so the nf_inode pointer should
- * never be dereferenced, only used for comparison.
+ * in the hashtable by inode pointer value.
*/
struct nfsd_file {
struct hlist_node nf_node;
From: Chuck Lever III <hidden> Date: 2022-07-07 17:14:00
On Jul 7, 2022, at 1:12 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 17:01 +0000, Chuck Lever III wrote:
quoted
quoted
On Jul 7, 2022, at 12:59 PM, Jeff Layton [off-list ref] wrote:
One other spot. We also dereference it in nfsd_file_mark_find_or_create,
but I think that specific instance is OK. We know that we still hold a
reference to the inode at that point since it comes from fhp->fh_dentry,
so we shouldn't need to worry about it disappearing out from under us.
Needs some annotation. I would prefer not to get that pointer from
nf_inode, then. As your comment says: compare only, never deref.
Yeah, maybe we should pass in the inode as a separate parameter to that
function?
That's what I ended up doing, that seems most clear.
--
Chuck Lever
From: Chuck Lever III <hidden> Date: 2022-07-07 17:25:25
On Jul 7, 2022, at 12:55 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
From: Jeff Layton <hidden> Date: 2022-07-07 17:30:42
On Thu, 2022-07-07 at 17:25 +0000, Chuck Lever III wrote:
quoted
On Jul 7, 2022, at 12:55 PM, Jeff Layton [off-list ref] wrote:
On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
quoted
The documenting comment for struct nf_file states:
/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/
However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.
Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)
Hi Jeff-
I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.
My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"
But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:
"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."
We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.
How about f->nf_file->f_inode ?
I'd probably prefer:
file_inode(f->nf_file)
...and I don't think there is a potential crash here either.
nfs4_show_superblock is called while holding the cl_lock. I don't think
the inode can disappear out from under you with that.
--
Jeff Layton [off-list ref]