On Fri, Aug 23, 2024 at 10:40 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Fri, 2024-08-23 at 10:14 -0400, Olga Kornievskaia wrote:
> > states_show() relied on sc_type field to be of valid type
> > before calling into a subfunction to show content of a
> > particular stateid. But from commit 3f29cc82a84c we
> > split the validity of the stateid into sc_status and no longer
> > changed sc_type to 0 while unhashing the stateid. This
> > resulted in kernel oopsing as something like
> > nfs4_show_open() would derefence sc_file which was NULL.
> >
> > To reproduce: mount the server with 4.0, read and close
> > a file and then on the server cat /proc/fs/nfsd/clients/2/states
> >
> > [ 513.590804] Call trace:
> > [ 513.590925] _raw_spin_lock+0xcc/0x160
> > [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd]
> > [ 513.591412] states_show+0x44c/0x488 [nfsd]
> > [ 513.591681] seq_read_iter+0x5d8/0x760
> > [ 513.591896] seq_read+0x188/0x208
> > [ 513.592075] vfs_read+0x148/0x470
> > [ 513.592241] ksys_read+0xcc/0x178
> >
> > Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type")
> > Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> > ---
> > fs/nfsd/nfs4state.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c3def49074a4..8351724b8a43 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v)
> > {
> > struct nfs4_stid *st = v;
> >
> > + if (!st->sc_file)
> > + return 0;
> > +
> > switch (st->sc_type) {
> > case SC_TYPE_OPEN:
> > return nfs4_show_open(s, st);
>
> OPEN stateids are the only ones that stick around for a while after
> we've closed them out (and that's only for v4.0, IIRC). The others all
> only release the file after unhashing.
That's good to know. I had a reproducer for the open but the locking
stateid seem to be not affected by it (and I dont know yet how to
setup a pnfs server). If only nfs4_show_open() needs to be fixed, I
can do that.
> I think it'd be better to still display that stateid, and just not do
> any of the bits in nfs4_show_open that require ->sc_file. We have the
> "admin-revoked" string that we display now when SC_STATUS_ADMIN_REVOKED
> is set. Maybe we can display "closed" or something in this case?
I'll do as you suggest and then add "closed" if sc_file is null.
Thank you.
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
