Received-SPF: pass (google.com: domain of linux-nfs+bounces-1459-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Benjamin Coddington <bcodding@redhat.com>
To: Jorge Mora <jmora1300@gmail.com>
Cc: linux-nfs@vger.kernel.org, trond.myklebust@hammerspace.com,
 anna@kernel.org
Subject: Re: [PATCH] NFSv4.2: fix listxattr maximum XDR buffer size
Date: Fri, 26 Jan 2024 08:48:11 -0500
Message-ID: <FE38CC90-A3BE-40CF-9365-557CA706376E@redhat.com>
In-Reply-To: <20240125145128.12945-1-mora@netapp.com>
References: <20240125145128.12945-1-mora@netapp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 25 Jan 2024, at 9:51, Jorge Mora wrote:

> Switch order of operations to avoid creating a short XDR buffer:
> e.g., buflen = 12, old xdrlen = 12, new xdrlen = 20.
>
> Having a short XDR buffer leads to lxa_maxcount be a few bytes
> less than what is needed to retrieve the whole list when using
> a buflen as returned by a call with size = 0:
>     buflen = listxattr(path, NULL, 0);
>     buf = malloc(buflen);
>     buflen = listxattr(path, buf, buflen);
>
> For a file with one attribute (name = '123456'), the first call
> with size = 0 will return buflen = 12 ('user.123456\x00').
> The second call with size = 12, sends LISTXATTRS with
> lxa_maxcount = 12 + 8 (cookie) + 4 (array count) = 24. The
> XDR buffer needs 8 (cookie) + 4 (array count) + 4 (name count)
> + 6 (name len) + 2 (padding) + 4 (eof) = 28 which is 4 bytes
> shorter than the lxa_maxcount provided in the call.
>
> Fixes: 04a5da690e8f ("NFSv4.2: define limits and sizes for user xattr handling")
> Signed-off-by: Jorge Mora <mora@netapp.com>
> ---
>  fs/nfs/nfs42.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs42.h b/fs/nfs/nfs42.h
> index b59876b01a1e..32ee9ad6a560 100644
> --- a/fs/nfs/nfs42.h
> +++ b/fs/nfs/nfs42.h
> @@ -55,11 +55,14 @@ int nfs42_proc_removexattr(struct inode *inode, const char *name);
>   * They would be 7 bytes long in the eventual buffer ("user.x\0"), and
>   * 8 bytes long XDR-encoded.
>   *
> - * Include the trailing eof word as well.
> + * Include the trailing eof word as well and make the result a multiple
> + * of 4 bytes.
>   */
>  static inline u32 nfs42_listxattr_xdrsize(u32 buflen)
>  {
> -	return ((buflen / (XATTR_USER_PREFIX_LEN + 2)) * 8) + 4;
> +	u32 size = 8 * buflen / (XATTR_USER_PREFIX_LEN + 2) + 4;
> +
> +	return (size + 3) & ~3;

Maybe XDR_QUADLEN(size) here?  Otherwise, looks correct.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben