Re: [PATCH v2] NFS: return DENIED in decode_lock_denied if we cannot decode owner

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2026-09-17 at 12:49 +0200, Roberto Bergantinos Corpas wrote:
> After commit 43502f6e8d1e ("NFS: fix open_owner_id_maxsz and related
> fields.") we dramatically changed the size of the LOCK/LOCKT reply buffer
> from 164 to 52 bytes. This made visible a situation on a buggy NFS
> server that sent oversized denied responses which led to us returning
> EIO instead of DENIED.
> 
> Now, apart from buggy server issue, this poses an interesting question:
> what if other implementations have legitimate but bigger than we expect
> lock owner response, especially now that we have reduced the size we
> allocate for it (20 bytes for the lock owner part).
> 
> This change proposes to tackle that issue returning DENIED instead of
> EIO regardless of whether we manage to decode the owner or not:
> 
> - At this point of decode_lock_denied we know there is an owner.
> - Server legitimately denied the lock request, with an owner that fits on
>   NFS4_OPAQUE_LIMIT but we returned EIO instead to userspace.
> - The owner data is decoded but actually never used, decode_lock path
>   simply retries, and decode_lockt turns it into 0 on file_lock and
>   returns it to userspace.
> - LOCK/LOCKT are the last operations on the compound so it's not relevant
>   if we didn't advance the pointer.
> - Also removes an inverted likely(!p) hint.
> 
> Fixes: 43502f6e8d1e ("NFS: fix open_owner_id_maxsz and related fields.")
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> Reviewed-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
> ---
>  v2:
>   - fixed formatting issues
> 
>  fs/nfs/nfs4xdr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index fc049ce4ba8a..9d0f7ad7525a 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -5169,8 +5169,7 @@ static int decode_lock_denied(struct xdr_stream *xdr, struct file_lock *fl)
>  	p = xdr_decode_hyper(p, &clientid); /* read 8 bytes */
>  	namelen = be32_to_cpup(p); /* read 4 bytes */  /* have read all 32 bytes now */
>  	p = xdr_inline_decode(xdr, namelen); /* variable size field */
> -	if (likely(!p))
> -		return -EIO;
> +	/* We have an owner here, return DENIED */
>  	return -NFS4ERR_DENIED;
>  }
>  

Typically, we don't work around server bugs in the client (and vice
versa), but I think this is a reasonable thing to do in this case since
we don't use remote lockowner information anywhere in the kernel.

Reviewed-by: Jeff Layton <jlayton@kernel.org>