panic!("docallb") in nfsrv_docallback

[Alan Somers]

On Fri, Sep 4, 2020 at 8:58 PM Rick Macklem <rmacklem at uoguelph.ca> wrote:

> Alan Somers wrote:
> >I just saw this panic on a 12-stable machine.  Unfortunately, I don't have
> >a core dump, just a stack trace.  It was serving NFS v4.0, with
> delegations
> >enabled.  The clients were all Debian, with Linux 3.16.0.
> I will generically note that I believe the Linux NFS client developers
> mostly
> test NFSv4.1, 4.2, so if the clients support NFSv4.1, it might be worth
> upgrading?
>
> Also, delegations aren't enabled by default for a couple of reasons.
> 1 - For a long time, Linux only knew how to use read delegations and I felt
>      (and still feel) they are pretty useless.
> 2 - They are complex to get right.
> 3 - Although they should reduce the number of Open operations against the
>      server, I haven't observed dramatic performance improvements because
>      of them.
>
> >The proximal cause of the panic seems to be that the file had a write
> >delegation issued to an unconfirmed client.  Root cause is harder to
> >determine.  Did the kernel previously issue a delegation to an unconfirmed
> >client?  Or did the client somehow change to an unconfirmed state after
> the
> >delegation was issued, perhaps due to a race?
> I think the first case is more likely. Since client confirmation happens
> immediately
> for NFSv4.1 (the ExchangeID and Createsession must occur before anything
> else can happen), I wouldn;t be surprised if the Linux client tries to do
> an Open
> before the SetClientIDConfirm has completed for NFSv4.0.
>
> >It's hard to tell, but I don't see any checks for lc_flags &
> >LCL_NEEDSCONFIRM in nfsrv_openctrl (which issues the delegations), so I'm
> >guessing that that's the problem.
> The server should definitely check for a confirmed ClientID during Open and
> fail any Open attempt where that is not the case.
> --> I'll take a look at the code. I wrote it about 20years ago, but I can
> probably
>        figure out how it works.;-)
>
> >  If so, then the event trace would look
> >like this:
> >
> >1) Client Alice sends SETCLIENTID.  The server creates a client state
> >structure
> >   for her.
> >_) Client Alice should've sent SETCLIENTID_CONFIRM, but doesn't.  Bad
> Alice!
> >2) Client Alice sends OPEN for some file, and is issued a write
> delegation.
> >   The server shouldn't have issued it, because Alice's client ID is
> >   unconfirmed.  Bad server!
> >3) Client Bob tries to do a GETATTR on that same file.
> >4) In nfsrv_checkgetattr, the kernel finds a write delegation for that
> file,
> >   owned by client Alice.
> >5) The kernel tries to send a NFSV4OP_CBGETATTR callback to Alice, to see
> >if the
> >   file's attributes have changed.
> >6) But Alice's client ID is unconfirmed.  Oh no!  Panic!
> >
> >Does this sound plausible?  Should there be a check for LCL_NEEDSCONFIRM
> >somewhere around line 3166 in nfs_nfsdstate.c?  Grateful for any help.
> Yes, it does. I would have thought that I'd have checked for the
> unconfirmed
> ClientID, but maybe not.
>
> It is also possible that the client somehow did a SetClientID after the
> Open
> that issued the delegation, putting it back in "unconfirmed" state.
> It that was the case, maybe the panic(), intended to catch corrupted data
> structures, was overkill.
>
> >-Alan
> >
> >P.S.: stack trace
> >
> >kdb_backtrace
> >vpanic
> >panic
> >nfsrv_docallback
> >nfsrv_checkgetattr
> nfsrv_checkgetattr() should probably check for the case of an unconfirmed
>    clientid and then return ignoring any delegations hanging off it instead
>    of attempting a callback.
> --> This would handle the case where the client did a SetClientID after the
>       Open that acquired the delegation, leaving the ClientID unconfirmed.
>      - The two RPCs doing SetClientID and SetClientIDConfirm are normally
>         done only upon mounting or when the client thinks it has lost the
>         ClientID due to a lease expiry, but there is also the case where
> it is
>         changing the callback address. (This could explain the SetClientID
>         happening after the Open that acquired the delegation.)
> --> Hint. Can you now see why NFSv4.1 chose to do things differently?
>
> nfsrvd_getattr
> nfsrvd_dorpc
> nfssvc_program
> svc_run_internal
> svc_thread_start
> fork_exit
> fork_trampoline
>
> Thanks for reporting it. I'll take a look, rick
>

Wow, thanks!  That's better feedback than I ever hoped for.  About fixing
the bug: my odds of reproducing it in the original setting are low, since
it's a production server.  And I doubt I could force it via normal traffic
to a development server, either.  It took many terabytes of traffic to hit
the bug this once.  I think my best shot would be to use LibNFS (
https://github.com/sahlberg/libnfs) to write a custom misbehaving NFS
client.  But that's daunting; the only NFSv4 examples are nearly 1,000
lines each.  But, I could probably do it with enough time.  Do you have any
better ideas?
-Alan