On Fri, Aug 23, 2024 at 11:10 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>
>
>
> > On Aug 23, 2024, at 11:03 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> >
> > On Fri, Aug 23, 2024 at 10:44 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >>
> >> On Fri, Aug 23, 2024 at 10:14:01AM -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);
> >>> --
> >>> 2.43.5
> >>>
> >>
> >> I'll wait for Neil/Jeff's Reviewed-by, but the root cause analysis
> >> seems plausible to me, and I'll plan to apply it for v6.11-rc.
> >>
> >> Btw, I noticed this at the tail of states_show():
> >>
> >> /* XXX: copy stateids? */
> >>
> >> I'm not really sure, but those won't ever have an sc_file, will
> >> they? Are COPY callback stateids something we want the server to
> >> display in this code? Just musing aloud: maybe the NULL sc_file
> >> check wants to be moved into the show helpers.
> >
> > I can see that the copy stateids still have type NFS4_COPY_STID so
> > they are not converted to the new type of SC_TYPE. But it just means
> > we are not displaying them. I'm not sure what happens to sc_file for
> > copy stateid, I'd need to check.
>
> If you're already going to move the sc_file check into
> nfs4_show_open(), you can defer looking at adding specific
> support for callback stateids. We'll add it to the to-do
> list.
Now that I've looked at some copy code and also got more familiar with
this walking the stateids in proc, I'd like to note that proc only
walks the stateids that are store in clp->cl_stateids but the copy
stateid that we keep around (and those are the copy stateids in the
source server) are stored under the nfsd_net (namespace). Not to say
we shouldn't modify states_show() not to add walking a different list
but wanted to raise it. Also on the destination server, we stop async
copy stateid under the clp->async_copies list and they only live for
the lifetime of the copy. I know this bordes on a different discussion
of keeping copy stateids...
>
>
> --
> Chuck Lever
>
>
