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] [day] [month] [year] [list]

Message-ID: <CALbTx=HUA6PdBufwmyYaN3J1pETUzRk=5HM7BKCDv8+PM2Qm9Q@mail.gmail.com>
Date:   Wed, 18 Dec 2019 14:48:37 +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 Tue, 17 Dec 2019 at 18:12, Robert Milkowski <rmilkowski@...il.com> wrote:
>
> On Mon, 16 Dec 2019 at 18:58, Trond Myklebust <trondmy@...merspace.com> wrote:
> >
> > 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:

I took a look at the client initialization and the
nfs4_init_clientid() already calls nfs4_setup_state_renewal(),
which in turn calls nfs4_proc_get_lease_time(), however that might
return with an error in which case it won't set the cl_last_renewal,
the code is:

static int nfs4_setup_state_renewal(struct nfs_client *clp)
...
        status = nfs4_proc_get_lease_time(clp, &fsinfo);
        if (status == 0) {
                nfs4_set_lease_period(clp, fsinfo.lease_time * HZ, now);
...

In my environment with nfsv4+krb the nfs4_proc_get_lease_time()
returns with -10016 (NFS4ERR_WRONGSEC) during initial mount (which
after a quick scan of the rfc seems that might be ok initially).
Also for v4.1 do_renew_lease() is called by nfs41_sequence_process()
multiple times during mount before we even reach nfs4_do_fsinfo() so
for 4.1+ it never gets cl_last_renewal==0, at least not under this
circumstances.

There might be other cases where nfs4_do_fsinfo() will get
clp->cl_last_renewal=0 for nfsv4.0, and since the nfs4_do_fsinfo()
already initializes the cl_last_renewal record, plus as Chuck pointed
out in case of v4.1+
it is expected to implicitly renew the lease anyway, I would rather
focus on fixing only the reported issue here for now, which is the
incorrect implicit renewal in fsinfo for v4.0, and leave improvements
to client initialization for another day.

I will send an updated patch shortly which doesn't impact v4.1+.