nfs4_show_superblock considered harmful :-)

nfs4_show_superblock considered harmful :-)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]


Hi,
 I've received a report of a 5.3 kernel crashing in
 nfs4_show_superblock().
 I was part way through preparing a patch when I concluded that
 the problem wasn't as straight forward as I thought.

 In the crash, the 'struct file *' passed to nfs4_show_superblock()
 was NULL.
 This file was acquired from find_any_file(), and every other caller
 of find_any_file() checks that the returned value is not NULL (though
 one BUGs if it is NULL - another WARNs).
 But nfs4_show_open() and nfs4_show_lock() don't.
 Maybe they should.  I didn't double check, but I suspect they don't
 hold enough locks to ensure that the files don't get removed.

 Then I noticed that nfs4_show_deleg() accesses fi_deleg_file without
 checking if it is NULL - Should it take fi_lock and make sure it is
 not NULL - and get a counted reference?
 And maybe nfs4_show_layout() has the same problem?

 I could probably have worked my way through fixing all of these, but
 then I discovered that these things are now 'struct nfsd_file *' rather
 than 'struct file *' and that the helpful documentation says:

 *    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.

 and yet nfs4_show_superblock() contains:

	struct inode *inode = f->nf_inode;

	seq_printf(s, "superblock: \"%02x:%02x:%ld\"",
					MAJOR(inode->i_sb->s_dev),
					 MINOR(inode->i_sb->s_dev),
					 inode->i_ino);

 do you see my problem?

 Is this really safe and the doco wrong? (I note that the use of nf_inode
 in nfsd_file_mark_find_or_create() looks wrong, but is actually safe).
 Or should we check if nf_file is non-NULL and use that?

 In short:
 - We should check find_any_file() return value - correct?
 - Do we need extra locking to stabilize fi_deleg_file?
 - ditto for ->ls_file
 - how can nfs4_show_superblock safely get s_dev and i_ino from a
   nfsd_file?

Thanks,

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
>  I've received a report of a 5.3 kernel crashing in
>  nfs4_show_superblock().
>  I was part way through preparing a patch when I concluded that
>  the problem wasn't as straight forward as I thought.
>
>  In the crash, the 'struct file *' passed to nfs4_show_superblock()
>  was NULL.
>  This file was acquired from find_any_file(), and every other caller
>  of find_any_file() checks that the returned value is not NULL (though
>  one BUGs if it is NULL - another WARNs).
>  But nfs4_show_open() and nfs4_show_lock() don't.
>  Maybe they should.  I didn't double check, but I suspect they don't
>  hold enough locks to ensure that the files don't get removed.

I think the only lock held is cl_lock, acquired in states_start.

We're starting here with an nfs4_stid that was found in the cl_stateids
idr.

A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
that idr under cl_lock before freeing the nfs4_stid and anything it
points to.

I think that was the theory....

One possible problem is downgrades, like nfs4_stateid_downgrade.

I'll keep mulling it over, thanks.

--b.

> 
> 
>  Then I noticed that nfs4_show_deleg() accesses fi_deleg_file without
>  checking if it is NULL - Should it take fi_lock and make sure it is
>  not NULL - and get a counted reference?
>  And maybe nfs4_show_layout() has the same problem?
> 
>  I could probably have worked my way through fixing all of these, but
>  then I discovered that these things are now 'struct nfsd_file *' rather
>  than 'struct file *' and that the helpful documentation says:
> 
>  *    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.
> 
>  and yet nfs4_show_superblock() contains:
> 
> 	struct inode *inode = f->nf_inode;
> 
> 	seq_printf(s, "superblock: \"%02x:%02x:%ld\"",
> 					MAJOR(inode->i_sb->s_dev),
> 					 MINOR(inode->i_sb->s_dev),
> 					 inode->i_ino);
> 
>  do you see my problem?
> 
>  Is this really safe and the doco wrong? (I note that the use of nf_inode
>  in nfsd_file_mark_find_or_create() looks wrong, but is actually safe).
>  Or should we check if nf_file is non-NULL and use that?
> 
>  In short:
>  - We should check find_any_file() return value - correct?
>  - Do we need extra locking to stabilize fi_deleg_file?
>  - ditto for ->ls_file
>  - how can nfs4_show_superblock safely get s_dev and i_ino from a
>    nfsd_file?
> 
> Thanks,
> 
> NeilBrown

