From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "dcritch@redhat.com" <dcritch@redhat.com>,
        "d.lesca@solinos.it" <d.lesca@solinos.it>
Subject: Re: [PATCH 1/2] nfsd: don't replace page in rq_pages if it's a
 continuation of last page
Thread-Topic: [PATCH 1/2] nfsd: don't replace page in rq_pages if it's a
 continuation of last page
Thread-Index: AQHZWL8bpLhVrwRqqEan4MoWlxHuRa7+8Q2AgAATp4A=
Date:   Fri, 17 Mar 2023 14:16:44 +0000
Message-ID: <65C84563-6BCD-41CE-AF68-80E1869D217F@oracle.com>
References: <20230317105608.19393-1-jlayton@kernel.org>
 <c57b48f500b859a3daf6f95ccefdfbec72e1c9de.camel@kernel.org>
In-Reply-To: <c57b48f500b859a3daf6f95ccefdfbec72e1c9de.camel@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?bGu31qs+vgiHfqCoN8IVVQxfjMVvZmGzDtUUV61mMg6D8UxlHZe9CROs+atw?=
 =?us-ascii?Q?OXpK9Tn0DYCO+hgpeT67MIoSCi7TCFaV43Gai8SS3rNb8X5y6LwVS1Ctu2Mz?=
 =?us-ascii?Q?XmdF5uWgtMn0Tj6ulWfTJ1X+tH3WLT6yGR71MQR3mgjVsosLM7KLzzb5cjfw?=
 =?us-ascii?Q?WhR3cQv5y+OcM0dyzAtKoQg3Ma8fKe+jFkS1BtKfHwz77yglzVipK80jbx36?=
 =?us-ascii?Q?vQxs+9/UKL888ztcDQwXq47c99sjVNy9g9R00s7iWbV/zDSLTg2+Fud65UbX?=
 =?us-ascii?Q?NtVoIP0QXW27MZzWYm/FxRK0mEqWakz89T1iH8kbS4vPWRvNBhwjEj/oFojY?=
 =?us-ascii?Q?rM/+6GdzLIzGRtCUvX/sFssUnL2i4XnrWxktqIHi6h+6Ef3prKFwziwGRc5K?=
 =?us-ascii?Q?I1VEjSyy9bSoeokjx9Ydw6Bo/9r0I5Oznbs0z3HDPK580jpfCxu/AmBm4tnF?=
 =?us-ascii?Q?vKs/QHeTZyB59lK2n5LLWHRPxLToiIeclh67lSp5K4HzVX6FR2RFpadUCuAI?=
 =?us-ascii?Q?4P6hgNaB+6dk5hggOR9BAJjWA5T8Sg4wSUxyVZu7bvL1BxLEM7z2IVEWuXgu?=
 =?us-ascii?Q?f0fM3kDwIRmTK6VSlQULjERK11a+Ns76xXRduwQqaEeqcfpajtVOvEu8nWjJ?=
 =?us-ascii?Q?MXb5pjnTMEHlu0+DBSY/JYR4WqClaLI5EALoWwaUxbhXdpYNcSGlhLpm2XZW?=
 =?us-ascii?Q?dX9b46StGZwYjWMouisIKnk5137n5as8VRtj1LN61bn1zYU6iBk+Uh8QQSDu?=
 =?us-ascii?Q?AEskkuSzt5c6gqPEj7GmfREqEwhoxZKfv9G7xZ3yksUDsEy9sZIczE40rpK6?=
 =?us-ascii?Q?LKGTI2B7S7MGoJv5abOVM5MVXKz+g2AJ4aNikGx53JgWrNnXltp6D626pjlA?=
 =?us-ascii?Q?Cw8CSLewuni0kNsmcI8RcUJ4x4HzLxrgDqOTrp95A8hZ23lByDGFNxwST65b?=
 =?us-ascii?Q?zSfG2wjEeVC5Wwhz32/DIfiQKanV1tG1aiTUaCW5/NQwl1G1dEF1XZR0ITdh?=
 =?us-ascii?Q?iJF10JaCTJgp5Fg9JFd1uMFFdZvvgWHKiJr2dEBNWU+fs8DkwuCM2pKun9Jw?=
 =?us-ascii?Q?+0eoYpcwYNtYo4/pUtRSNoXdHpBYQn/HeF0RPVK/tpMC0yh73mX8j2MFaFwO?=
 =?us-ascii?Q?xyl3fPYJxQmNSOzzp4HAudloszeTYP2ZQYlPBqoj371wFqWuaJDWILHgtl1M?=
 =?us-ascii?Q?eysqPZW6AeN2aPPh3pAdT3ezmS6YUJvieHiOiUcRlc1d++DEo+p8ShsY4arp?=
 =?us-ascii?Q?ZD+Lpj2tNyAw7buQOVbs7ptqPBBsQ1GyxJdiy93Ko2IAhDbnT6nmbaNJU2oU?=
 =?us-ascii?Q?BmPgYkE1DF6AhCPWonaFXnm6WOxeaKo2lehop/vO0d5FI9JucGHfaUrJ7c0g?=
 =?us-ascii?Q?j6jWKYFFR47RDU2aQuILeh4QYSY38Du7mVu6sHJ7OgBHUau5N4G3PIGawP7l?=
 =?us-ascii?Q?LB8hok1J3/Q/FwTgKiN7p6xItr5otveYFGYYe+uVmi1cYILFqoWTHWxzWofU?=
 =?us-ascii?Q?Lwwwp9rjvbirTlqa5tYy68goKmNd2aFHtC+w+lnX67dfjE4yxaDzTGd0gSdf?=
 =?us-ascii?Q?UcoXLvQBSdP3TwUFLemNxrqbovrj4DsTQleIwAIfMtP7gDRF9/DuzfQhzeby?=
 =?us-ascii?Q?nw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8A56B78F8F130345B7ED1E0915F4A39B@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: NnNM9bQgT70xb6iU0dZhUyXYlHW73RVYGXv5Kia2L39qGuYkF0demU1wV44Hjj8QeM4JiuHVk6E2mIHX2hXKz+HaE2HrECvZJGzAae4H2chOq3xUsZs4p4uR18InBalGa5uyvFHeuV3DVu9RWeVgeHr1QKS+piVoD4g3q5SS91O2kJn+UvWrBMRc58KWDCGPsi7BXlcP+5pbI2BD+ugNQLD7Q4F+0kPA6YbEjqEpE/4Mm9ijZ1d+q4Xw/xxkH0V94s9XTYshn6RnyIdY2W8kC6ZT6q5bnQwjCPPMe7U9aDeAB9oDh+RvtePMVcV9URSaKAb0JZuNE9QOjGs/arjG1Jy9U0VjdEyYjO+2Bk7nQ2oLcpBIYw+WuYY0MoJnNhOsPSJ8kRv/8+jrMgL9wc73cwiub7RYtPBNJW+umRknlll1hMDOpSJO2W40rUZjpYh66myT6Q7jXDzCzi+KjyvyUlTZiL/UfntVVHp3/GWgFZ/pvRMYrAhpMDaUu4tU6a7+nrxuzXdoJFJUjjfgUO4dU62FROzGHHdnZB9lOzvATcTVEJMsUnhoELLLRv0I8ApCNBEYMJm7SBvC0trxv6h0n7fa7iBprFMIopAbI5BZ89BKuyZ/ZSXJ7zm0Mb/xQMF0X6C0pqabeYKkme+xd2AQjKRir+Ta2D7aY6jTlo1bxtZTgepNcxtw0QFFYilbitiXQX30Nt88Cu3ckjA+iQWomfcx+7+d/NspcNToawZJhkofXOq0AsbF/CwBxn3QSDASjeWLMB71GyXcxbSxhBuw1tg3cCQDjt1CUg1ymC/ULxQ1lXWDquX2ai7UNnXjOyXt/67IA+d4kjYLo9tA4biAH7vm2HyGzLjX9TQw+3RECIc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 579f30ef-72d6-4821-62d2-08db26f23c93
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2023 14:16:44.2665
 (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: n8OYyf8kZuIoscpm08WqSEVwma4xmwulPFwSnq3Ltwph4YHuAVtE9OLdoMoVI+/ZbnJ9UwmcaG8dX23sDzdlCQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6365
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-17_08,2023-03-16_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 bulkscore=0
 adultscore=0 phishscore=0 mlxscore=0 suspectscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303170098
X-Proofpoint-GUID: BZHWjlN03QJ5gf2I6kaXnhhxL0cgbaWi
X-Proofpoint-ORIG-GUID: BZHWjlN03QJ5gf2I6kaXnhhxL0cgbaWi
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 17, 2023, at 9:06 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2023-03-17 at 06:56 -0400, Jeff Layton wrote:
>> The splice read calls nfsd_splice_actor to put the pages containing file
>> data into the svc_rqst->rq_pages array. It's possible however to get a
>> splice result that only has a partial page at the end, if (e.g.) the
>> filesystem hands back a short read that doesn't cover the whole page.
>>=20
>> nfsd_splice_actor will plop the partial page into its rq_pages array and
>> return. Then later, when nfsd_splice_actor is called again, the
>> remainder of the page may end up being filled out. At this point,
>> nfsd_splice_actor will put the page into the array _again_ corrupting
>> the reply. If this is done enough times, rq_next_page will overrun the
>> array and corrupt the trailing fields -- the rq_respages and
>> rq_next_page pointers themselves.
>>=20
>> If we've already added the page to the array in the last pass, don't add
>> it to the array a second time when dealing with a splice continuation.
>> This was originally handled properly in nfsd_splice_actor, but commit
>> 91e23b1c3982 removed the check for it, and started universally replacing
>> pages.
>>=20
>> Fixes: 91e23b1c3982 ("NFSD: Clean up nfsd_splice_actor()")
>> Reported-by: Dario Lesca <d.lesca@solinos.it>
>> Tested-by: David Critch <dcritch@redhat.com>
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2150630
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>> fs/nfsd/vfs.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 502e1b7742db..3709ef57d96e 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -941,8 +941,11 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, str=
uct pipe_buffer *buf,
>> 	struct page *last_page;
>>=20
>> 	last_page =3D page + (offset + sd->len - 1) / PAGE_SIZE;
>> -	for (page +=3D offset / PAGE_SIZE; page <=3D last_page; page++)
>> -		svc_rqst_replace_page(rqstp, page);
>> +	for (page +=3D offset / PAGE_SIZE; page <=3D last_page; page++) {
>> +		/* Only replace page if we haven't already done so */
>=20
> Note that I think that this was probably the real rationale for the pp[-
> 1] check that 91e23b1c3982 removed. Given that, maybe we should flesh
> this comment out a bit more for posterity?
>=20
> 		/*
> 		 * When we're splicing from a pipe, it's possible that
> 		 * we'll get an incomplete page that may be updated on
> 		 * a later call. Only splice it into rq_pages once.
> 		 */

The "real" bug here is that the API contract for pipe splicing
isn't well defined, so I agree that it's very likely the pp[-1]
check was because a splice can call the actor repeatedly for the
same page. No one could remember why that check was there.

To be clear, if the passed-in page matches the current page in
the rqst, we're "extending the current page" rather than avoiding
replacement... maybe:

	/*
	 * Skip page replacement when extending the contents
	 * of the current page.
	 */

In the patch description, would you mention that this case
arises if the READ request is not page-aligned?

If you resend this patch, please Cc: viro@ . Thanks for chasing
this down!


>> +		if (page !=3D *(rqstp->rq_next_page - 1))
>> +			svc_rqst_replace_page(rqstp, page);
>> +	}
>> 	if (rqstp->rq_res.page_len =3D=3D 0)	// first call
>> 		rqstp->rq_res.page_base =3D offset % PAGE_SIZE;
>> 	rqstp->rq_res.page_len +=3D sd->len;
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever