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>,
        Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v2 2/2] sunrpc: add bounds checking to
 svc_rqst_replace_page
Thread-Topic: [PATCH v2 2/2] sunrpc: add bounds checking to
 svc_rqst_replace_page
Thread-Index: AQHZWPPHhxprfvZio0CVZD7mRL0pxa7/QGmAgAADkYCAAAD6gIAADlIAgAAgfAA=
Date:   Fri, 17 Mar 2023 20:55:41 +0000
Message-ID: <27346F71-367D-42DF-85AB-D168012F5C41@oracle.com>
References: <20230317171309.73607-1-jlayton@kernel.org>
 <20230317171309.73607-2-jlayton@kernel.org>
 <6F8F3C24-A043-443A-BBB7-E4788FBE29C9@oracle.com>
 <827d46876b57bec309164d4c9513bac523ad5843.camel@kernel.org>
 <F6CE9F70-628B-44D3-A8B1-D4EEBBA28B87@oracle.com>
 <c78fff223129f289214ceada0d82f6952e0d3a82.camel@kernel.org>
In-Reply-To: <c78fff223129f289214ceada0d82f6952e0d3a82.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?Bz6/7LgwKqMCJiZBV51RzKxB7cjcADDSzLrhvNXTdtzYE1qOxSAM46XPFxwb?=
 =?us-ascii?Q?osGWqMXty7QleHobSJdMCgcyTUdNYzpicsgXhLxaJC0RWjAsTfR1JR8MO2Sy?=
 =?us-ascii?Q?MX3nLorFBcHVVgywhB1RO41pC8bx5EOyn1PNo6roA1HL+868w9AFl2/HwgMm?=
 =?us-ascii?Q?Pf9EiiWpVsFtIkwH4bYIxSByg5yBD+UJ62OtJAE/7E+fD0KqmonlT917001h?=
 =?us-ascii?Q?zxIsa1nsI30qkSBGQfsynvvgzxao6zbZNzChoTJ9CrKCw2NCOggfOuynrT+n?=
 =?us-ascii?Q?0KVTsjOyym56g5RAJ2vZ3GxOHst+13FugIv+NGEuF9WVUCCUsQJtLCbzSOOx?=
 =?us-ascii?Q?6FeFhsKLTc2zDh5YocucZGsa/MdX6yfj9QhaHX8WJV3bjiUGLyMj0VzegFp8?=
 =?us-ascii?Q?HioQmicha0A4pgegD2zY4PMbEFkp03NRDDe/AeZ13QEg1AjkHwvXWiCtxPIe?=
 =?us-ascii?Q?onH/9Hj0jW2BmSUJPTvyyBdVBv0ckAvRXZAmrTx9l1tSQ5eB9gDpSiDV4j+r?=
 =?us-ascii?Q?CLGTUg4EVwYxvyr+j7lOGzyiuJp/V45j0YgcfgpDgBJ6Ryj1l+488Dbf+asB?=
 =?us-ascii?Q?4iCtm6Ih9FK9OzTmnoD5BmuEfWusy1um6iO51lO7Se9LA6J+8rPqen0PzpRH?=
 =?us-ascii?Q?xlmkhuKEOcRdg6j/NOSzTa29Z9iYkYHTBUUYfEgbkLD6UzQ5hdIomLbxZIIi?=
 =?us-ascii?Q?a3krEeWpv8/VTPQp/pxgfrzgWpu60LopyD+BIYKBLUjRj8MH9Q3y3otXoqoa?=
 =?us-ascii?Q?SajwQCSd9m2/qudBhX0UMoaYnFoeEhmHUMl0SYiRhPur2ut/gaMhClWUZMpF?=
 =?us-ascii?Q?EIy+/hztaDR9Nt/Gl6xEfWGdq44ZLzNtnNsRDAg8tM6M7PLrZ933q/LyX9yg?=
 =?us-ascii?Q?hZ2+yw8s3K1umz0dVTkIeT+xTR76Y6wFSRiPBVJqZcpxZntp+RFXsTMy89bj?=
 =?us-ascii?Q?6utmjuunOa7yuCV6xkA3YwJWwCAqxmC3b0nukZyKHkZsGFBkqSOO82YTJXV4?=
 =?us-ascii?Q?xscQ2kRrwCEBD7ipQgBEzPVr70VznzijvIPRhsEiq1cxYhgvlUGrFz9oqILi?=
 =?us-ascii?Q?/2eFvTyG4Bg679JaWGEdMtOKMQJXj9BYXJOybSnLaD+ZMgAmre4qA2I4B/3h?=
 =?us-ascii?Q?MWJV7kbpPSL+/9H0zlK8AyzzOUdDkyASm7BQzge/WYQOh/dzB/YgLzzak3Rr?=
 =?us-ascii?Q?LCUGph+QJ323iECwvP7yGFDdEwpaWN+177isAtgtbqlHdX0Hicr7PfY7o338?=
 =?us-ascii?Q?8agOnCidmKIo44GJHEsoRUIKrMbJ2Ase/wRD/be4eEYxXbCgl+Vx/BUU9YXr?=
 =?us-ascii?Q?FFel05fNl0HSXnQ9lFQCXK7XgfXIPJRd3tuiowJNZfuMGraugY2OXcceCq16?=
 =?us-ascii?Q?6h1++Udt6PRJiWZ8s9Up8GAI9Kr/UghKzX1J9OKWAR2dTEc0PM5kod4lwxfg?=
 =?us-ascii?Q?uuBjjiJWbJKL3aYcaA6SgqJDLo8VoG2Z982QFW+f0Br6xod/s29/KTTIiTEa?=
 =?us-ascii?Q?SZmVXHTvBnVBgyscNIBCIwlF5KnYsw4k3+O6rnaFRX2NNATNIVio2XGTRpRd?=
 =?us-ascii?Q?Qz8qFRCmJvOLnEBSoDppsfgGm+EuV7QeWp5L+CmvajSQNVXMZqJd58oSrLzh?=
 =?us-ascii?Q?+w=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6F6FC3C3DE92C9469EE0D2AD4E1E0220@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: 547G/s6j+DevsplhKnoofsNX4F0uHdpRX50StmyFEuvlTALRsknXW6zBk/x574FSHbwEViBRRsht2/bHXcYI3mHzht9QTHuo7g0FCVaDrHkfh6/oOd82OA8K+rwu2GcaQ5ZaG3eb2K0XUewwp0rbQdRlW2bI5F8t8Whfkdtb5ax0a7VRMFEdKLGzLniLDB7D5stRSY/AbEi/4jwjfGVnkm6Ol/aq2SBvXuYRfHLnp4Qbn6ItrwvQOlA5UXL7BNSglusR0oU+j/jy9M5bGDevIX55v7I0yzbdClrpxW56hpSn1xPe/lpOm3LBb5Q5vhuOhBEcSfvSQGmCWmCUTqEGSaFLt6Rp14M5XPjUrR/co7gJV3oSAaXu/+kvA6Lx7yWT+LCBGlf8pQfJCmSJU8jIxJDaw5/AddELDcf+IKRaLZqeaHg6k3/MYV+/MYemzFaPnnMJZjmJwXUD74Q8mPA5QS+bonocFCG85+E14iyU9vhmC3IS7HRYUPHk1UfocQ5Nsc9hPwoyzt9aIeUNnD3nKt099ndK570w80W031pzQkenVnzKQKYaj+34GfAvTndscYcVP16WH0MzhEF4w29oAlBnSVpe0df1os0ULMC5OEBgQtBixQmCA8ZsipqR07LSg5OiTvuTqVr/bHm2BhKngXA8fS2BKMQJDZSPV7aMvpBJMbjolaJQl7rdT4Rwae70e1nUw7anKHfJqtE7nDPyktO/p1qncfeS01ekKldelLkfR5ije1P9HHzJeap9plT8xkM1i9GbukteHe9SiD6KfeVkqeWtH5jXK7iU/7mL7DcK546tileZEZ8A0/OQRFgLb/Q1F/KyBNMwpS+axHUZPJSzvuz21jMlKgU/Phze17rIinvW1YdCMiuxZgDKMLxhFlDouYKvQZIJULZUBrmBsw==
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: b2ffe162-447c-488f-f0f5-08db2729f81b
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2023 20:55:41.2467
 (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: cEtMEB99o5SK8brcOO2XNForbom+aYVFkMof/npCo2PNUVkmMQ5CVuPaJ5+zvG5H2oXq3i2W1Zfkghfs/oIVlw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5820
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_17,2023-03-16_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999
 bulkscore=0 mlxscore=0 adultscore=0 suspectscore=0 phishscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303150002 definitions=main-2303170147
X-Proofpoint-GUID: 1VRcTp2b-75cJeJKdfO-5iUHMd9YSt2O
X-Proofpoint-ORIG-GUID: 1VRcTp2b-75cJeJKdfO-5iUHMd9YSt2O
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 17, 2023, at 2:59 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2023-03-17 at 18:08 +0000, Chuck Lever III wrote:
>>=20
>>> On Mar 17, 2023, at 2:04 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> On Fri, 2023-03-17 at 17:51 +0000, Chuck Lever III wrote:
>>>>> On Mar 17, 2023, at 1:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>>=20
>>>>> If rq_next_page ends up pointing outside the array, then we can
>>>>> corrupt
>>>>> memory when we go to change its value. Ensure that it hasn't strayed
>>>>> outside the array, and have svc_rqst_replace_page return -EIO
>>>>> without
>>>>> changing anything if it has.
>>>>>=20
>>>>> Fix up nfsd_splice_actor (the only caller) to handle this case by
>>>>> either
>>>>> returning an error or a short splice when this happens.
>>>>=20
>>>> IMO it's not worth the extra complexity to return a short splice.
>>>> This is a "should never happen" scenario in a hot I/O path. Let's
>>>> keep this code as simple as possible, and use unlikely() for the
>>>> error cases in both nfsd_splice_actor and svc_rqst_replace_page().
>>>>=20
>>>=20
>>> Are there any issues with just returning an error even though we have
>>> successfully spliced some of the data into the buffer? I guess the
>>> caller will just see an EIO or whatever instead of the short read in
>>> that case?
>>=20
>> NFSv4 READ is probably going to truncate the XDR buffer. I'm not
>> sure NFSv3 is so clever, so you should test it.
>=20
> Honestly, I don't have the cycles to do that sort of fault injection
> testing for this.

nfsd_splice_actor() has never returned an error, so IMO it is
necessary to confirm that when svc_rqst_replace_page() returns
an error, it doesn't create further problems. I don't see how
we can avoid some kind of simple fault injection while developing
the fix.

Tell you what, I can take it from here if you'd like.


> If you think handling it as a short read is overblown,
> then tell me what you would like see here.

It's not the short reads that bugs me, it's the additional
code in a hot path that is worrisome.

--
Chuck Lever