Re: nfs4_show_superblock considered harmful :-)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

On Fri, May 29 2020, J. Bruce Fields wrote:

> On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
>>  I've received a report of a 5.3 kernel crashing in
>>  nfs4_show_superblock().
>>  I was part way through preparing a patch when I concluded that
>>  the problem wasn't as straight forward as I thought.
>>
>>  In the crash, the 'struct file *' passed to nfs4_show_superblock()
>>  was NULL.
>>  This file was acquired from find_any_file(), and every other caller
>>  of find_any_file() checks that the returned value is not NULL (though
>>  one BUGs if it is NULL - another WARNs).
>>  But nfs4_show_open() and nfs4_show_lock() don't.
>>  Maybe they should.  I didn't double check, but I suspect they don't
>>  hold enough locks to ensure that the files don't get removed.
>
> I think the only lock held is cl_lock, acquired in states_start.
>
> We're starting here with an nfs4_stid that was found in the cl_stateids
> idr.
>
> A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
> that idr under cl_lock before freeing the nfs4_stid and anything it
> points to.
>
> I think that was the theory....
>
> One possible problem is downgrades, like nfs4_stateid_downgrade.
>
> I'll keep mulling it over, thanks.

I had another look at code and maybe move_to_close_lru() is the problem.
It can clear remove the files and clear sc_file without taking
cl_lock.  So some protection is needed against that.

I think that only applies to nfs4_show_open() - not show_lock etc.
But I wonder it is might be best to include some extra protection
for each different case, just in case some future code change
allow sc_file to become NULL before the state is detached.

I'd feel more comforatable about nfs4_show_superblock() if it ignored
nf_inode and just used nf_file - it is isn't NULL.  It looks like it
can never be set from non-NULL to NULL.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Mon, Jun 01, 2020 at 12:01:07PM +1000, NeilBrown wrote:
> On Fri, May 29 2020, J. Bruce Fields wrote:
> 
> > On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
> >>  I've received a report of a 5.3 kernel crashing in
> >>  nfs4_show_superblock().
> >>  I was part way through preparing a patch when I concluded that
> >>  the problem wasn't as straight forward as I thought.
> >>
> >>  In the crash, the 'struct file *' passed to nfs4_show_superblock()
> >>  was NULL.
> >>  This file was acquired from find_any_file(), and every other caller
> >>  of find_any_file() checks that the returned value is not NULL (though
> >>  one BUGs if it is NULL - another WARNs).
> >>  But nfs4_show_open() and nfs4_show_lock() don't.
> >>  Maybe they should.  I didn't double check, but I suspect they don't
> >>  hold enough locks to ensure that the files don't get removed.
> >
> > I think the only lock held is cl_lock, acquired in states_start.
> >
> > We're starting here with an nfs4_stid that was found in the cl_stateids
> > idr.
> >
> > A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
> > that idr under cl_lock before freeing the nfs4_stid and anything it
> > points to.
> >
> > I think that was the theory....
> >
> > One possible problem is downgrades, like nfs4_stateid_downgrade.
> >
> > I'll keep mulling it over, thanks.
> 

Oops, I neglected this a while....

> I had another look at code and maybe move_to_close_lru() is the problem.
> It can clear remove the files and clear sc_file without taking
> cl_lock.  So some protection is needed against that.
> 
> I think that only applies to nfs4_show_open() - not show_lock etc.
> But I wonder it is might be best to include some extra protection
> for each different case, just in case some future code change
> allow sc_file to become NULL before the state is detached.
> 
> I'd feel more comforatable about nfs4_show_superblock() if it ignored
> nf_inode and just used nf_file - it is isn't NULL.  It looks like it
> can never be set from non-NULL to NULL.

But then that means we've always got a reference on the inode, doesn't
it?  So I still don't understand the nf_inode comment.

So maybe the NULL checks are mainly all we need.

Also it looks to me like ls_file lasts as long as the layout stateid, so
maybe it's OK.

--b.

commit 4eef57aa4fc0
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Jul 15 13:31:36 2020 -0400

    nfsd4: fix NULL dereference in nfsd/clients display code
    
    Reported-by: NeilBrown <neilb@suse.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ab5c8857ae5a..08b8376c74d7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
 	return ret;
 }
 
+static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
+{
+	struct nfsd_file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = nfsd_file_get(f->fi_deleg_file);
+	spin_unlock(&f->fi_lock);
+	return ret;
+}
+
 static atomic_long_t num_delegations;
 unsigned long max_delegations;
 
@@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
 	oo = ols->st_stateowner;
 	nf = st->sc_file;
 	file = find_any_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);
@@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
 	oo = ols->st_stateowner;
 	nf = st->sc_file;
 	file = find_any_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);
@@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
 
 	ds = delegstateid(st);
 	nf = st->sc_file;
-	file = nf->fi_deleg_file;
+	file = find_deleg_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);

Re: nfs4_show_superblock considered harmful :-)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]

On Wed, Jul 15 2020, J. Bruce Fields wrote:

> On Mon, Jun 01, 2020 at 12:01:07PM +1000, NeilBrown wrote:
>> On Fri, May 29 2020, J. Bruce Fields wrote:
>> 
>> > On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
>> >>  I've received a report of a 5.3 kernel crashing in
>> >>  nfs4_show_superblock().
>> >>  I was part way through preparing a patch when I concluded that
>> >>  the problem wasn't as straight forward as I thought.
>> >>
>> >>  In the crash, the 'struct file *' passed to nfs4_show_superblock()
>> >>  was NULL.
>> >>  This file was acquired from find_any_file(), and every other caller
>> >>  of find_any_file() checks that the returned value is not NULL (though
>> >>  one BUGs if it is NULL - another WARNs).
>> >>  But nfs4_show_open() and nfs4_show_lock() don't.
>> >>  Maybe they should.  I didn't double check, but I suspect they don't
>> >>  hold enough locks to ensure that the files don't get removed.
>> >
>> > I think the only lock held is cl_lock, acquired in states_start.
>> >
>> > We're starting here with an nfs4_stid that was found in the cl_stateids
>> > idr.
>> >
>> > A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
>> > that idr under cl_lock before freeing the nfs4_stid and anything it
>> > points to.
>> >
>> > I think that was the theory....
>> >
>> > One possible problem is downgrades, like nfs4_stateid_downgrade.
>> >
>> > I'll keep mulling it over, thanks.
>> 
>
> Oops, I neglected this a while....
>
>> I had another look at code and maybe move_to_close_lru() is the problem.
>> It can clear remove the files and clear sc_file without taking
>> cl_lock.  So some protection is needed against that.
>> 
>> I think that only applies to nfs4_show_open() - not show_lock etc.
>> But I wonder it is might be best to include some extra protection
>> for each different case, just in case some future code change
>> allow sc_file to become NULL before the state is detached.
>> 
>> I'd feel more comforatable about nfs4_show_superblock() if it ignored
>> nf_inode and just used nf_file - it is isn't NULL.  It looks like it
>> can never be set from non-NULL to NULL.
>
> But then that means we've always got a reference on the inode, doesn't
> it?  So I still don't understand the nf_inode comment.

My main problem with nf_inode is the comment

/*
 * 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.
 */

That comment is incompatible with the code in
nfsd_file_mark_find_or_create() and with the code in
nfs4_show_superblock().

