> On Jul 26, 2021, at 7:59 AM, trondmy@xxxxxxxxxx wrote:
>
> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>
> We really should not call rpc_wake_up_queued_task_set_status() with
> xprt->snd_task as an argument unless we are certain that is actually an
> rpc_task.
>
> Fixes: 0445f92c5d53 ("SUNRPC: Fix disconnection races")
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> ---
> include/linux/sunrpc/xprt.h | 1 +
> net/sunrpc/xprt.c | 6 ++++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index c8c39f22d3b1..59cd97da895b 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -432,6 +432,7 @@ void xprt_release_write(struct rpc_xprt *, struct rpc_task *);
> #define XPRT_CONGESTED (9)
> #define XPRT_CWND_WAIT (10)
> #define XPRT_WRITE_SPACE (11)
> +#define XPRT_SND_IS_COOKIE (12)
+1 !!!
However, there are one or more tracepoint call sites that
need to know that snd_task is a valid pointer. As part of
this patch, would you review include/trace/events/sunrpc.h
and check that those are also safe?
> static inline void xprt_set_connected(struct rpc_xprt *xprt)
> {
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index fb6db09725c7..bddd354a0076 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -775,9 +775,9 @@ void xprt_force_disconnect(struct rpc_xprt *xprt)
> /* Try to schedule an autoclose RPC call */
> if (test_and_set_bit(XPRT_LOCKED, &xprt->state) == 0)
> queue_work(xprtiod_workqueue, &xprt->task_cleanup);
> - else if (xprt->snd_task)
> + else if (xprt->snd_task && !test_bit(XPRT_SND_IS_COOKIE, &xprt->state))
> rpc_wake_up_queued_task_set_status(&xprt->pending,
> - xprt->snd_task, -ENOTCONN);
> + xprt->snd_task, -ENOTCONN);
> spin_unlock(&xprt->transport_lock);
> }
> EXPORT_SYMBOL_GPL(xprt_force_disconnect);
> @@ -866,6 +866,7 @@ bool xprt_lock_connect(struct rpc_xprt *xprt,
> goto out;
> if (xprt->snd_task != task)
> goto out;
> + set_bit(XPRT_SND_IS_COOKIE, &xprt->state);
> xprt->snd_task = cookie;
> ret = true;
> out:
> @@ -881,6 +882,7 @@ void xprt_unlock_connect(struct rpc_xprt *xprt, void *cookie)
> if (!test_bit(XPRT_LOCKED, &xprt->state))
> goto out;
> xprt->snd_task =NULL;
> + clear_bit(XPRT_SND_IS_COOKIE, &xprt->state);
> xprt->ops->release_xprt(xprt, NULL);
> xprt_schedule_autodisconnect(xprt);
> out:
> --
> 2.31.1
>
--
Chuck Lever
