Re: [PATCH 1/1] NFSv4.2: fix LISTXATTR buffer receive size

[Chuck Lever <chuck.lever@oracle.com>]

Hi Olga-

> On Nov 13, 2020, at 2:08 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> xfstest generic/013 over on a NFSoRDMA over SoftRoCE or iWarp panics
> and running with KASAN reports:

There is still only a highly circumstantial connection between
soft RoCE and iWarp and these crashes, so this description seems
misleading and/or pre-mature.


> [  216.018711] BUG: KASAN: wild-memory-access in rpcrdma_complete_rqst+0x447/0x6e0 [rpcrdma]
> [  216.024195] Write of size 12 at addr 0005088000000000 by task kworker/1:1H/480
> [  216.028820]
> [  216.029776] CPU: 1 PID: 480 Comm: kworker/1:1H Not tainted 5.8.0-rc5+ #37
> [  216.034247] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
> [  216.040604] Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> [  216.043739] Call Trace:
> [  216.045014]  dump_stack+0x7c/0xb0
> [  216.046757]  ? rpcrdma_complete_rqst+0x447/0x6e0 [rpcrdma]
> [  216.050008]  ? rpcrdma_complete_rqst+0x447/0x6e0 [rpcrdma]
> [  216.053091]  kasan_report.cold.10+0x6a/0x85
> [  216.055703]  ? rpcrdma_complete_rqst+0x447/0x6e0 [rpcrdma]
> [  216.058979]  check_memory_region+0x183/0x1e0
> [  216.061933]  memcpy+0x38/0x60
> [  216.064077]  rpcrdma_complete_rqst+0x447/0x6e0 [rpcrdma]
> [  216.067502]  ? rpcrdma_reset_cwnd+0x70/0x70 [rpcrdma]
> [  216.070268]  ? recalibrate_cpu_khz+0x10/0x10
> [  216.072585]  ? rpcrdma_reply_handler+0x604/0x6e0 [rpcrdma]
> [  216.075469]  __ib_process_cq+0xa7/0x220 [ib_core]
> [  216.078077]  ib_cq_poll_work+0x31/0xb0 [ib_core]
> [  216.080451]  process_one_work+0x387/0x6c0
> [  216.082498]  worker_thread+0x57/0x5a0
> [  216.084425]  ? process_one_work+0x6c0/0x6c0
> [  216.086583]  kthread+0x1ca/0x200
> [  216.088775]  ? kthread_create_on_node+0xc0/0xc0
> [  216.091847]  ret_from_fork+0x22/0x30
> 
> Fixes: 6c2190b3fcbc ("NFS: Fix listxattr receive buffer size")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
> fs/nfs/nfs42xdr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> index 6e060a8..e88bc7a 100644
> --- a/fs/nfs/nfs42xdr.c
> +++ b/fs/nfs/nfs42xdr.c
> @@ -196,7 +196,8 @@
> 				 1 + nfs4_xattr_name_maxsz + 1)
> #define decode_setxattr_maxsz   (op_decode_hdr_maxsz + decode_change_info_maxsz)
> #define encode_listxattrs_maxsz  (op_encode_hdr_maxsz + 2 + 1)
> -#define decode_listxattrs_maxsz  (op_decode_hdr_maxsz + 2 + 1 + 1 + 1)
> +#define decode_listxattrs_maxsz  (op_decode_hdr_maxsz + 2 + 1 + 1 + \
> +				  XDR_QUADLEN(NFS4_OPAQUE_LIMIT))

From RFC 8276, Section 8.4.3.2:

   /// struct LISTXATTRS4resok {
   ///         nfs_cookie4    lxr_cookie;
   ///         xattrkey4      lxr_names<>;
   ///         bool           lxr_eof;
   /// };

The decode_listxattrs_maxsz macro defines the maximum size of
the /fixed portion/ of the LISTXATTRS reply. That is the operation
status code, the cookie, and the EOF flag. maxsz has an extra
"+ 1" for rpc_prepare_reply_pages() to deal with possible XDR
padding. The post-6c2190b3fcbc value of this macro is already
correct, and removing the "+ 1" is wrong.

In addition, the variable portion of the result is an unbounded
array of component4 fields, nothing to do with NFS4_OPAQUE_LIMIT,
so that's just an arbitrary constant here with no justification.

Adding more space to the receive buffer happens to help the case
where there are no xattrs to list. That doesn't mean its the
correct solution in general. It certainly won't be sufficient to
handle an xattr name array that is larger than 1024 bytes.


The client manages the variable portion of that result in the
reply's pages array, which is set up by nfs4_xdr_enc_listxattrs().

Further:

> rpcrdma_complete_rqst+0x447

is in the paragraph of rpcrdma_inline_fixup() that copies received
bytes into the receive xdr_buf's pages array.

The address "0005088000000000" is bogus. Since
nfs4_xdr_enc_listxattrs() sets XDRBUF_SPARSE_PAGES, it's likely
it is relying on the transport to allocate pages for this buffer,
and possibly that page allocation has failed or has a bug.

Please determine why the encode side has not set up the correct
number of pages to handle the LISTXATTRS result. Until then I
have to NACK this one.


> #define encode_removexattr_maxsz (op_encode_hdr_maxsz + 1 + \
> 				  nfs4_xattr_name_maxsz)
> #define decode_removexattr_maxsz (op_decode_hdr_maxsz + \
> -- 
> 1.8.3.1

--
Chuck Lever