* nfs4_show_superblock considered harmful :-) @ 2020-05-29 0:53 NeilBrown 2020-05-29 22:06 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: NeilBrown @ 2020-05-29 0:53 UTC (permalink / raw) To: J. Bruce Fields, Jeff Layton; +Cc: linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-05-29 0:53 nfs4_show_superblock considered harmful :-) NeilBrown @ 2020-05-29 22:06 ` J. Bruce Fields 2020-06-01 2:01 ` NeilBrown 0 siblings, 1 reply; 11+ messages in thread From: J. Bruce Fields @ 2020-05-29 22:06 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-05-29 22:06 ` J. Bruce Fields @ 2020-06-01 2:01 ` NeilBrown 2020-07-15 18:54 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: NeilBrown @ 2020-06-01 2:01 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Jeff Layton, linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-06-01 2:01 ` NeilBrown @ 2020-07-15 18:54 ` J. Bruce Fields 2020-07-15 23:05 ` NeilBrown 2020-07-16 17:19 ` J. Bruce Fields 0 siblings, 2 replies; 11+ messages in thread From: J. Bruce Fields @ 2020-07-15 18:54 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-15 18:54 ` J. Bruce Fields @ 2020-07-15 23:05 ` NeilBrown 2020-07-15 23:43 ` J. Bruce Fields 2020-07-16 17:19 ` J. Bruce Fields 1 sibling, 1 reply; 11+ messages in thread From: NeilBrown @ 2020-07-15 23:05 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Jeff Layton, linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-15 23:05 ` NeilBrown @ 2020-07-15 23:43 ` J. Bruce Fields 0 siblings, 0 replies; 11+ messages in thread From: J. Bruce Fields @ 2020-07-15 23:43 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-15 18:54 ` J. Bruce Fields 2020-07-15 23:05 ` NeilBrown @ 2020-07-16 17:19 ` J. Bruce Fields 2020-07-16 23:43 ` NeilBrown 1 sibling, 1 reply; 11+ messages in thread From: J. Bruce Fields @ 2020-07-16 17:19 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-16 17:19 ` J. Bruce Fields @ 2020-07-16 23:43 ` NeilBrown 2020-07-17 1:03 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: NeilBrown @ 2020-07-16 23:43 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Jeff Layton, linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-16 23:43 ` NeilBrown @ 2020-07-17 1:03 ` J. Bruce Fields 2020-07-17 1:31 ` NeilBrown 0 siblings, 1 reply; 11+ messages in thread From: J. Bruce Fields @ 2020-07-17 1:03 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-17 1:03 ` J. Bruce Fields @ 2020-07-17 1:31 ` NeilBrown 2020-07-17 2:18 ` J. Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: NeilBrown @ 2020-07-17 1:31 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Jeff Layton, linux-nfs [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfs4_show_superblock considered harmful :-) 2020-07-17 1:31 ` NeilBrown @ 2020-07-17 2:18 ` J. Bruce Fields 0 siblings, 0 replies; 11+ messages in thread From: J. Bruce Fields @ 2020-07-17 2:18 UTC (permalink / raw) To: NeilBrown; +Cc: Jeff Layton, linux-nfs 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-07-17 2:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-29 0:53 nfs4_show_superblock considered harmful :-) NeilBrown 2020-05-29 22:06 ` J. Bruce Fields 2020-06-01 2:01 ` NeilBrown 2020-07-15 18:54 ` J. Bruce Fields 2020-07-15 23:05 ` NeilBrown 2020-07-15 23:43 ` J. Bruce Fields 2020-07-16 17:19 ` J. Bruce Fields 2020-07-16 23:43 ` NeilBrown 2020-07-17 1:03 ` J. Bruce Fields 2020-07-17 1:31 ` NeilBrown 2020-07-17 2:18 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).