>
> So maybe the NULL checks are mainly all we need.
>
> Also it looks to me like ls_file lasts as long as the layout stateid, so
> maybe it's OK.
>
> --b.
>
> commit 4eef57aa4fc0
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Jul 15 13:31:36 2020 -0400
>
>     nfsd4: fix NULL dereference in nfsd/clients display code
>     
>     Reported-by: NeilBrown <neilb@suse.de>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ab5c8857ae5a..08b8376c74d7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> +{
> +	struct nfsd_file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = nfsd_file_get(f->fi_deleg_file);
> +	spin_unlock(&f->fi_lock);
> +	return ret;
> +}
> +
>  static atomic_long_t num_delegations;
>  unsigned long max_delegations;
>  
> @@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
>  	file = find_any_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
>  	file = find_any_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>  
>  	ds = delegstateid(st);
>  	nf = st->sc_file;
> -	file = nf->fi_deleg_file;
> +	file = find_deleg_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);

You'll need to add nfsd_file_put(file) toward the end of this function.
Otherwise, I think this patch is a step in the right direction.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Thu, Jul 16, 2020 at 09:05:39AM +1000, NeilBrown wrote:
> On Wed, Jul 15 2020, J. Bruce Fields wrote:
> 
> > On Mon, Jun 01, 2020 at 12:01:07PM +1000, NeilBrown wrote:
> >> On Fri, May 29 2020, J. Bruce Fields wrote:
> >> 
> >> > On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
> >> >>  I've received a report of a 5.3 kernel crashing in
> >> >>  nfs4_show_superblock().
> >> >>  I was part way through preparing a patch when I concluded that
> >> >>  the problem wasn't as straight forward as I thought.
> >> >>
> >> >>  In the crash, the 'struct file *' passed to nfs4_show_superblock()
> >> >>  was NULL.
> >> >>  This file was acquired from find_any_file(), and every other caller
> >> >>  of find_any_file() checks that the returned value is not NULL (though
> >> >>  one BUGs if it is NULL - another WARNs).
> >> >>  But nfs4_show_open() and nfs4_show_lock() don't.
> >> >>  Maybe they should.  I didn't double check, but I suspect they don't
> >> >>  hold enough locks to ensure that the files don't get removed.
> >> >
> >> > I think the only lock held is cl_lock, acquired in states_start.
> >> >
> >> > We're starting here with an nfs4_stid that was found in the cl_stateids
> >> > idr.
> >> >
> >> > A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
> >> > that idr under cl_lock before freeing the nfs4_stid and anything it
> >> > points to.
> >> >
> >> > I think that was the theory....
> >> >
> >> > One possible problem is downgrades, like nfs4_stateid_downgrade.
> >> >
> >> > I'll keep mulling it over, thanks.
> >> 
> >
> > Oops, I neglected this a while....
> >
> >> I had another look at code and maybe move_to_close_lru() is the problem.
> >> It can clear remove the files and clear sc_file without taking
> >> cl_lock.  So some protection is needed against that.
> >> 
> >> I think that only applies to nfs4_show_open() - not show_lock etc.
> >> But I wonder it is might be best to include some extra protection
> >> for each different case, just in case some future code change
> >> allow sc_file to become NULL before the state is detached.
> >> 
> >> I'd feel more comforatable about nfs4_show_superblock() if it ignored
> >> nf_inode and just used nf_file - it is isn't NULL.  It looks like it
> >> can never be set from non-NULL to NULL.
> >
> > But then that means we've always got a reference on the inode, doesn't
> > it?  So I still don't understand the nf_inode comment.
> 
> My main problem with nf_inode is the comment
> 
> /*
>  * 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.
>  */
> 
> That comment is incompatible with the code in
> nfsd_file_mark_find_or_create() and with the code in
> nfs4_show_superblock().

