linux-kernel - Re: [PATCH v2] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Dec 2019 13:32:24 -0500
From:   Chuck Lever <chuck.lever@...cle.com>
To:     Robert Milkowski <rmilkowski@...il.com>
Cc:     Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna.schumaker@...app.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease

Hi Robert-

> On Dec 24, 2019, at 9:36 AM, Robert Milkowski <rmilkowski@...il.com> wrote:
> 
> From: Robert Milkowski <rmilkowski@...il.com>
> 
> Currently, each time nfs4_do_fsinfo() is called it will do an implicit
> NFS4 lease renewal, which is not compliant with the NFS4 specification.
> This can result in a lease being expired by an NFS server.

In addition to stating the bug symptoms, IMO you need

Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases")

And this description needs to explain how 83ca7f5ab31f broke things.


> Signed-off-by: Robert Milkowski <rmilkowski@...il.com>
> ---
> fs/nfs/nfs4proc.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 76d3716..b6cad9a 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5016,19 +5016,23 @@ static int _nfs4_do_fsinfo(struct nfs_server *server, struct nfs_fh *fhandle,
> 
> static int nfs4_do_fsinfo(struct nfs_server *server, struct nfs_fh *fhandle, struct nfs_fsinfo *fsinfo)
> {
> +	struct nfs_client *clp = server->nfs_client;
> 	struct nfs4_exception exception = {
> 		.interruptible = true,
> 	};
> -	unsigned long now = jiffies;
> +	unsigned long last_renewal = jiffies;
> 	int err;
> 
> 	do {
> 		err = _nfs4_do_fsinfo(server, fhandle, fsinfo);
> 		trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err);

There are two usual practices to introduce behavior that diverges
amongst NFSv4 minor versions. Neither practice is followed here.

- The difference is added to the XDR encoder and decoder. You could
do that by adding a RENEW to the COMPOUND in the NFSv4.0 case.

- The function is converted to a virtual function which is added to
struct nfs4_minor_version_ops.

Prior to 83ca7f5ab31f, IIRC this function was part of a code path
that did actually renew the lease. It seems to me that the proper
fix here is to make NFSv4.0 renew the lease, not the other way
around. Is there a reason not to add a RENEW to the NFSv4.0 version
of the fsinfo COMPOUND?


> 		if (err == 0) {
> -			nfs4_set_lease_period(server->nfs_client,
> +			/* no implicit lease renewal allowed here for v4.0 */
> +			if (clp->cl_minorversion == 0 && clp->cl_last_renewal != 0)
> +				last_renewal = clp->cl_last_renewal;
> +			nfs4_set_lease_period(clp,
> 					fsinfo->lease_time * HZ,
> -					now);
> +					last_renewal);
> 			break;
> 		}
> 		err = nfs4_handle_exception(server, err, &exception);
> -- 
> 1.8.3.1

--
Chuck Lever