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:     Dan Aloni <dan.aloni@vastdata.com>
CC:     Anna Schumaker <Anna.Schumaker@netapp.com>,
        "trondmy@kernel.org" <trondmy@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trondmy@hammerspace.com>
Subject: Re: [PATCH] NFSD: trim reads past NFS_OFFSET_MAX
Thread-Topic: [PATCH] NFSD: trim reads past NFS_OFFSET_MAX
Thread-Index: AQHYDvfTRZNTkkSWjkWDIwR2Vc39WaxuD5oAgADuzQCAAEhDAA==
Date:   Sat, 22 Jan 2022 17:05:49 +0000
Message-ID: <04E4C6DC-B78F-45DA-871A-296379B2D484@oracle.com>
References: <fa9974724216c43f9bdb3fd39555d398fde11e59.camel@hammerspace.com>
 <20220121185023.260128-1-dan.aloni@vastdata.com>
 <5DD3C5DF-52EF-4C84-894C-FCBB9A0F4259@oracle.com>
 <20220122124710.4l5bzmfxhf2o2yee@gmail.com>
In-Reply-To: <20220122124710.4l5bzmfxhf2o2yee@gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?NjbWzx5cYIe8LxgrMTfN4gjR0ltLlanbgKpolu+Ka6GO8CoJkzfK68L+v/WO?=
 =?us-ascii?Q?10flV5JyIv7kCeSJNlU1wKSlXyGMOTGekhOAxqFlbmA2538BKRT+ltSb4XW1?=
 =?us-ascii?Q?Ip/xRaJ2N6sbVFriLPONcojPHKdvhN4ZSXFrzlarco/lW3hkBfyW/KS3R8K4?=
 =?us-ascii?Q?KgIY5KGmPi8AljGHsaFL91DYAcin3m9hWXlXt9HrFQbSlxl9AvCNLd5c8SB/?=
 =?us-ascii?Q?c5i13wlVJy0C2IXwvCFbwAUqZh35t9oMQf9Fj7/mhK7OowvvSZKRu+Axq3Bw?=
 =?us-ascii?Q?CydNKInUMObldwboxBYzCQyOwJxjDONv2tvBWNjgmlhM6g2hBsKZww3J1aWq?=
 =?us-ascii?Q?vS/KHiMTjqAM2/Oa/GSkSSmym1MKWBlpNfNDlBfrJ9FUjwbhTX3/ZHTOaKmE?=
 =?us-ascii?Q?DM9qExOCyoZKrIAPOAZJYX/6tMK/xApiWnVkRKvC3TdSNY+Mj66HJ82EvnEZ?=
 =?us-ascii?Q?gt1iRDVXammqweCNHV/iLdBUJSc8l3If5mlUo8ofC+afXQwsNKiofjpB7ajr?=
 =?us-ascii?Q?96BGBQta4f4tdIjWmMA02ymWr3lbGAyNc/niz1gz/+tGrUZLy5T1bzNjx5Qb?=
 =?us-ascii?Q?N2FevpUg+PidsGn3fd0Eyamjw+T+zlPrjyju0B05j7fs9R5c9/8caSZ86KXN?=
 =?us-ascii?Q?YVFZd/vtibYvWYGpUD3MhH2aNwagKZDGnDcLvFNVt9iTzCcBaPGd98A4f07a?=
 =?us-ascii?Q?1SlwIDECWDPjKBTQ32B9Ht1aYPJyuifKTgMv+5wPqZxlEa3aD22Itd74ngdu?=
 =?us-ascii?Q?fxD81bIuKRUZOobfdry/7Rn1p7+5DURQvO83sAv/5JVKG6EN8sgPikgB0Nie?=
 =?us-ascii?Q?aYtuC+lLaMxfXVSqc9VADx/Myx3QolunZ2AcDqeuPfnetCAJz68UYT5wN5Dz?=
 =?us-ascii?Q?FzFb0+6O9XaF/9/ACrPB9AEhfwvpmNWej+L0tVLRuV0erXHcKQ4XYZJTu+l6?=
 =?us-ascii?Q?13lRH4bOH1OgC8avxNQ8g0D5DBfzOYVmrNs1QAZWRkTTe2jXx26KM36tDrE4?=
 =?us-ascii?Q?2x/DLmhVTFEQNo760z0xSXe5hG7xha7S/82+PnNa8tUVi+fjvzjSZ8l0JjHa?=
 =?us-ascii?Q?oPqLodXNbf/xyfcTk2QJLx/XumDCvK27AOmSm7p2yY0f3CFx4C1slzUTCIyq?=
 =?us-ascii?Q?ridB9kXfRMlfpWbsXLMRSfQAK/+6CGMqa1phrWq5Mb4NsvBBhJPrhlO3Xwt/?=
 =?us-ascii?Q?GnduNkodH+KLZUAuBDE2qiYxqoFHaZPmjUrTiz+fxQUdLfGS9Ja3sILKIgx7?=
 =?us-ascii?Q?S1ZfJTZhF+s2wqMbRIs0KHngTORf8wMeHDa+S8ZkQNyw3P3rc8nXeNabysL/?=
 =?us-ascii?Q?BQJoXGund2u5Vwcmo7RtljYQKVo1a/ct9gnQv9+X07Y9LqDM2EqCES+CJRTO?=
 =?us-ascii?Q?txgeWK3oJXhXOaRdZk0DMrEfQZJ0YfdNhN7DDKbTxntLTeohY6ntUV8i2pqZ?=
 =?us-ascii?Q?l1cT7V40jMT/Hjz5FO/T3EK4xYPdfNDuKgzbRDJuGfR8TMjwWSsqFQaX6LTy?=
 =?us-ascii?Q?gzfszvxX3TrFD5uPybg7FLWJjP2RfmRkcVd/cZJSeN0Q1B7QRBacuSWUxail?=
 =?us-ascii?Q?mE4JmKBGYyTmyWZpZO4OU8ps4Cc5AsxQRZY5uchwICl2XzApqqUEW6GOAur0?=
 =?us-ascii?Q?T00+J/E9yqm+BgA67Ntw0Gc=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5A6EE41BDAAB72479864EBB5338FC6FA@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: CH0PR10MB4858.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9512a26a-dfe5-49b1-8651-08d9ddc9706f
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jan 2022 17:05:49.3389
 (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: i2XfLXTYLjx7Mzhn2tFXEqXI+G+Q8zzVU+s6zNZ47sj60KDeIBkLMPHTf3DNXsSf2O9tTP8pe8NkjqvhV5B2OQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2939
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10234 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 spamscore=0 adultscore=0 phishscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2201220122
X-Proofpoint-ORIG-GUID: vbtAW8QQCiGWGBvyQgGroVWHBUa6RL_a
X-Proofpoint-GUID: vbtAW8QQCiGWGBvyQgGroVWHBUa6RL_a
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jan 22, 2022, at 7:47 AM, Dan Aloni <dan.aloni@vastdata.com> wrote:
>=20
> On Fri, Jan 21, 2022 at 10:32:28PM +0000, Chuck Lever III wrote:
>>> On Jan 21, 2022, at 1:50 PM, Dan Aloni <dan.aloni@vastdata.com> wrote:
>>>=20
>>> Due to change 8cfb9015280d ("NFS: Always provide aligned buffers to the
>>> RPC read layers"), a read of 0xfff is aligned up to server rsize of
>>> 0x1000.
>>>=20
>>> As a result, in a test where the server has a file of size
>>> 0x7fffffffffffffff, and the client tries to read from the offset
>>> 0x7ffffffffffff000, the read causes loff_t overflow in the server and i=
t
>>> returns an NFS code of EINVAL to the client. The client as a result
>>> indefinitely retries the request.
>>=20
>> An infinite loop in this case is a client bug.
>>=20
>> Section 3.3.6 of RFC 1813 permits the NFSv3 READ procedure
>> to return NFS3ERR_INVAL. The READ entry in Table 6 of RFC
>> 5661 permits the NFSv4 READ operation to return
>> NFS4ERR_INVAL.
>>=20
>> Was the client side fix for this issue rejected?
>=20
> Yeah, see Trond's response in
>=20
>   https://lore.kernel.org/linux-nfs/fa9974724216c43f9bdb3fd39555d398fde11=
e59.camel@hammerspace.com/
>=20
> So it is both a client and server bugs?

Splitting hairs, but yes there are issues on both sides
IMO. Bad behavior due to bugs on both sides is actually
not uncommon.

Trond is correct that the server is not dealing totally
correctly with the range of values in a READ request.

However, as I pointed out, the specification permits NFS
servers to return NFS[34]ERR_INVAL on READ. And in fact,
there is already code in the NFSv4 READ path that returns
INVAL, for example:

 785         if (read->rd_offset >=3D OFFSET_MAX)
 786                 return nfserr_inval;

I'm not sure the specifications describe precisely when
the server /must/ return INVAL, but the client needs to
be prepared to handle it reasonably. If INVAL results in
an infinite loop, then that's a client bug.

IMO changing the alignment for that case is a band-aid.
The underlying looping behavior is what is the root
problem. (So... I agree with Trond's NACK, but for
different reasons).


>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 738d564ca4ce..754f4e9ff4a2 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1046,6 +1046,10 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct =
svc_fh *fhp,
>>> 	__be32 err;
>>>=20
>>> 	trace_nfsd_read_start(rqstp, fhp, offset, *count);
>>> +
>>> +	if (unlikely(offset + *count > NFS_OFFSET_MAX))
>>> +		*count =3D NFS_OFFSET_MAX - offset;
>>=20
>> Can @offset ever be larger than NFS_OFFSET_MAX?
>=20
> We have this check in `nfsd4_read`, `(read->rd_offset >=3D OFFSET_MAX)`.
> (should it have been `>` rather?).

Don't think so, a zero-byte READ should be valid.

However it's rather interesting that it does not use
NFS_OFFSET_MAX here. Does anyone know why NFSv3 uses
NFS_OFFSET_MAX but NFSv4 and NLM use OFFSET_MAX?


--
Chuck Lever