From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Chuck Lever <cel@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        "dcritch@redhat.com" <dcritch@redhat.com>,
        "d.lesca@solinos.it" <d.lesca@solinos.it>
Subject: Re: [PATCH v1 0/3] rq_pages bounds checking
Thread-Topic: [PATCH v1 0/3] rq_pages bounds checking
Thread-Index: AQHZWSSVPN++Ads9h0qgGm1FjjxOzK8AT6OAgABYc4A=
Date:   Sat, 18 Mar 2023 15:20:36 +0000
Message-ID: <8356EDBD-7062-4F44-87F1-FF2BAE7F359F@oracle.com>
References: <167909365790.1672.13118429954916842449.stgit@klimt.1015granger.net>
 <cf2ad1349a19b472653c500cc7e287843a0cb8c7.camel@kernel.org>
In-Reply-To: <cf2ad1349a19b472653c500cc7e287843a0cb8c7.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?0Of0NrJ1OkNwA8DWy1Yh3Q6G0VA+PBFVlDGcO1Ka9MlW0eynzGdZ6BP+7KKM?=
 =?us-ascii?Q?7BXIsdB6Nu3RxK3sJZuZ+jdboaEi7gOOqlVfyH3LQH+JXkYRHlroJSA/wMgZ?=
 =?us-ascii?Q?Bm1FsTMY9WwyoMth/UZqMBiacy7On2Q2x5RnLPCRXOKyJCcUse80l0w04aWI?=
 =?us-ascii?Q?KQyIt6MT4eBuvxAtxbhVw0U3VJsK+wWky6bObC24pniSIljvTgLBQf2V0HX7?=
 =?us-ascii?Q?bz41e0lPJCmOc6kL/66bntqipvXf8xEl4uSjVUzjmaG64FWWDfb/RSpu7mHT?=
 =?us-ascii?Q?NTEBcx9xvsePo4b9hKWs5mnMWhPzpXCH4eKiwk3RYLZXYJdFenxvsySVhLvF?=
 =?us-ascii?Q?sILBq+c0dT8/IbosVCIu+i+cr7KJm7Qe8m+ck/khfhDXhMVjF1Cf9fJ5Po0V?=
 =?us-ascii?Q?BPDHFQiokKriMMc2u1co0+r3Lnlb+Yyg0fwBc091yGD+lBqaA3t/qzZa1e2Y?=
 =?us-ascii?Q?eJYwLp6Sj7lkSKzYLQbeXSATBd/aI6XVFX25amgVIIHh8BIn3b0KIv/hQsqy?=
 =?us-ascii?Q?rA1T0NNW6C1KEooN1qvBw4ymhVmk5ydMa6I94g9bweF0pbNj9MX6q3WwBRIk?=
 =?us-ascii?Q?pLF3rIw7NSbrnitlp8XB41hSkFZP2QZF+pBz3IzG3XeAXqjTprygiM51lMtd?=
 =?us-ascii?Q?Jla2UBbu59y2j8vkef90/yB7DRKK03jlpw4N8AjC61djm1845WB8mhHkqEHB?=
 =?us-ascii?Q?KWbhPqlQrST3gXaOF8dBvVtOP3JOu9tg76q93mLWGTQKh/thKHRlw+0JMQGe?=
 =?us-ascii?Q?V+p51OiE68gm2i4rOUxtmoEIT8yU7pbhWzQCNmTM/iG9+HOiz2h1q8v3v9a3?=
 =?us-ascii?Q?d8HWQZDFQzbiBYnTEIs4QzxY/8ryBY/KSAuEp0GKd4bbAQdYKyWa8fSf4d/w?=
 =?us-ascii?Q?83RXBWleaZGTH41HlXfT+zwslM8kUPaQkYRtd1W0jyi8twJ5neQw8JKUgEL1?=
 =?us-ascii?Q?PM5B5xstuehk3RoBVsT0U4dsqbHdR2GQa5Rmrf5YZlpLx/IIhEeqrAlJjYKH?=
 =?us-ascii?Q?a+VF5FI2HE2Qr6BQer449R0r707+4cco/9kUoQKE41/5Z3E8kf6K+8Hfxg1Z?=
 =?us-ascii?Q?VPxV7TQNUZwxO+YgQmt4dgPAI5e5blSGxmY15Tivnz6aFqcLxuWlebuYiKAe?=
 =?us-ascii?Q?+d7jLzgKuJwTcWMkMqevFMnvBBOQnyJYDSryh9QQzvGBmvx+A1jtpYCSHaHC?=
 =?us-ascii?Q?59yKFNg3lguGtkUsrHnMaaFAUgFbSFm+Fc8bLh4mqk0fgHzsW4BP78aAaaUe?=
 =?us-ascii?Q?ix0kiK8zZtgNVz1oslhd8IH2eDkllfPBW0R/byQYBKzYe6sHCET7p7bslJfD?=
 =?us-ascii?Q?/0FBqnMnHeMzTr7eEpMHia4N9kgcXFEytXArQYB4UplAvvu6mNkzbZOtmZzB?=
 =?us-ascii?Q?rA2HFUXO2bEgC5yIgku2kYTHDo4kiirghpVAfn5GuXPA2RossPXK0HZiXqjR?=
 =?us-ascii?Q?WoL87oBA3xo+3i6Vc4qOtRMaKb+FQFn8MCl5JExHx0SaLxMrGSIDsN0jAU2i?=
 =?us-ascii?Q?JtC/ra499WJ2S2mG2mKcwnG9Fi13uufIloNvxcLh1phiCugjANBkz+5Bpe62?=
 =?us-ascii?Q?UutgmBWPlfy2UAskKj4xjhYLy2fWLsLvVq11BER433NxdzU7MRwFzCh0A8hc?=
 =?us-ascii?Q?IA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1AFF46BD43E37D45A57CF2DC3A064C24@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: pM1kzzvKPdArSZWAi/4BtYDOkNIFrn1ar6rBVWvS0NsGLCx1gZr80mA+r7V26pfH8pH5LSIIorp2t1RfP+svfcYcX5eZ6NK3uPE7Euaxfn1LyhgNdmJ1g9HxtFBE7M+GRtXhPecERNFLUKRtXbxUYDaEnIye1kE3huofeWqqQOkDifSU+IgEYmoTTK7U3gG466oajCUrNrmLBc0rx/Aqn9UB25RuM5OvxJIZZkzFxv/mwDzVrD65aa5Io0JeXcE+V7goEq9JvbGKfX1cvxDwQgJ4YsOlkID4R8UBPTDxVpiXpV9n2KA25TBLE882TJjh+TkcwSwWhRGFLtCe4pfwrvV3V0K7oP+hs3N05lBXnfYQ5dbbgEUdLhytIXnTvW8oh5k3ChJbDau+oAr+/sEqRX7/+4YFMrZdhkxqZuVVeKMxzxzPOKUc0ghRnwn6IvpO3uBK1GkemCu28pIH/L9AxE1alRfm2SnN/pF3n1C4LTOVVclObdusFCHZrWp5P0w8Iu3qg6u4vsre51p2+j6D+xQWZ9x+1JXKLo+3YJ4OlAL8nDQXsXpXSovVr849qkXArWEB9X4lghFXgng00wqnexONU1EFf7QNIH2e/qb70bURhyZQoXHnIVdFrr2Uj2vntcORVnbOz05bgCHt4qbFiGqsFl2RAt3FplILg52YUihX7y2n3tKGOuoKqDKfsi/Ak+GiUKkMvHXCyz40GD58C9HsPSMXiKfAOdRnKJaIg1C38DQskfQvZGOkmL0rbSA/VcFDS2f045XaFbTgVpXDU6EMjLBBFxQTT/mSKULx4YhqinkJiDKHXGRtauDBuppj14ZMUmlJTMyT5qn1ri7nRFaLSK4wEn7YjlxQ9jAgokmbis9HN+IX55pqM1Ym9XC9Zwj2vNK47fvjPzuyZolPFQ==
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: 34a33d8b-eaf5-4217-954f-08db27c452f3
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Mar 2023 15:20:36.1200
 (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: 8sbpv4o8bAUvPIOAvJVLkibhABWNmv7zzlbSre5tLlqJiqfa7kAqhFCYNK3BuUAyDhRooXf64geqOOWmFaqWiA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5209
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-18_09,2023-03-16_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 adultscore=0
 mlxlogscore=944 mlxscore=0 bulkscore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303180133
X-Proofpoint-ORIG-GUID: moW1qzE7-ZCOheFsiE2W4D34pON9a3WX
X-Proofpoint-GUID: moW1qzE7-ZCOheFsiE2W4D34pON9a3WX
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 18, 2023, at 6:04 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2023-03-17 at 19:01 -0400, Chuck Lever wrote:
>> A slightly modified take on Jeff's earlier patches, tested with
>> both NFSv3 and NFSv4.1 via simple fault injection in
>> svc_rqst_replace_page().
>>=20
>> In general I'm in favor of more rq_pages bounds checking by
>> replacing direct modification of the rq_respages and rq_next_page
>> fields with accessor functions.
>>=20
>> ---
>>=20
>> Chuck Lever (2):
>>      SUNRPC: add bounds checking to svc_rqst_replace_page
>>      NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_acto=
r()
>>=20
>> Jeff Layton (1):
>>      nfsd: don't replace page in rq_pages if it's a continuation of last=
 page
>>=20
>>=20
>> fs/nfsd/vfs.c                 | 15 +++++++++++++--
>> include/linux/sunrpc/svc.h    |  2 +-
>> include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
>> net/sunrpc/svc.c              | 15 ++++++++++++++-
>> 4 files changed, 53 insertions(+), 4 deletions(-)
>>=20
>> --
>> Chuck Lever
>>=20
>=20
> Looks good, Chuck, thanks. You can add this to the last two:
>=20
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Excellent, thanks!

When I started I expected 3/3 to be more substantial, but since it's
just a handful of lines and the patch descriptions are about the same,
I'm going to squash 2/3 and 3/3 together.

Only question is whether to apply that to nfsd-next or nfsd-fixes.
Since it's a defensive change, I was thinking nfsd-next. Let me know
if you think it should get merged sooner.


--
Chuck Lever