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

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

[Roberto Bergantinos Corpas]

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 (20bytes 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>
---
 fs/nfs/nfs4xdr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index fc049ce4ba8a..bae84e228520 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5169,8 +5169,8 @@ 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;
 }
 
-- 
2.45.0

Re: NFS: return DENIED in decode_lock_denied if we cannot decode owner

[Prabhakar Pujeri]

Hi Roberto,

Nice catch. I walked the caller paths against this and the reasoning
holds:

- decode_lock() passes fl = NULL and retries internally; decode_lockt()
  uses only offset/length/type, all decoded from the fixed 32-byte
  header before the owner field is reached. Nobody reads the owner
  bytes, so the removed EIO branch was the last consumer of that
  xdr_inline_decode() result, and its removal cannot leave a half-filled
  file_lock behind.
- The first 32-byte decode check still returns EIO on a truncated
  reply, so a genuinely malformed DENIED still fails early.
- The likely() inversion is real: decode failure is not the common case
  in a function that exists precisely because a denial always carries
  an owner.
- The "last operation" justification is, if anything, conservative --
  decode_lock()/decode_lockt() return non-zero status to the compound
  decode loop, which stops at this op either way, so pointer
  advancement would be moot even with more ops queued.

> 	p = xdr_inline_decode(xdr, namelen); /* variable size field */
> -	if (likely(!p))
> -		return -EIO;
> +	/* We have an owner here, return DENIED */
> +
>  	return -NFS4ERR_DENIED;

nit: the blank line between the comment and the return is unusual; I
would keep the comment immediately above the return.

> After commit 43502f6e8d1e ("NFS: fix open_owner_id_maxsz and related
> fields.")we dramatically changed the size of the LOCK/LOCKT reply
> buffer

nit: missing space -- "fields.") we dramatically changed". A few lines
further down the changelog also has "20bytes" -> "20 bytes".

No test evidence came with the posting. For a two-line decode tweak I
do not consider that a blocker, though a synthetic oversized-owner
reply (pynfs or a mangled-RPC test harness) would make a fine selftest
someday.

With the changelog nits fixed:

Reviewed-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>