2021-07-26 12:00:35

by Trond Myklebust

Subject: [PATCH 1/2] SUNRPC: Fix potential memory corruption

From: Trond Myklebust <[email protected]>

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 <[email protected]>
---
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)

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

2021-07-26 12:03:03

by Trond Myklebust

Subject: [PATCH 2/2] SUNRPC: Clean up scheduling of autoclose

From: Trond Myklebust <[email protected]>

Consolidate duplicated code in xprt_force_disconnect() and
xprt_conditional_disconnect().

Signed-off-by: Trond Myklebust <[email protected]>
---
net/sunrpc/xprt.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index bddd354a0076..aae5a328b15b 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -760,6 +760,20 @@ void xprt_disconnect_done(struct rpc_xprt *xprt)
}
EXPORT_SYMBOL_GPL(xprt_disconnect_done);

+/**
+ * xprt_schedule_autoclose_locked - Try to schedule an autoclose RPC call
+ * @xprt: transport to disconnect
+ */
+static void xprt_schedule_autoclose_locked(struct rpc_xprt *xprt)
+{
+ set_bit(XPRT_CLOSE_WAIT, &xprt->state);
+ if (test_and_set_bit(XPRT_LOCKED, &xprt->state) == 0)
+ queue_work(xprtiod_workqueue, &xprt->task_cleanup);
+ 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_force_disconnect - force a transport to disconnect
* @xprt: transport to disconnect
@@ -771,13 +785,7 @@ void xprt_force_disconnect(struct rpc_xprt *xprt)

/* Don't race with the test_bit() in xprt_clear_locked() */
spin_lock(&xprt->transport_lock);
- set_bit(XPRT_CLOSE_WAIT, &xprt->state);
- /* 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 && !test_bit(XPRT_SND_IS_COOKIE, &xprt->state))
- rpc_wake_up_queued_task_set_status(&xprt->pending,
- xprt->snd_task, -ENOTCONN);
+ xprt_schedule_autoclose_locked(xprt);
spin_unlock(&xprt->transport_lock);
}
EXPORT_SYMBOL_GPL(xprt_force_disconnect);
@@ -817,11 +825,7 @@ void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie)
goto out;
if (test_bit(XPRT_CLOSING, &xprt->state))
goto out;
- set_bit(XPRT_CLOSE_WAIT, &xprt->state);
- /* Try to schedule an autoclose RPC call */
- if (test_and_set_bit(XPRT_LOCKED, &xprt->state) == 0)
- queue_work(xprtiod_workqueue, &xprt->task_cleanup);
- xprt_wake_pending_tasks(xprt, -EAGAIN);
+ xprt_schedule_autoclose_locked(xprt);
out:
spin_unlock(&xprt->transport_lock);
}
--
2.31.1

2021-07-26 14:16:09

by Chuck Lever III

Subject: Re: [PATCH 1/2] SUNRPC: Fix potential memory corruption

> On Jul 26, 2021, at 7:59 AM, [email protected] wrote:
>
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> ---
> 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