[lkml]   [2019]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patches in this message
SubjectRE: [PATCH v2] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease
Hi Chuck,

> > On Dec 24, 2019, at 9:36 AM, Robert Milkowski <> wrote:
> >
> > From: Robert Milkowski <>
> >
> > 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")

Right, makes sense. I will include it in the next patch revision.

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

Is adding the below to the previous description enough?

The 83ca7f5ab31f introduced implicit renew operation when calling nfs4_do_fsinfo(),
which is not done by NFS servers which under some circumstances results in nfsv4.0 lease
to expire on a server side which then will return NFS4ERR_EXPIRED or NFS4ERR_STALE_CLIENTID.
This can be easily reproduced by frequently unmounting a sub-mount over nfsv4.0,
which after a grace period will result in an error returned by a server when accessing a file.
This can also happen after a sub-mount is automatically unmounted due to inactivity
(after nfs_mountpoint_expiry_timeout), then the sub-mount is stat'ed causing it to be mounted again,
and a file is accessed shortly after (this all depends on specific grace time, last RENEW, etc.).
This specific case was observed on a production systems.

> 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.

Thanks for the explanation, I'm learning here and really do appreciate any help.
So given the above advise I ended up with the below:

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 76d3716..6d075f0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4998,12 +4998,16 @@ static int nfs4_proc_statfs(struct nfs_server *server, struct nfs_fh *fhandle, s
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_fsinfo_arg args = {
.fh = fhandle,
.bitmask = server->attr_bitmask,
+ .clientid = clp->cl_clientid,
+ .renew = nfs4_has_session(clp) ? 0 : 1, /* append RENEW */
struct nfs4_fsinfo_res res = {
.fsinfo = fsinfo,
+ .renew = nfs4_has_session(clp) ? 0 : 1,
struct rpc_message msg = {
.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_FSINFO],
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 936c577..0ce9a10 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -555,11 +555,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
#define NFS4_enc_fsinfo_sz (compound_encode_hdr_maxsz + \
encode_sequence_maxsz + \
encode_putfh_maxsz + \
- encode_fsinfo_maxsz)
+ encode_fsinfo_maxsz + \
+ encode_renew_maxsz)
#define NFS4_dec_fsinfo_sz (compound_decode_hdr_maxsz + \
decode_sequence_maxsz + \
decode_putfh_maxsz + \
- decode_fsinfo_maxsz)
+ decode_fsinfo_maxsz + \
+ decode_renew_maxsz)
#define NFS4_enc_renew_sz (compound_encode_hdr_maxsz + \
#define NFS4_dec_renew_sz (compound_decode_hdr_maxsz + \
@@ -2646,6 +2648,8 @@ static void nfs4_xdr_enc_fsinfo(struct rpc_rqst *req, struct xdr_stream *xdr,
encode_sequence(xdr, &args->seq_args, &hdr);
encode_putfh(xdr, args->fh, &hdr);
encode_fsinfo(xdr, args->bitmask, &hdr);
+ if (args->renew)
+ encode_renew(xdr, args->clientid, &hdr);

@@ -6778,6 +6782,11 @@ static int nfs4_xdr_dec_fsinfo(struct rpc_rqst *req, struct xdr_stream *xdr,
status = decode_putfh(xdr);
if (!status)
status = decode_fsinfo(xdr, res->fsinfo);
+ if (status)
+ goto out;
+ if (res->renew)
+ status = decode_renew(xdr);
return status;

diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 72d5695..49bd673 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1025,11 +1025,14 @@ struct nfs4_fsinfo_arg {
struct nfs4_sequence_args seq_args;
const struct nfs_fh * fh;
const u32 * bitmask;
+ clientid4 clientid;
+ unsigned char renew:1;

struct nfs4_fsinfo_res {
struct nfs4_sequence_res seq_res;
struct nfs_fsinfo *fsinfo;
+ unsigned char renew:1;

struct nfs4_getattr_arg {

Let me know if that's what you had it mind and if no further comments I will finish testing and submit new patch.

> 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?

Strictly speaking I don't think renew is required here, but adding it as part of the compound
operation is harmless and more in-line with how it is currently done for v4.1.

Also, before the 83ca7f5ab31f, implicit lease renewal was only done in nfs4_proc_setclientid_confirm(),
but the function is not called when mounting a sub-mount, and it was not done in nfs4_do_fsinfo() either.
The implicit renewal in nfs4_do_fsinfo() when mounting each submount was introduced by the commit, before it only happened on root
So this particular issue I'm trying to fix here did not occur before the change, I think.

(btw: according to RFC7530 section 9.5. the implicit renewal in setclientid_confirm() wasn't correct either but I think it was

 \ /
  Last update: 2019-12-27 20:07    [W:0.055 / U:2.872 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site