Re: [PATCH 1/2] nfsd: clear cl_cb_session on DESTROY_SESSION

[Jeff Layton <jlayton@kernel.org>]

Dueling agent review. Looks like both problems are valid:

> The parent commit drains in-flight backchannel callbacks before
> nfsd4_destroy_session() frees the session, which closes the primary
> use-after-free.  clp->cl_cb_session still caches the freed pointer
> though: no destroy, expire, or shutdown path clears it, so the
> stale value survives until the next CREATE_SESSION overwrites it.

The drain is added by the companion patch "nfsd: drain inflight
callbacks in probe_callback_sync", which is patch 2/2 in this
series and applied after this one.  Should "The parent commit
drains" be rephrased, or should the series order be swapped so
the drain lands first?

> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 50827405468d..8af2d0cc37c2 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c

[ ... ]

> @@ -1278,10 +1280,12 @@ static int grab_slot(struct nfsd4_session *ses)
>  static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct rpc_task *task)
>  {
>  	struct nfs4_client *clp = cb->cb_clp;
> -	struct nfsd4_session *ses = clp->cl_cb_session;
> +	struct nfsd4_session *ses = READ_ONCE(clp->cl_cb_session);
>
>  	if (cb->cb_held_slot >= 0)
>  		return true;
> +	if (!ses)
> +		return false;

When ses is NULL, this returns false without calling
rpc_sleep_on().  The existing slot-unavailable path a few lines
below sleeps the task on cl_cb_waitq before returning false:

    rpc_sleep_on(&clp->cl_cb_waitq, task, NULL);
    cb->cb_held_slot = grab_slot(ses);
    if (cb->cb_held_slot < 0)
        return false;

nfsd41_cb_get_slot is the rpc_call_prepare callback, and
rpc_init_task() sets tk_action to rpc_prepare_task when
rpc_call_prepare is provided.  When prepare returns without
calling rpc_call_start() and without sleeping the task,
__rpc_execute() loops back and calls prepare again because
tk_action is unchanged and the task is not queued:

    __rpc_execute():
    for (;;) {
        do_action = task->tk_action; /* rpc_prepare_task */
        ...
        do_action(task);             /* nfsd4_cb_prepare */
        if (!RPC_IS_QUEUED(task)) {  /* true, not sleeping */
            cond_resched();
            continue;                /* back to the top */
        }

Can a callback reach this path?  After nfsd4_destroy_session()
clears cl_cb_session but before cl_cb_client is torn down,
another client could trigger a delegation recall.
nfsd4_run_cb() queues the work, nfsd4_run_cb_work() sees
cl_cb_client is still set and calls rpc_call_async(), and the
task enters the prepare loop above.

Would something like the following avoid the spin?

    if (!ses) {
        task->tk_rpc_status = -EIO;
        return false;
    }

That would route the task through rpc_exit_task ->
nfsd4_cb_sequence_done (which already handles NULL session via
goto requeue) and on to nfsd4_cb_release, breaking the loop.

>  	cb->cb_held_slot = grab_slot(ses);
>  	if (cb->cb_held_slot < 0) {
>  		rpc_sleep_on(&clp->cl_cb_waitq, task, NULL);

[ ... ]