Yeah, understood.  I'm inclined to think the comment's just wrong, but
not sure enough to be comfortable deleting it yet....

--b.

> 
> >
> > So maybe the NULL checks are mainly all we need.
> >
> > Also it looks to me like ls_file lasts as long as the layout stateid, so
> > maybe it's OK.
> >
> > --b.
> >
> > commit 4eef57aa4fc0
> > Author: J. Bruce Fields <bfields@redhat.com>
> > Date:   Wed Jul 15 13:31:36 2020 -0400
> >
> >     nfsd4: fix NULL dereference in nfsd/clients display code
> >     
> >     Reported-by: NeilBrown <neilb@suse.de>
> >     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ab5c8857ae5a..08b8376c74d7 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
> >  	return ret;
> >  }
> >  
> > +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> > +{
> > +	struct nfsd_file *ret;
> > +
> > +	spin_lock(&f->fi_lock);
> > +	ret = nfsd_file_get(f->fi_deleg_file);
> > +	spin_unlock(&f->fi_lock);
> > +	return ret;
> > +}
> > +
> >  static atomic_long_t num_delegations;
> >  unsigned long max_delegations;
> >  
> > @@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
> >  	oo = ols->st_stateowner;
> >  	nf = st->sc_file;
> >  	file = find_any_file(nf);
> > +	if (!file)
> > +		return 0;
> >  
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> > @@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
> >  	oo = ols->st_stateowner;
> >  	nf = st->sc_file;
> >  	file = find_any_file(nf);
> > +	if (!file)
> > +		return 0;
> >  
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> > @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
> >  
> >  	ds = delegstateid(st);
> >  	nf = st->sc_file;
> > -	file = nf->fi_deleg_file;
> > +	file = find_deleg_file(nf);
> > +	if (!file)
> > +		return 0;
> >  
> >  	seq_printf(s, "- ");
> >  	nfs4_show_stateid(s, &st->sc_stateid);
> 
> You'll need to add nfsd_file_put(file) toward the end of this function.
> Otherwise, I think this patch is a step in the right direction.
> 
> Thanks,
> NeilBrown

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Wed, Jul 15, 2020 at 02:54:56PM -0400, J. Bruce Fields wrote:
> commit 4eef57aa4fc0
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Jul 15 13:31:36 2020 -0400
> 
>     nfsd4: fix NULL dereference in nfsd/clients display code
>     
>     Reported-by: NeilBrown <neilb@suse.de>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ab5c8857ae5a..08b8376c74d7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> +{
> +	struct nfsd_file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = nfsd_file_get(f->fi_deleg_file);
> +	spin_unlock(&f->fi_lock);
> +	return ret;
> +}
> +
...
> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>  
>  	ds = delegstateid(st);
>  	nf = st->sc_file;
> -	file = nf->fi_deleg_file;
> +	file = find_deleg_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);

Oops, I added a "get" without a corresponding "put".

--b.

commit 8d2edfe45286
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Jul 15 13:31:36 2020 -0400

    nfsd4: fix NULL dereference in nfsd/clients display code
    
    We hold the cl_lock here, and that's enough to keep stateid's from going
    away, but it's not enough to prevent the files they point to from going
    away.  Take fi_lock and a reference and check for NULL, as we do in
    other code.
    
    Reported-by: NeilBrown <neilb@suse.de>
    Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ab5c8857ae5a..9d45117c8c18 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
 	return ret;
 }
 
+static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
+{
+	struct nfsd_file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = nfsd_file_get(f->fi_deleg_file);
+	spin_unlock(&f->fi_lock);
+	return ret;
+}
+
 static atomic_long_t num_delegations;
 unsigned long max_delegations;
 
@@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
 	oo = ols->st_stateowner;
 	nf = st->sc_file;
 	file = find_any_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);
@@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
 	oo = ols->st_stateowner;
 	nf = st->sc_file;
 	file = find_any_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);
@@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
 
 	ds = delegstateid(st);
 	nf = st->sc_file;
