Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     "trondmy@kernel.org" <trondmy@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/2] SUNRPC: Fix potential memory corruption
Thread-Topic: [PATCH 1/2] SUNRPC: Fix potential memory corruption
Thread-Index: AQHXghWxFYZhbtne1EyFuTz/JgrzT6tVTP2A
Date:   Mon, 26 Jul 2021 14:14:43 +0000
Message-ID: <11A2FD92-5090-429E-9E1E-FBC63007BF27@oracle.com>
References: <20210726115924.8576-1-trondmy@kernel.org>
In-Reply-To: <20210726115924.8576-1-trondmy@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?bA9nQiezKqfnkjUCGjhkM6kHkB7PPPAhDTI+HYUW4D2WiguGjogF9/adK6f4?=
 =?us-ascii?Q?FYDKRvRN7x01gqNw5Bp+vkcWJIGZaZdldzzG5KUiKtiweZP3t1i+F9qhXZot?=
 =?us-ascii?Q?TDRkUsuHuQu7DP1y5vc9E50MkdPQlB7+G7B0RHf5lq0rBvO54LPs0BNrQvCk?=
 =?us-ascii?Q?bAFFopUs/65XHN9uueKO69El0Q1O109hF0dR5wG19R2qQa71egZGbM5z7sSX?=
 =?us-ascii?Q?b08V4thLxD+AAYzvwlMhc4sv6oCwCgsxXqx7uu4r6nixdZv7gl84JJEs20Bb?=
 =?us-ascii?Q?GrPIx2f8ACOhCYANx5mrFaCR1mTUpBEwNLPjKyyjoJMylDeSvH2MwAhoJBew?=
 =?us-ascii?Q?99C5EYl+C5x525jAe9pwpbvQhOo91Ikj9pw4Ho0uqdVSY9AOK31rfcx91491?=
 =?us-ascii?Q?lJbtjhHEFmRJ/dLOuTIuIMRAHQKdgZt/glRL7pX51aGQMSNeltI5c5UzNaEf?=
 =?us-ascii?Q?I1fqMoftnjGkCGhJTNhydcim+YA9gZFl2KlncpmRdpPSxpnj7HfaZ/74RhM0?=
 =?us-ascii?Q?+8so7D43sPnAk62sMel8q9tzNzDdpsCub8iLAPIqTL8JseXQS7q7pJhaUrG0?=
 =?us-ascii?Q?7VnthO0ETqFZK9eczDouX2DS+iN1m7gsIY6Li6mqA+Zjlt8dpHF+nBxrNp2/?=
 =?us-ascii?Q?aY3aJYjRY83uvKOVNM7PDo6pKuwpAMvD+K9ifGH+kMsZAg7oM8t+d+mz6is8?=
 =?us-ascii?Q?M7WhDRzQuH8lVxqXyScWk0y4OF3qS4wJWOHIexbeFY6z63OXMaci1pHph7Uq?=
 =?us-ascii?Q?pSUjygW+NTMFY7sbi7q0AZyofnaIERLOWKXptfrrUq/wrliAhLFnmX3xDjxq?=
 =?us-ascii?Q?t4mQ76OSMqrtgY3xe3fqWRPi/YmQeaxTJ/xCuvhfrfiwzbL284zBNP9Lhm1X?=
 =?us-ascii?Q?p5xiT+eiggKALnf+mK/tWjq7pjTPbHKh++c4BrKEWXu483oba0GmW1lsSZr9?=
 =?us-ascii?Q?3kEO3VqxUhHan9A9W85PFZ0va03WyO3cmawpoimcEG5aZ2iSHseJ2ASy35cX?=
 =?us-ascii?Q?FtmMMgESXw0aFo7Zf/VATmRkvPDiXlMTrR2b7gvYdTV29p+ArrxMjmjwtpJ6?=
 =?us-ascii?Q?ZmjQDtGYZe1bCtOh39/A96h3uwsc/u5ZweSEWJeNNpAu4ASjNvDLhXU+Y1zR?=
 =?us-ascii?Q?Ra3kY1F5J+AI09y8gNbLHh1cB2NJxPGOC4hIqzgo2oq9hIUQEr64/WLYujdN?=
 =?us-ascii?Q?0z29YwoJXS6Z+j+IuvljPzBz81i11CpbqOHAHxoCglTTADNGUVizL66oUgzi?=
 =?us-ascii?Q?9LfASfE1Ar76ZITgpW8KaAWZ3RHqkGIEhBp4YAsBgTcHJxmqA71cFHnELN2G?=
 =?us-ascii?Q?gbnDneFVCQnUS1hiyxwfx0Do?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <07D940F3EB786A47A256F3A594A47FDA@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e71971eb-5739-45cf-8186-08d9503fb766
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2021 14:14:43.8371
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: PDAztt1eDRVa1D/x6hj1U6+zKzwn/jeJbg/NxurgTQQqP76uwRpuRkdDtrGlrPv7u2FVhRQstPbr6IXIDKJZ0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4194
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10056 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0 phishscore=0
 malwarescore=0 spamscore=0 mlxlogscore=999 mlxscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2107260081
X-Proofpoint-ORIG-GUID: EGVnrHSQ8mTrz9iuLtmYGAgPeUuZogJJ
X-Proofpoint-GUID: EGVnrHSQ8mTrz9iuLtmYGAgPeUuZogJJ
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 26, 2021, at 7:59 AM, trondmy@kernel.org wrote:
>=20
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>=20
> 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.
>=20
> Fixes: 0445f92c5d53 ("SUNRPC: Fix disconnection races")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> include/linux/sunrpc/xprt.h | 1 +
> net/sunrpc/xprt.c           | 6 ++++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>=20
> 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 r=
pc_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) =3D=3D 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 !=3D task)
> 		goto out;
> +	set_bit(XPRT_SND_IS_COOKIE, &xprt->state);
> 	xprt->snd_task =3D cookie;
> 	ret =3D 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 =3DNULL;
> +	clear_bit(XPRT_SND_IS_COOKIE, &xprt->state);
> 	xprt->ops->release_xprt(xprt, NULL);
> 	xprt_schedule_autodisconnect(xprt);
> out:
> --=20
> 2.31.1
>=20

--
Chuck Lever