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
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
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
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 <[email protected]>
Date: Wed Jul 15 13:31:36 2020 -0400
nfsd4: fix NULL dereference in nfsd/clients display code
Reported-by: NeilBrown <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
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);
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 <[email protected]>
> Date: Wed Jul 15 13:31:36 2020 -0400
>
> nfsd4: fix NULL dereference in nfsd/clients display code
>
> Reported-by: NeilBrown <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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
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 <[email protected]>
> > Date: Wed Jul 15 13:31:36 2020 -0400
> >
> > nfsd4: fix NULL dereference in nfsd/clients display code
> >
> > Reported-by: NeilBrown <[email protected]>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> >
> > 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
On Wed, Jul 15, 2020 at 02:54:56PM -0400, J. Bruce Fields wrote:
> commit 4eef57aa4fc0
> Author: J. Bruce Fields <[email protected]>
> Date: Wed Jul 15 13:31:36 2020 -0400
>
> nfsd4: fix NULL dereference in nfsd/clients display code
>
> Reported-by: NeilBrown <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
Signed-off-by: J. Bruce Fields <[email protected]>
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;
}
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 <[email protected]>
>> Date: Wed Jul 15 13:31:36 2020 -0400
>>
>> nfsd4: fix NULL dereference in nfsd/clients display code
>>
>> Reported-by: NeilBrown <[email protected]>
>> Signed-off-by: J. Bruce Fields <[email protected]>
>>
>> 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 <[email protected]>
> 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 <[email protected]>
> Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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;
> }
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;
}
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 <[email protected]>
for the whole patch.
Thanks,
NeilBrown
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 <[email protected]>
>
> for the whole patch.
Thanks!--b.