-	file = nf->fi_deleg_file;
+	file = find_deleg_file(nf);
+	if (!file)
+		return 0;
 
 	seq_printf(s, "- ");
 	nfs4_show_stateid(s, &st->sc_stateid);
@@ -2529,6 +2545,7 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
 	seq_printf(s, ", ");
 	nfs4_show_fname(s, file);
 	seq_printf(s, " }\n");
+	nfsd_file_put(file);
 
 	return 0;
 }

Re: nfs4_show_superblock considered harmful :-)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3829 bytes --]

On Thu, Jul 16 2020, J. Bruce Fields wrote:

> On Wed, Jul 15, 2020 at 02:54:56PM -0400, J. Bruce Fields wrote:
>> commit 4eef57aa4fc0
>> Author: J. Bruce Fields <bfields@redhat.com>
>> Date:   Wed Jul 15 13:31:36 2020 -0400
>> 
>>     nfsd4: fix NULL dereference in nfsd/clients display code
>>     
>>     Reported-by: NeilBrown <neilb@suse.de>
>>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index ab5c8857ae5a..08b8376c74d7 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
>>  	return ret;
>>  }
>>  
>> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
>> +{
>> +	struct nfsd_file *ret;
>> +
>> +	spin_lock(&f->fi_lock);
>> +	ret = nfsd_file_get(f->fi_deleg_file);
>> +	spin_unlock(&f->fi_lock);
>> +	return ret;
>> +}
>> +
> ...
>> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>>  
>>  	ds = delegstateid(st);
>>  	nf = st->sc_file;
>> -	file = nf->fi_deleg_file;
>> +	file = find_deleg_file(nf);
>> +	if (!file)
>> +		return 0;
>>  
>>  	seq_printf(s, "- ");
>>  	nfs4_show_stateid(s, &st->sc_stateid);
>
> Oops, I added a "get" without a corresponding "put".
>
> --b.
>
> commit 8d2edfe45286
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Jul 15 13:31:36 2020 -0400
>
>     nfsd4: fix NULL dereference in nfsd/clients display code
>     
>     We hold the cl_lock here, and that's enough to keep stateid's from going
>     away, but it's not enough to prevent the files they point to from going
>     away.  Take fi_lock and a reference and check for NULL, as we do in
>     other code.
>     
>     Reported-by: NeilBrown <neilb@suse.de>
>     Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ab5c8857ae5a..9d45117c8c18 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> +{
> +	struct nfsd_file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = nfsd_file_get(f->fi_deleg_file);

A test on f->fi_deleg_file being non-NULL would make this look safer.
It would  also make the subsequent test on the return value appear sane.

Thanks,
NeilBrown

> +	spin_unlock(&f->fi_lock);
> +	return ret;
> +}
> +
>  static atomic_long_t num_delegations;
>  unsigned long max_delegations;
>  
> @@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
>  	file = find_any_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
>  	file = find_any_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>  
>  	ds = delegstateid(st);
>  	nf = st->sc_file;
> -	file = nf->fi_deleg_file;
> +	file = find_deleg_file(nf);
> +	if (!file)
> +		return 0;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2529,6 +2545,7 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>  	seq_printf(s, ", ");
>  	nfs4_show_fname(s, file);
>  	seq_printf(s, " }\n");
> +	nfsd_file_put(file);
>  
>  	return 0;
>  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Fri, Jul 17, 2020 at 09:43:40AM +1000, NeilBrown wrote:
> On Thu, Jul 16 2020, J. Bruce Fields wrote:
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
> >  	return ret;
> >  }
> >  
> > +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> > +{
> > +	struct nfsd_file *ret;
> > +
> > +	spin_lock(&f->fi_lock);
> > +	ret = nfsd_file_get(f->fi_deleg_file);
> 
> A test on f->fi_deleg_file being non-NULL would make this look safer.
> It would  also make the subsequent test on the return value appear sane.

Yes, thanks!-b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c2a2e56c896d..6e8811e7c134 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -509,10 +509,11 @@ find_any_file(struct nfs4_file *f)
 
 static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
 {
-	struct nfsd_file *ret;
+	struct nfsd_file *ret = NULL;
 
 	spin_lock(&f->fi_lock);
-	ret = nfsd_file_get(f->fi_deleg_file);
+	if (f->fi_deleg_file)
+		ret = nfsd_file_get(f->fi_deleg_file);
 	spin_unlock(&f->fi_lock);
 	return ret;
 }

Re: nfs4_show_superblock considered harmful :-)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

