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

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]

Message-ID: <CALbTx=H_49MroKwuwyThirwtiJE5686cyCZwKth-yVSRx0srug@mail.gmail.com>
Date:   Tue, 17 Dec 2019 18:12:16 +0000
From:   Robert Milkowski <rmilkowski@...il.com>
To:     Trond Myklebust <trondmy@...merspace.com>
Cc:     "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] NFSv4: nfs4_do_fsinfo() should not do implicit lease renewals

On Mon, 16 Dec 2019 at 18:58, Trond Myklebust <trondmy@...merspace.com> wrote:
>
> -On Mon, 2019-12-16 at 18:43 +0000, Robert Milkowski wrote:
> > > From: Trond Myklebust <trondmy@...merspace.com>
> > ...
> > > NACK. The above argument only applies to legacy minor version 0
> > > setups, and does not apply to NFSv4.1 or newer.
> >
> > Correct. However many sites still use v4.0.
> >
>
> That's not a good reason to break code that works just fine for
> NFSv4.1.
>

Of course not, that's not what I meant and I misunderstood your nack too.

> It would be better to move the initialisation of clp->cl_last_renewal
> into nfs4_init_clientid() and nfs41_init_clientid() (after the calls to
> nfs4_proc_setclientid_confirm() and nfs4_proc_create_session()
> respectively).
>

This could be done but this is potentially a separate change, as in
nfs4_do_fsinfo() we still need to
make sure we do not implicitly renew lease for v4.0, so I think the
patch needs to be modified as:

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

This way it won't affect newer nfs versions (I don't think it affected
them in any bad way, still it was unintended and not correct).
Agree with the above change?

btw: Chuck, thanks for the hint regarding the SEQUENCE op on v4.1+