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:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v2 2/5] NFSD: Fix NFSv3 SETATTR/CREATE's handling of large
 file sizes
Thread-Topic: [PATCH v2 2/5] NFSD: Fix NFSv3 SETATTR/CREATE's handling of
 large file sizes
Thread-Index: AQHYFtAIqaz3sFLzU0KCmTXzQ7oeOqx9dbEAgAADKYA=
Date:   Mon, 31 Jan 2022 18:49:13 +0000
Message-ID: <A3A5CC01-BDDB-4C14-A164-1AA3753DEA11@oracle.com>
References: <164365324981.3304.4571955521912946906.stgit@bazille.1015granger.net>
 <164365349299.3304.4161554101383665486.stgit@bazille.1015granger.net>
 <cb06de6582d9a428405af43d0cb92e0c2d04c76f.camel@hammerspace.com>
In-Reply-To: <cb06de6582d9a428405af43d0cb92e0c2d04c76f.camel@hammerspace.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?vZF6Wi+DI6VxY1e2QLNb9Gnkvd7mPuCBJJkf0iKcOsYQszi3mzJo5KgwKtnT?=
 =?us-ascii?Q?Q8rHoH2Kt0jaY/NjBBPYwQDes4nVApIjrY0xQQKdAYiossBJzWPrEGADCG1w?=
 =?us-ascii?Q?XmMwh2sWcpSvjwJNW83KDVDoMM2mFy3ca8u3MYCHsU8dAmloW3lipejJg6PV?=
 =?us-ascii?Q?P7chq58kIm11aw4SrhNKwxBLKvr+HcZ64YaTy/oJW9WVOKlgK3AwxXt9nxfF?=
 =?us-ascii?Q?GVCfdszS7qtFWbCZwUcjovCOlILtRJKhcS/GvHlXbxTuIEMB4N9EXrLVlq/a?=
 =?us-ascii?Q?z8PBi0sHnW1eGriZKR4DrtySvB7iZzwwptKG4As4ytSg5W6ROEGPb3GCfDwB?=
 =?us-ascii?Q?6swCgQ270Pn9u4U+rhzc7wpUQ0qz8wPBmsl3LJV8A4dY/rl3KL6NYwXxlG8t?=
 =?us-ascii?Q?v6lM0lE+lwHbuZOy/FqEce7yj2L7J/VZrNmBkTDb3zAobCouCcjZZLPoNwJn?=
 =?us-ascii?Q?zPp9spg0/KkYEXvUfNFoSSqcRs18uK2MTyIOvUPh/+W3TKyhqu2NmA/EbSCv?=
 =?us-ascii?Q?ZCD8ZE/HpvkeF/k0d9C0rWErU3Lz3BiJ9g2ameTI7MKTtsQuhJPZzsh4S4Nd?=
 =?us-ascii?Q?7LXdFBfEwf5lVeeUryzhMMu8odnDLEQu9POOX1Ryg/Iw656U7R/GXmyhsmCH?=
 =?us-ascii?Q?0ZK1QKed2TjKco5PmP8gzHCb9BH6f9KPGqGU6E3KSO6ojArbDXq15bNeqtq8?=
 =?us-ascii?Q?is0rthvbFCCqtvpjZCF3PWv87/xWvjfQgaixIMsbKLezllNhRoC9bglsOhAF?=
 =?us-ascii?Q?+PRNgQURIUF72usYx2zSi5e9a9eQUBVcXxVAH4Vi1ZwJz8ZEXC9AfIfhI5nR?=
 =?us-ascii?Q?hwhuZNZvPtZft1/Hm1CfY6OeopwZ6nI+YAcifquNXdufaOr+r6cnKrBS/iQt?=
 =?us-ascii?Q?bQX7HeaJmt7swRVIWKKWAxNFGwjHc/J4kR95zz9XpAipAVcWPTgVhzSU6Hzw?=
 =?us-ascii?Q?zn2YzS5CVoHr2XX3bWEM7Kp3TAC7tHI1NdCH9agx0yeXHBJE5GAQObKaQvrs?=
 =?us-ascii?Q?xGZOxp4QdwWaBFmc4LjR0PqIyozeW+lQbznOZ58mY8S+Dyy3NhgLjd9DHN/7?=
 =?us-ascii?Q?GhcW/1pUR6ziQCYKzlF5VcMEdnlC+0/xFGScJkLRU9uf26Dl8djzuplf98RB?=
 =?us-ascii?Q?jpvPHimryc1lDPV4wi9ZXi4U51Xh5zk9kJ1icewduNXCVAx6UdSxmWE3nfw7?=
 =?us-ascii?Q?C6TDmgVySotJar4zxElKkfri+bERuMlHq2EGcKvfzgvkjLqBd/c+8jhQM4zL?=
 =?us-ascii?Q?m4FHms8lpRHhB2oM5uP0KORvr86w24YqofRtCaugerLpBP9GvPjFXzoStO4R?=
 =?us-ascii?Q?f7uJ+oe5EH6k3TwGyD0f/sO4eE4PqDiAYSM2v9PIaS3rjG0m65LbmjXaVsoW?=
 =?us-ascii?Q?0AM4q249S5W1KO4IlhDNy53Oay2oOIMJJ6nsw07on4cRMvm0Mke56ixysZth?=
 =?us-ascii?Q?LifumxwrJuSkHfAN+n1JuJajMU+/Q+yQ2bMWRm4s1CMN6EenM0DPXLoksxHK?=
 =?us-ascii?Q?Lgdd1h4j03/U/iETR2Wf0kpFmvrvnQIsNbHZgiMAahwtuR1G2fHamGetxpji?=
 =?us-ascii?Q?fzOABzpyCFxURxJS/371He1fLM+ypGN4g1FD8hHjsXzB7xeTtFEh/tVTIQ6h?=
 =?us-ascii?Q?BOsCPEhPa65H40ObE3oj1x8=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <106049B2681F2F4797E332EF06734370@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: 000157d8-eb1f-44ee-9823-08d9e4ea6020
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Jan 2022 18:49:13.5083
 (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: p2ZhVM6V4ihoRq5L+4RfCWo2UliFi4+ioReUkiZ5UYf0kD5g/gpb0kSk5w6hNtxRyfUBq9vLhndMNMq0Z5s5TQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2748
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10244 signatures=673430
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 phishscore=0
 mlxscore=0 adultscore=0 suspectscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2201310121
X-Proofpoint-GUID: 2CQDdHN_39cicMwdYeQSPofGQHu7kLsm
X-Proofpoint-ORIG-GUID: 2CQDdHN_39cicMwdYeQSPofGQHu7kLsm
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jan 31, 2022, at 1:37 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Mon, 2022-01-31 at 13:24 -0500, Chuck Lever wrote:
>> iattr::ia_size is a loff_t, so these NFSv3 procedures must be
>> careful to deal with incoming client size values that are larger
>> than s64_max without corrupting the value.
>>=20
>> Silently capping the value results in storing a different value
>> than the client passed in which is unexpected behavior, so remove
>> the min_t() check in decode_sattr3().
>>=20
>> Moreover, a large file size is not an XDR error, since anything up
>> to U64_MAX is permitted for NFSv3 file size values. So it has to be
>> dealt with in nfs3proc.c, not in the XDR decoder.
>>=20
>> Size comparisons like in inode_newsize_ok should now work as
>> expected -- the VFS returns -EFBIG if the new size is larger than
>> the underlying filesystem's s_maxbytes.
>>=20
>> However, RFC 1813 permits only the WRITE procedure to return
>> NFS3ERR_FBIG. Extra checks are needed to prevent NFSv3 SETATTR and
>> CREATE from returning FBIG. Unfortunately RFC 1813 does not provide
>> a specific status code for either procedure to indicate this
>> specific failure, so I've chosen NFS3ERR_INVAL for SETATTR and
>> NFS3ERR_IO for CREATE.
>>=20
>> Applications and NFS clients might be better served if the server
>> stuck with NFS3ERR_FBIG despite what RFC 1813 says.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfsd/nfs3proc.c |    9 +++++++++
>>  fs/nfsd/nfs3xdr.c  |    2 +-
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 8ef53f6726ec..02edc7074d06 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -73,6 +73,10 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
>>         fh_copy(&resp->fh, &argp->fh);
>>         resp->status =3D nfsd_setattr(rqstp, &resp->fh, &argp->attrs,
>>                                     argp->check_guard, argp-
>>> guardtime);
>> +
>> +       if (resp->status =3D=3D nfserr_fbig)
>> +               resp->status =3D nfserr_inval;
>> +
>>         return rpc_success;
>>  }
>> =20
>> @@ -245,6 +249,11 @@ nfsd3_proc_create(struct svc_rqst *rqstp)
>>         resp->status =3D do_nfsd_create(rqstp, dirfhp, argp->name,
>> argp->len,
>>                                       attr, newfhp, argp->createmode,
>>                                       (u32 *)argp->verf, NULL, NULL);
>> +
>> +       /* CREATE must not return NFS3ERR_FBIG */
>> +       if (resp->status =3D=3D nfserr_fbig)
>> +               resp->status =3D nfserr_io;
>> +
>>         return rpc_success;
>>  }
>> =20
>> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
>> index 7c45ba4db61b..2e47a07029f1 100644
>> --- a/fs/nfsd/nfs3xdr.c
>> +++ b/fs/nfsd/nfs3xdr.c
>> @@ -254,7 +254,7 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp,
>> struct xdr_stream *xdr,
>>                 if (xdr_stream_decode_u64(xdr, &newsize) < 0)
>>                         return false;
>>                 iap->ia_valid |=3D ATTR_SIZE;
>> -               iap->ia_size =3D min_t(u64, newsize, NFS_OFFSET_MAX);
>> +               iap->ia_size =3D newsize;
>>         }
>>         if (xdr_stream_decode_u32(xdr, &set_it) < 0)0
>>                 return false;
>>=20
>>=20
>=20
> NACK.
>=20
> Unlike NFSV4, NFSv3 has reference implementations, not a reference
> specification document. There is no need to change those
> implementations to deal with the fact that RFC1813 is underspecified.
>=20
> This change would just serve to break client behaviour, for no good
> reason.

So, I _have_ been asking around. This is not a change that
I'm proposing blithely.

Which part of the change is wrong, and which clients would
break? Solaris NFSv3 server is supposed to return NFS3ERR_FBIG
in this case, I believe. NFSD could return NFS3ERR_FBIG in
these cases instead.

Is there somewhere that the behavior of the reference
implementation is documented? If the current XDR decoder
behavior is a de facto standard, that should be noted in a
comment here.


--
Chuck Lever