On Thu, Jul 16 2020, J. Bruce Fields wrote:

> On Fri, Jul 17, 2020 at 09:43:40AM +1000, NeilBrown wrote:
>> On Thu, Jul 16 2020, J. Bruce Fields wrote:
>> > --- a/fs/nfsd/nfs4state.c
>> > +++ b/fs/nfsd/nfs4state.c
>> > @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
>> >  	return ret;
>> >  }
>> >  
>> > +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
>> > +{
>> > +	struct nfsd_file *ret;
>> > +
>> > +	spin_lock(&f->fi_lock);
>> > +	ret = nfsd_file_get(f->fi_deleg_file);
>> 
>> A test on f->fi_deleg_file being non-NULL would make this look safer.
>> It would  also make the subsequent test on the return value appear sane.
>
> Yes, thanks!-b.
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c2a2e56c896d..6e8811e7c134 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -509,10 +509,11 @@ find_any_file(struct nfs4_file *f)
>  
>  static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
>  {
> -	struct nfsd_file *ret;
> +	struct nfsd_file *ret = NULL;
>  
>  	spin_lock(&f->fi_lock);
> -	ret = nfsd_file_get(f->fi_deleg_file);
> +	if (f->fi_deleg_file)
> +		ret = nfsd_file_get(f->fi_deleg_file);
>  	spin_unlock(&f->fi_lock);
>  	return ret;
>  }

Reviewed-by: NeilBrown <neilb@suse.de>

for the whole patch.
Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: nfs4_show_superblock considered harmful :-)

[J. Bruce Fields]

On Fri, Jul 17, 2020 at 11:31:17AM +1000, NeilBrown wrote:
> On Thu, Jul 16 2020, J. Bruce Fields wrote:
> 
> > On Fri, Jul 17, 2020 at 09:43:40AM +1000, NeilBrown wrote:
> >> On Thu, Jul 16 2020, J. Bruce Fields wrote:
> >> > --- a/fs/nfsd/nfs4state.c
> >> > +++ b/fs/nfsd/nfs4state.c
> >> > @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f)
> >> >  	return ret;
> >> >  }
> >> >  
> >> > +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> >> > +{
> >> > +	struct nfsd_file *ret;
> >> > +
> >> > +	spin_lock(&f->fi_lock);
> >> > +	ret = nfsd_file_get(f->fi_deleg_file);
> >> 
> >> A test on f->fi_deleg_file being non-NULL would make this look safer.
> >> It would  also make the subsequent test on the return value appear sane.
> >
> > Yes, thanks!-b.
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c2a2e56c896d..6e8811e7c134 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -509,10 +509,11 @@ find_any_file(struct nfs4_file *f)
> >  
> >  static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> >  {
> > -	struct nfsd_file *ret;
> > +	struct nfsd_file *ret = NULL;
> >  
> >  	spin_lock(&f->fi_lock);
> > -	ret = nfsd_file_get(f->fi_deleg_file);
> > +	if (f->fi_deleg_file)
> > +		ret = nfsd_file_get(f->fi_deleg_file);
> >  	spin_unlock(&f->fi_lock);
> >  	return ret;
> >  }
> 
> Reviewed-by: NeilBrown <neilb@suse.de>
> 
> for the whole patch.

Thanks!--b.