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>
Subject: Re: [PATCH RFC 1/4] NFS: Unset RPC_TASK_NO_RETRANS_TIMEOUT for async
 lease renewal
Thread-Topic: [PATCH RFC 1/4] NFS: Unset RPC_TASK_NO_RETRANS_TIMEOUT for async
 lease renewal
Thread-Index: AQHXeMf61G05eO8qbEGbWIAqZ3VMZqtCoTqAgAAflIA=
Date:   Wed, 14 Jul 2021 17:53:44 +0000
Message-ID: <844DA52F-49E6-4F4E-B7BC-CDDD4620BEFD@oracle.com>
References: <162627611661.1294.9189768423517916152.stgit@manet.1015granger.net>
 <162627781762.1294.17862468684529354297.stgit@manet.1015granger.net>
 <2debf0bf8076dd81d7aa413a31c32fe48ad06961.camel@hammerspace.com>
In-Reply-To: <2debf0bf8076dd81d7aa413a31c32fe48ad06961.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?AsFLEjGr+YBi5gjDi/J7bvjpPxfBVHPWiZn+60q5XvVCLPW1dys9xVSOu2g/?=
 =?us-ascii?Q?8wAubMDVVuPACI5HQtSC6r6vbSVEgyWW1gC5ydT39GeLQY92IC3/RWMk0Ize?=
 =?us-ascii?Q?u3b12g22OmRgxisphI9xHWC3KnUfnJMqhHu7RfShg96Ad2lwRrCFEqFNzKlW?=
 =?us-ascii?Q?DDFrd1e0kO0Sv+8iQchUyLJ39E0WokI8sL/tdA32fxvZMAbHEGU/DnBEN6EF?=
 =?us-ascii?Q?voQP5cLCrnnwxL+yAAwU/h8xykiD0ijMgmltXgyFeLqs1dLpZgugjwStjlHt?=
 =?us-ascii?Q?1MyLe8n2wDn2/LfeMGY1MGQE2h14oFjHqc8M+4JM+nCpBLhmIMfMKNk7gv8t?=
 =?us-ascii?Q?7KysnxQ+2BtSXCp4PHfRKUBXeTr5e07CEvbnpW+kac0nH13jwPlLsgek64B0?=
 =?us-ascii?Q?fWi4FwAjlt6o0emv/W0t/ADawQVEZODg6MxkCq90PoShfmRXT7137SG+oMOE?=
 =?us-ascii?Q?NZ2NqZsdUtasSC3T5tEyFn2Wux6YT4GM75BHsV3tzRQ2ObtVFTXU6ckyfm+e?=
 =?us-ascii?Q?RUnakEl36wwxmXxJfwOS8qDc6Wq/qo67W7i0gBKTZQG82srAO5ojHnnYf53r?=
 =?us-ascii?Q?LkkRVXJ3oDenj/7d5dl3tteqtUA6ToEDo559ZQNCHSRp7FtZWWDXCzDVk4/1?=
 =?us-ascii?Q?mm+xubFTTdz8rgFdhW2XkfhDmYqpk/r8lJgnrh+4P6DezCDeR36PpP4aZLvJ?=
 =?us-ascii?Q?Tn0wAqY2stldZZaVbWN1E9NAw5Hxs4YFUcfwQuxCqorYZSpFJP1DH1igq3q9?=
 =?us-ascii?Q?x5NvcCll/xiRkexHNTGeBHIcjUnfI8Ybk4jaqfeRn+vR9WYxYZIPsnXOzlQZ?=
 =?us-ascii?Q?iGKj1DqUetx7y+pEKXnAjmgSpUVN8ws5j21WufZXYWbBqePTra+Lp9fXPsMT?=
 =?us-ascii?Q?IHZATOsI1iqHKKMjzjMq9j/7trr2oNZEJTV/6skNdnwb10Ad7sFr2PPb4d7+?=
 =?us-ascii?Q?wNErzLsGAfMtH8jPdog2YwXo6g4L9UFDBiO5sS3N37mY8uaz3j2vaHIe+sMC?=
 =?us-ascii?Q?aPGSwUZOoXBu1k3GMsmHoXAd/YkAeyVLxZpMjh8zNylIScYROdzBT9VnAh1r?=
 =?us-ascii?Q?egMRh/EohD0Se1R7ThqcinGpNfWjlxyKATIfB7Xy0EvKoHOsyEreLDJfVRmP?=
 =?us-ascii?Q?hrgV0cw7E7b+CRed175DIZ4A0c1bhdGpoyAvS6VvzhV70OGZRvD1d2F2Mftp?=
 =?us-ascii?Q?rP0IO/58sTaYM9eF3yyi/rJ4ZM5L37DSoR+ltpqq25J92P/5e9EBLzmIhmMq?=
 =?us-ascii?Q?l3kkcLtBjy1x5ocV8hP8ibtFZ6jo1AMgHDNvDiUPBEgOpC+0Qku0Rn13Zd/7?=
 =?us-ascii?Q?wAQOo90AS8FTxslP2WfWeYEf?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <82BBF328E42D6A48A01C25AAF1D33E8E@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: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 09c76ec8-6d33-45b1-8451-08d946f0530e
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jul 2021 17:53:44.8404
 (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: Fw9ech4HU4SWq3wGf4YaoxpoyDL5gjcbBJ/SVW8T+O1+lfeglKDq0Zguq2rEb+AK3TvuXVDdMuYmfxLIP17IKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2968
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10045 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 spamscore=0 suspectscore=0 adultscore=0 mlxlogscore=999 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2107140106
X-Proofpoint-ORIG-GUID: eu2H78HNkLzm4I4tB2O2oaRhf2kcZkCA
X-Proofpoint-GUID: eu2H78HNkLzm4I4tB2O2oaRhf2kcZkCA
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 14, 2021, at 12:00 PM, Trond Myklebust <trondmy@hammerspace.com> w=
rote:
>=20
> On Wed, 2021-07-14 at 11:50 -0400, Chuck Lever wrote:
>> In some rare failure modes, the server is actually reading the
>> transport, but then just dropping the requests on the floor.
>> TCP_USER_TIMEOUT cannot detect that case.
>>=20
>> Prevent such a stuck server from pinning client resources
>> indefinitely by ensuring that async lease renewal requests can time
>> out even if the connection is still operational.
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfs/nfs4proc.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>=20
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index e1214bb6b7ee..346217f6a00b 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -5612,6 +5612,12 @@ struct nfs4_renewdata {
>>   * nfs4_proc_async_renew(): This is not one of the nfs_rpc_ops; it
>> is a special
>>   * standalone procedure for queueing an asynchronous RENEW.
>>   */
>> +static void nfs4_renew_prepare(struct rpc_task *task, void
>> *calldata)
>> +{
>> +       task->tk_flags &=3D ~RPC_TASK_NO_RETRANS_TIMEOUT;
>> +       rpc_call_start(task);
>> +}
>> +
>>  static void nfs4_renew_release(void *calldata)
>>  {
>>         struct nfs4_renewdata *data =3D calldata;
>> @@ -5650,6 +5656,7 @@ static void nfs4_renew_done(struct rpc_task
>> *task, void *calldata)
>>  }
>> =20
>>  static const struct rpc_call_ops nfs4_renew_ops =3D {
>> +       .rpc_call_prepare =3D nfs4_renew_prepare,
>>         .rpc_call_done =3D nfs4_renew_done,
>>         .rpc_release =3D nfs4_renew_release,
>>  };
>> @@ -9219,6 +9226,8 @@ static void nfs41_sequence_prepare(struct
>> rpc_task *task, void *data)
>>         struct nfs4_sequence_args *args;
>>         struct nfs4_sequence_res *res;
>> =20
>> +       task->tk_flags &=3D ~RPC_TASK_NO_RETRANS_TIMEOUT;
>> +
>>         args =3D task->tk_msg.rpc_argp;
>>         res =3D task->tk_msg.rpc_resp;
>=20
> This isn't necessary. The server isn't allowed to drop these calls on
> the floor.

Again, a server bug, a misconfiguration, a dependence on an
inoperative network service (like DNS), a weird crash, or even
a malicious server can pin client resources. This is plainly a
denial of service. Clients cannot depend on "the server is not
allowed to" if they are to be considered secure.

I'm not suggesting that the Linux client should make a heroic
effort to operate normally when a server behaves like this. I
_am_ suggesting that the client should protect itself and its
users by not pinning its own resources when a server is
behaving bizarrely.

I can drop 1/4 & 2/4 for the moment, but I don't agree that
3/4 & 4/4 alone are adequate to resolve the denial of service.

----

Now with regard to the spec requirement, I believe it might
be under-specified. It's at least problematic.

This is from RFC 8881 Section 2.9.2, and is referring in
particular to non-NULL NFSv4 operations:

> A replier MUST NOT silently drop a request, even if the request is a retr=
y. (The silent drop behavior of RPCSEC_GSS [4] does not apply because this =
behavior happens at the RPCSEC_GSS layer, a lower layer in the request proc=
essing.) Instead, the replier SHOULD return an appropriate error (see Secti=
on 2.10.6.1), or it MAY disconnect the connection.


It states that the server cannot drop a request, but the text
does /not/ literally mandate that the only signal for a lost
request is connection loss -- that part is only a MAY.

Further the use of SHOULD suggests that a session error is the
preferred mechanism for signaling such a loss. The use of MAY
indicates that connection termination is permissible but not
preferred.

Moreover, the text is not careful to state that these two options
are exhaustive. It leaves open the possibility for other server
responses in this situation. (I recognize that might not have
been the intent of the authors, but that's certainly how it
reads a decade later).

Let's not even get into the carve-out for RPCSEC_GSS silent drops.

Thus I don't think we can lean on the current spec to ensure that
a server will always signal the loss of a reply by disconnecting,
especially in cases where there is no session. Further down in
ss2.9.2, we have this one-sentence paragraph:

> In addition, as described in Section 2.10.6.2, while a session is active,=
 the NFSv4.1 requester MUST NOT stop waiting for a reply.


So what about when there is no active session? For example, what
about EXCHANGE_ID, CREATE_SESSION, BIND_CONN_TO_SESSION, and
DESTROY_CLIENTID ? I would argue that a high quality client
implementation will not wait indefinitely for the completion of
these operations.


--
Chuck Lever