[PATCH 0/1] NFSv4: deal with v4.0 servers that request confirmation on reclaim-type opens

[PATCH 0/1] NFSv4: deal with v4.0 servers that request confirmation on reclaim-type opens

[Scott Mayhew]

A Red Hat customer started having problems with open reclaims after a
kernel upgrade.  It turns out that the customer's NFS server is setting
the OPEN4_RESULT_CONFIRM in response to an OPEN(CLAIM_PREVIOUS), which
is not permitted by RFC 7530.

The client used to be able to handle that because nfs4_open_prepare() used
to copy the filehandle from the OPEN args to the OPEN results when doing
an open by filehandle.  That copy was removed by commit 4e2fcac77390
"nfsv4: Use correct inode in _nfs4_opendata_to_nfs4_state()".

It looks to me like adding the copy back in would not affect the
behavior of the nfs4_opendata_get_inode() helper that was added in
4e2fcac77390 (it uses the filehandle to look up the inode when doing an
open by name)... so this patch adds the copy back in and also adds a
warning in the event that the client does receive OPEN4_RESULT_CONFIRM
in response to an OPEN(CLAIM_PREVIOUS).

Scott Mayhew (1):
  NFSv4: deal with v4.0 servers that request confirmation on
    reclaim-type opens

 fs/nfs/nfs4proc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH] NFSv4: deal with v4.0 servers that request confirmation on reclaim-type opens

[Scott Mayhew]

There are some NFS v4.0 servers that request confirmation on
reclaim-type opens even though it is disallowed by RFC 7530.

The Linux NFS client used to cope with those okay due to
1) nfs4_opendata_alloc() setting the filehandle in the args for the
OPEN_CONFIRM to point to the filehandle in the OPEN results, and
2) nfs4_open_prepare() copying the filehandle from the OPEN args to the
OPEN results when doing an open by filehandle

The latter part was removed by commit 4e2fcac77390 ("nfsv4: Use correct
inode in _nfs4_opendata_to_nfs4_state()")... and since there's no GETFH
in the OPEN compound when doing a reclaim-type open, if one of these
servers does request confirmation we wind up sending a zero filehandle
in the PUTFH of the OPEN_CONFIRM compound and the OPEN_CONFIRM fails
with NFS4ERR_BADHANDLE, causing the reclaim to fail.

Add the copying of the filehandle back to nfs4_open_prepare() and
log a warning in the event that the client does get a reply to
OPEN(CLAIM_PREVIOUS) that has NFS4_OPEN_RESULT_CONFIRM set.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/nfs4proc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 867457d6dfbe..5e521a506da3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2323,6 +2323,7 @@ static void nfs4_open_prepare(struct rpc_task *task, void *calldata)
 		/* Fall through */
 	case NFS4_OPEN_CLAIM_FH:
 		task->tk_msg.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_OPEN_NOATTR];
+		nfs_copy_fh(&data->o_res.fh, data->o_arg.fh);
 	}
 	data->timestamp = jiffies;
 	if (nfs4_setup_sequence(data->o_arg.server->nfs_client,
@@ -2470,8 +2471,14 @@ static int _nfs4_recover_proc_open(struct nfs4_opendata *data)
 
 	nfs_fattr_map_and_free_names(NFS_SERVER(dir), &data->f_attr);
 
-	if (o_res->rflags & NFS4_OPEN_RESULT_CONFIRM)
+	if (o_res->rflags & NFS4_OPEN_RESULT_CONFIRM) {
+		if (data->o_arg.claim == NFS4_OPEN_CLAIM_PREVIOUS)
+			pr_err_ratelimited("NFS: Broken NFSv4 server %s is "
+					"requesting open confirmation for "
+					"OPEN(CLAIM_PREVIOUS)\n",
+					NFS_SERVER(dir)->nfs_client->cl_hostname);
 		status = _nfs4_proc_open_confirm(data);
+	}
 
 	return status;
 }
-- 
2.17.1

Re: [PATCH] NFSv4: deal with v4.0 servers that request confirmation on reclaim-type opens

[Trond Myklebust]

On Fri, 2018-11-30 at 12:41 -0500, Scott Mayhew wrote:
> There are some NFS v4.0 servers that request confirmation on
> reclaim-type opens even though it is disallowed by RFC 7530.
> 
> The Linux NFS client used to cope with those okay due to
> 1) nfs4_opendata_alloc() setting the filehandle in the args for the
> OPEN_CONFIRM to point to the filehandle in the OPEN results, and
> 2) nfs4_open_prepare() copying the filehandle from the OPEN args to
> the
> OPEN results when doing an open by filehandle
> 
> The latter part was removed by commit 4e2fcac77390 ("nfsv4: Use
> correct
> inode in _nfs4_opendata_to_nfs4_state()")... and since there's no
> GETFH
> in the OPEN compound when doing a reclaim-type open, if one of these
> servers does request confirmation we wind up sending a zero
> filehandle
> in the PUTFH of the OPEN_CONFIRM compound and the OPEN_CONFIRM fails
> with NFS4ERR_BADHANDLE, causing the reclaim to fail.
> 
> Add the copying of the filehandle back to nfs4_open_prepare() and
> log a warning in the event that the client does get a reply to
> OPEN(CLAIM_PREVIOUS) that has NFS4_OPEN_RESULT_CONFIRM set.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 867457d6dfbe..5e521a506da3 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2323,6 +2323,7 @@ static void nfs4_open_prepare(struct rpc_task
> *task, void *calldata)
>  		/* Fall through */
>  	case NFS4_OPEN_CLAIM_FH:
>  		task->tk_msg.rpc_proc =
> &nfs4_procedures[NFSPROC4_CLNT_OPEN_NOATTR];
> +		nfs_copy_fh(&data->o_res.fh, data->o_arg.fh);
>  	}
>  	data->timestamp = jiffies;
>  	if (nfs4_setup_sequence(data->o_arg.server->nfs_client,
> @@ -2470,8 +2471,14 @@ static int _nfs4_recover_proc_open(struct
> nfs4_opendata *data)
>  
>  	nfs_fattr_map_and_free_names(NFS_SERVER(dir), &data->f_attr);
>  
> -	if (o_res->rflags & NFS4_OPEN_RESULT_CONFIRM)
> +	if (o_res->rflags & NFS4_OPEN_RESULT_CONFIRM) {
> +		if (data->o_arg.claim == NFS4_OPEN_CLAIM_PREVIOUS)
> +			pr_err_ratelimited("NFS: Broken NFSv4 server %s
> is "
> +					"requesting open confirmation
> for "
> +					"OPEN(CLAIM_PREVIOUS)\n",
> +					NFS_SERVER(dir)->nfs_client-
> >cl_hostname);
>  		status = _nfs4_proc_open_confirm(data);
> +	}
>  
>  	return status;
>  }

If the spec says that the server is broken, then we don't change the
client. We already have one ugly hack like this, but that is no excuse
for adding a second.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com