[v1,20/23] NFS: Account for XDR pad of buf->pages

Message ID	20190211162525.2817.60645.stgit@manet.1015granger.net (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-nfs-owner@kernel.org> Subject: [PATCH v1 20/23] NFS: Account for XDR pad of buf->pages From: Chuck Lever <chuck.lever@oracle.com> To: anna.schumaker@netapp.com Cc: linux-rdma@vger.kernel.org, linux-nfs@vger.kernel.org Date: Mon, 11 Feb 2019 11:25:25 -0500 Message-ID: <20190211162525.2817.60645.stgit@manet.1015granger.net> In-Reply-To: <20190211161920.2817.20881.stgit@manet.1015granger.net> References: <20190211161920.2817.20881.stgit@manet.1015granger.net> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk
Series	NFS client patches for v5.1 (complete) \| expand [v1,00/23] NFS client patches for v5.1 (complete) [v1,01/23] xprtrdma: Fix sparse warnings [v1,02/23] xprtrdma: Check inline size before providing a Write chunk [v1,03/23] xprtrdma: Reduce the doorbell rate (Receive) [v1,04/23] SUNRPC: Display symbolic flag names in RPC trace events [v1,05/23] SUNRPC: Add xdr_stream::rqst field [v1,06/23] SUNRPC: Add XDR overflow trace event [v1,07/23] SUNRPC: Add trace event that reports reply page vector alignment [v1,08/23] NFS: Remove print_overflow_msg() [v1,09/23] NFS: Add trace events to report non-zero NFS status codes [v1,10/23] SUNRPC: Remove some dprintk() call sites from auth functions [v1,11/23] SUNRPC: Remove rpc_xprt::tsh_size [v1,12/23] SUNRPC: Add build option to disable support for insecure enctypes [v1,13/23] SUNRPC: Use struct xdr_stream when constructing RPC Call header [v1,14/23] SUNRPC: Clean up rpc_verify_header() [v1,15/23] SUNRPC: Use struct xdr_stream when decoding RPC Reply header [v1,16/23] SUNRPC: Introduce trace points in rpc_auth_gss.ko [v1,17/23] SUNRPC: Remove xdr_buf_trim() [v1,18/23] SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files [v1,19/23] SUNRPC: Introduce rpc_prepare_reply_pages() [v1,20/23] NFS: Account for XDR pad of buf->pages [v1,21/23] SUNRPC: Make AUTH_SYS and AUTH_NULL set au_verfsize [v1,22/23] SUNRPC: Add rpc_auth::au_ralign field [v1,23/23] SUNRPC: Use au_rslack when computing reply buffer size

Chuck Lever Feb. 11, 2019, 4:25 p.m. UTC

Certain NFS results (eg. READLINK) might expect a data payload that
is not an exact multiple of 4 bytes. In this case, XDR encoding
is required to pad that payload so its length on the wire is a
multiple of 4 bytes. The constants that define the maximum size of
each NFS result do not appear to account for this extra word.

In each case where the data payload is to be received into pages:

- 1 word is added to the size of the receive buffer allocated by
  call_allocate

- rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
  extra buffer space falls into the rcv_buf's tail iovec

- If buf->pagelen is word-aligned, an XDR pad is not needed and
  is thus removed from the tail

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs2xdr.c  |    6 +++---
 fs/nfs/nfs3xdr.c  |   10 +++++-----
 fs/nfs/nfs4xdr.c  |   15 ++++++++-------
 net/sunrpc/clnt.c |    6 +++++-
 net/sunrpc/xdr.c  |    2 ++
 5 files changed, 23 insertions(+), 16 deletions(-)

Olga Kornievskaia April 5, 2019, 5:36 p.m. UTC | #1

Hi Chuck,

This patch break ACLs. After applying this patch nfs4_getfacl fails
(it fails within xdr and returns ENOTSUPP). Any ideas why?

On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> Certain NFS results (eg. READLINK) might expect a data payload that
> is not an exact multiple of 4 bytes. In this case, XDR encoding
> is required to pad that payload so its length on the wire is a
> multiple of 4 bytes. The constants that define the maximum size of
> each NFS result do not appear to account for this extra word.
>
> In each case where the data payload is to be received into pages:
>
> - 1 word is added to the size of the receive buffer allocated by
>   call_allocate
>
> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>   extra buffer space falls into the rcv_buf's tail iovec
>
> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>   is thus removed from the tail
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/nfs2xdr.c  |    6 +++---
>  fs/nfs/nfs3xdr.c  |   10 +++++-----
>  fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>  net/sunrpc/clnt.c |    6 +++++-
>  net/sunrpc/xdr.c  |    2 ++
>  5 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> index 1dcd0fe..a7ed29d 100644
> --- a/fs/nfs/nfs2xdr.c
> +++ b/fs/nfs/nfs2xdr.c
> @@ -56,11 +56,11 @@
>
>  #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>  #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> -#define NFS_readlinkres_sz     (2)
> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> +#define NFS_readlinkres_sz     (2+1)
> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>  #define NFS_writeres_sz         (NFS_attrstat_sz)
>  #define NFS_stat_sz            (1)
> -#define NFS_readdirres_sz      (1)
> +#define NFS_readdirres_sz      (1+1)
>  #define NFS_statfsres_sz       (1+NFS_info_sz)
>
>  static int nfs_stat_to_errno(enum nfs_stat);
> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> index a54dcf4..110358f 100644
> --- a/fs/nfs/nfs3xdr.c
> +++ b/fs/nfs/nfs3xdr.c
> @@ -69,13 +69,13 @@
>  #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>  #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>  #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>  #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>  #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>  #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>  #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>  #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>  #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>  #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> @@ -85,7 +85,7 @@
>  #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>                                 XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>  #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>  #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>
>  static int nfs3_stat_to_errno(enum nfs_stat);
> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>         result->op_status = status;
>         if (status != NFS3_OK)
>                 goto out_status;
> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>         error = decode_read3resok(xdr, result);
>  out:
>         return error;
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index d0fa18d..6d9d5e2 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>                                  nfs4_fattr_bitmap_maxsz)
>  #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 3)
> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>  #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>                                  2 + encode_verifier_maxsz + 5 + \
>                                 nfs4_label_maxsz)
>  #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> -                                decode_verifier_maxsz)
> +                                decode_verifier_maxsz + 1)
>  #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>  #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 4)
>  #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>  #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>  #define encode_getacl_maxsz    (encode_getattr_maxsz)
>  #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> -                                nfs4_fattr_bitmap_maxsz + 1)
> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>  #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 3)
>  #define decode_setacl_maxsz    (decode_setattr_maxsz)
>  #define encode_fs_locations_maxsz \
>                                 (encode_getattr_maxsz)
>  #define decode_fs_locations_maxsz \
> -                               (0)
> +                               (1)
>  #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>  #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>
> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>                                 1 /* opaque devaddr4 length */ + \
>                                   /* devaddr4 payload is read into page */ \
>                                 1 /* notification bitmap length */ + \
> -                               1 /* notification bitmap, word 0 */)
> +                               1 /* notification bitmap, word 0 */ + \
> +                               1 /* possible XDR padding */)
>  #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>                                 encode_stateid_maxsz)
>  #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>                                 decode_stateid_maxsz + \
> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>  #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>                                 2 /* offset */ + \
>                                 2 /* length */ + \
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index f780605..4ea38b0 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>                              unsigned int base, unsigned int len,
>                              unsigned int hdrsize)
>  {
> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> +       /* Subtract one to force an extra word of buffer space for the
> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> +        */
> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> +
>         xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>         trace_rpc_reply_pages(req);
>  }
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 7cca515..aa8177d 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>
>         tail->iov_base = buf + offset;
>         tail->iov_len = buflen - offset;
> +       if ((xdr->page_len & 3) == 0)
> +               tail->iov_len -= sizeof(__be32);
>
>         xdr->buflen += len;
>  }
>

Chuck Lever April 5, 2019, 5:51 p.m. UTC | #2

> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> Hi Chuck,
> 
> This patch break ACLs. After applying this patch nfs4_getfacl fails
> (it fails within xdr and returns ENOTSUPP). Any ideas why?

Possibly the macro that defines the maximum size of the reply
is incorrect.


> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> Certain NFS results (eg. READLINK) might expect a data payload that
>> is not an exact multiple of 4 bytes. In this case, XDR encoding
>> is required to pad that payload so its length on the wire is a
>> multiple of 4 bytes. The constants that define the maximum size of
>> each NFS result do not appear to account for this extra word.
>> 
>> In each case where the data payload is to be received into pages:
>> 
>> - 1 word is added to the size of the receive buffer allocated by
>>  call_allocate
>> 
>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>>  extra buffer space falls into the rcv_buf's tail iovec
>> 
>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>>  is thus removed from the tail
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfs/nfs2xdr.c  |    6 +++---
>> fs/nfs/nfs3xdr.c  |   10 +++++-----
>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>> net/sunrpc/clnt.c |    6 +++++-
>> net/sunrpc/xdr.c  |    2 ++
>> 5 files changed, 23 insertions(+), 16 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
>> index 1dcd0fe..a7ed29d 100644
>> --- a/fs/nfs/nfs2xdr.c
>> +++ b/fs/nfs/nfs2xdr.c
>> @@ -56,11 +56,11 @@
>> 
>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
>> -#define NFS_readlinkres_sz     (2)
>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
>> +#define NFS_readlinkres_sz     (2+1)
>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>> #define NFS_writeres_sz         (NFS_attrstat_sz)
>> #define NFS_stat_sz            (1)
>> -#define NFS_readdirres_sz      (1)
>> +#define NFS_readdirres_sz      (1+1)
>> #define NFS_statfsres_sz       (1+NFS_info_sz)
>> 
>> static int nfs_stat_to_errno(enum nfs_stat);
>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
>> index a54dcf4..110358f 100644
>> --- a/fs/nfs/nfs3xdr.c
>> +++ b/fs/nfs/nfs3xdr.c
>> @@ -69,13 +69,13 @@
>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
>> @@ -85,7 +85,7 @@
>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>>                                XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>> 
>> static int nfs3_stat_to_errno(enum nfs_stat);
>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>>        result->op_status = status;
>>        if (status != NFS3_OK)
>>                goto out_status;
>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>        error = decode_read3resok(xdr, result);
>> out:
>>        return error;
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index d0fa18d..6d9d5e2 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>                                 nfs4_fattr_bitmap_maxsz)
>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>>                                 encode_stateid_maxsz + 3)
>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>>                                 2 + encode_verifier_maxsz + 5 + \
>>                                nfs4_label_maxsz)
>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
>> -                                decode_verifier_maxsz)
>> +                                decode_verifier_maxsz + 1)
>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>>                                 encode_stateid_maxsz + 4)
>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
>> -                                nfs4_fattr_bitmap_maxsz + 1)
>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>>                                 encode_stateid_maxsz + 3)
>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
>> #define encode_fs_locations_maxsz \
>>                                (encode_getattr_maxsz)
>> #define decode_fs_locations_maxsz \
>> -                               (0)
>> +                               (1)
>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>> 
>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>                                1 /* opaque devaddr4 length */ + \
>>                                  /* devaddr4 payload is read into page */ \
>>                                1 /* notification bitmap length */ + \
>> -                               1 /* notification bitmap, word 0 */)
>> +                               1 /* notification bitmap, word 0 */ + \
>> +                               1 /* possible XDR padding */)
>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>>                                encode_stateid_maxsz)
>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>>                                decode_stateid_maxsz + \
>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>>                                2 /* offset */ + \
>>                                2 /* length */ + \
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index f780605..4ea38b0 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>>                             unsigned int base, unsigned int len,
>>                             unsigned int hdrsize)
>> {
>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
>> +       /* Subtract one to force an extra word of buffer space for the
>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
>> +        */
>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
>> +
>>        xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>>        trace_rpc_reply_pages(req);
>> }
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 7cca515..aa8177d 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>> 
>>        tail->iov_base = buf + offset;
>>        tail->iov_len = buflen - offset;
>> +       if ((xdr->page_len & 3) == 0)
>> +               tail->iov_len -= sizeof(__be32);
>> 
>>        xdr->buflen += len;
>> }
>> 

--
Chuck Lever

Olga Kornievskaia April 5, 2019, 7:17 p.m. UTC | #3

On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > Hi Chuck,
> >
> > This patch break ACLs. After applying this patch nfs4_getfacl fails
> > (it fails within xdr and returns ENOTSUPP). Any ideas why?
>
> Possibly the macro that defines the maximum size of the reply
> is incorrect.
>

This also breaks FS_LOCATION. I'm going to go on the limb here and say
that it probably breaks whatever else it modified. The question is:
can't we just revert it??

>
> > On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> Certain NFS results (eg. READLINK) might expect a data payload that
> >> is not an exact multiple of 4 bytes. In this case, XDR encoding
> >> is required to pad that payload so its length on the wire is a
> >> multiple of 4 bytes. The constants that define the maximum size of
> >> each NFS result do not appear to account for this extra word.
> >>
> >> In each case where the data payload is to be received into pages:
> >>
> >> - 1 word is added to the size of the receive buffer allocated by
> >>  call_allocate
> >>
> >> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> >>  extra buffer space falls into the rcv_buf's tail iovec
> >>
> >> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> >>  is thus removed from the tail
> >>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> fs/nfs/nfs2xdr.c  |    6 +++---
> >> fs/nfs/nfs3xdr.c  |   10 +++++-----
> >> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> >> net/sunrpc/clnt.c |    6 +++++-
> >> net/sunrpc/xdr.c  |    2 ++
> >> 5 files changed, 23 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> >> index 1dcd0fe..a7ed29d 100644
> >> --- a/fs/nfs/nfs2xdr.c
> >> +++ b/fs/nfs/nfs2xdr.c
> >> @@ -56,11 +56,11 @@
> >>
> >> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> >> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> >> -#define NFS_readlinkres_sz     (2)
> >> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> >> +#define NFS_readlinkres_sz     (2+1)
> >> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> >> #define NFS_writeres_sz         (NFS_attrstat_sz)
> >> #define NFS_stat_sz            (1)
> >> -#define NFS_readdirres_sz      (1)
> >> +#define NFS_readdirres_sz      (1+1)
> >> #define NFS_statfsres_sz       (1+NFS_info_sz)
> >>
> >> static int nfs_stat_to_errno(enum nfs_stat);
> >> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> >> index a54dcf4..110358f 100644
> >> --- a/fs/nfs/nfs3xdr.c
> >> +++ b/fs/nfs/nfs3xdr.c
> >> @@ -69,13 +69,13 @@
> >> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> >> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> >> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> >> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> >> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> >> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> >> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> >> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> >> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> >> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> >> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> >> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> >> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> >> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> >> @@ -85,7 +85,7 @@
> >> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> >>                                XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> >> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> >> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> >>
> >> static int nfs3_stat_to_errno(enum nfs_stat);
> >> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> >>        result->op_status = status;
> >>        if (status != NFS3_OK)
> >>                goto out_status;
> >> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>        error = decode_read3resok(xdr, result);
> >> out:
> >>        return error;
> >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >> index d0fa18d..6d9d5e2 100644
> >> --- a/fs/nfs/nfs4xdr.c
> >> +++ b/fs/nfs/nfs4xdr.c
> >> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>                                 nfs4_fattr_bitmap_maxsz)
> >> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> >>                                 encode_stateid_maxsz + 3)
> >> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> >> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> >> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> >>                                 2 + encode_verifier_maxsz + 5 + \
> >>                                nfs4_label_maxsz)
> >> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> >> -                                decode_verifier_maxsz)
> >> +                                decode_verifier_maxsz + 1)
> >> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> >> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> >> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> >> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> >>                                 encode_stateid_maxsz + 4)
> >> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> >> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> >> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> >> -                                nfs4_fattr_bitmap_maxsz + 1)
> >> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> >> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> >>                                 encode_stateid_maxsz + 3)
> >> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> >> #define encode_fs_locations_maxsz \
> >>                                (encode_getattr_maxsz)
> >> #define decode_fs_locations_maxsz \
> >> -                               (0)
> >> +                               (1)
> >> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >>
> >> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>                                1 /* opaque devaddr4 length */ + \
> >>                                  /* devaddr4 payload is read into page */ \
> >>                                1 /* notification bitmap length */ + \
> >> -                               1 /* notification bitmap, word 0 */)
> >> +                               1 /* notification bitmap, word 0 */ + \
> >> +                               1 /* possible XDR padding */)
> >> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> >>                                encode_stateid_maxsz)
> >> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> >>                                decode_stateid_maxsz + \
> >> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> >> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> >> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> >>                                2 /* offset */ + \
> >>                                2 /* length */ + \
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index f780605..4ea38b0 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> >>                             unsigned int base, unsigned int len,
> >>                             unsigned int hdrsize)
> >> {
> >> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> >> +       /* Subtract one to force an extra word of buffer space for the
> >> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> >> +        */
> >> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> >> +
> >>        xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> >>        trace_rpc_reply_pages(req);
> >> }
> >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >> index 7cca515..aa8177d 100644
> >> --- a/net/sunrpc/xdr.c
> >> +++ b/net/sunrpc/xdr.c
> >> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> >>
> >>        tail->iov_base = buf + offset;
> >>        tail->iov_len = buflen - offset;
> >> +       if ((xdr->page_len & 3) == 0)
> >> +               tail->iov_len -= sizeof(__be32);
> >>
> >>        xdr->buflen += len;
> >> }
> >>
>
> --
> Chuck Lever
>
>
>

Chuck Lever April 5, 2019, 7:23 p.m. UTC | #4

> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> 
>>> Hi Chuck,
>>> 
>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
>> 
>> Possibly the macro that defines the maximum size of the reply
>> is incorrect.
>> 
> 
> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> that it probably breaks whatever else it modified.

It modifies READ, READDIR, and READLINK. Are those broken?


> The question is: can't we just revert it??

Why not "root cause" it first?


>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> 
>>>> Certain NFS results (eg. READLINK) might expect a data payload that
>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
>>>> is required to pad that payload so its length on the wire is a
>>>> multiple of 4 bytes. The constants that define the maximum size of
>>>> each NFS result do not appear to account for this extra word.
>>>> 
>>>> In each case where the data payload is to be received into pages:
>>>> 
>>>> - 1 word is added to the size of the receive buffer allocated by
>>>> call_allocate
>>>> 
>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>>>> extra buffer space falls into the rcv_buf's tail iovec
>>>> 
>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>>>> is thus removed from the tail
>>>> 
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> fs/nfs/nfs2xdr.c  |    6 +++---
>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>>>> net/sunrpc/clnt.c |    6 +++++-
>>>> net/sunrpc/xdr.c  |    2 ++
>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
>>>> 
>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
>>>> index 1dcd0fe..a7ed29d 100644
>>>> --- a/fs/nfs/nfs2xdr.c
>>>> +++ b/fs/nfs/nfs2xdr.c
>>>> @@ -56,11 +56,11 @@
>>>> 
>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
>>>> -#define NFS_readlinkres_sz     (2)
>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
>>>> +#define NFS_readlinkres_sz     (2+1)
>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
>>>> #define NFS_stat_sz            (1)
>>>> -#define NFS_readdirres_sz      (1)
>>>> +#define NFS_readdirres_sz      (1+1)
>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
>>>> 
>>>> static int nfs_stat_to_errno(enum nfs_stat);
>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
>>>> index a54dcf4..110358f 100644
>>>> --- a/fs/nfs/nfs3xdr.c
>>>> +++ b/fs/nfs/nfs3xdr.c
>>>> @@ -69,13 +69,13 @@
>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
>>>> @@ -85,7 +85,7 @@
>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>>>>                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>>>> 
>>>> static int nfs3_stat_to_errno(enum nfs_stat);
>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>>>>       result->op_status = status;
>>>>       if (status != NFS3_OK)
>>>>               goto out_status;
>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>       error = decode_read3resok(xdr, result);
>>>> out:
>>>>       return error;
>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>> index d0fa18d..6d9d5e2 100644
>>>> --- a/fs/nfs/nfs4xdr.c
>>>> +++ b/fs/nfs/nfs4xdr.c
>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>                                nfs4_fattr_bitmap_maxsz)
>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>>>>                                encode_stateid_maxsz + 3)
>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>>>>                                2 + encode_verifier_maxsz + 5 + \
>>>>                               nfs4_label_maxsz)
>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
>>>> -                                decode_verifier_maxsz)
>>>> +                                decode_verifier_maxsz + 1)
>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>>>>                                encode_stateid_maxsz + 4)
>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>>>>                                encode_stateid_maxsz + 3)
>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
>>>> #define encode_fs_locations_maxsz \
>>>>                               (encode_getattr_maxsz)
>>>> #define decode_fs_locations_maxsz \
>>>> -                               (0)
>>>> +                               (1)
>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>>>> 
>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>                               1 /* opaque devaddr4 length */ + \
>>>>                                 /* devaddr4 payload is read into page */ \
>>>>                               1 /* notification bitmap length */ + \
>>>> -                               1 /* notification bitmap, word 0 */)
>>>> +                               1 /* notification bitmap, word 0 */ + \
>>>> +                               1 /* possible XDR padding */)
>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>>>>                               encode_stateid_maxsz)
>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>>>>                               decode_stateid_maxsz + \
>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>>>>                               2 /* offset */ + \
>>>>                               2 /* length */ + \
>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>> index f780605..4ea38b0 100644
>>>> --- a/net/sunrpc/clnt.c
>>>> +++ b/net/sunrpc/clnt.c
>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>>>>                            unsigned int base, unsigned int len,
>>>>                            unsigned int hdrsize)
>>>> {
>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
>>>> +       /* Subtract one to force an extra word of buffer space for the
>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
>>>> +        */
>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
>>>> +
>>>>       xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>>>>       trace_rpc_reply_pages(req);
>>>> }
>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>>> index 7cca515..aa8177d 100644
>>>> --- a/net/sunrpc/xdr.c
>>>> +++ b/net/sunrpc/xdr.c
>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>>>> 
>>>>       tail->iov_base = buf + offset;
>>>>       tail->iov_len = buflen - offset;
>>>> +       if ((xdr->page_len & 3) == 0)
>>>> +               tail->iov_len -= sizeof(__be32);
>>>> 
>>>>       xdr->buflen += len;
>>>> }
>>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever

Olga Kornievskaia April 5, 2019, 7:27 p.m. UTC | #5

On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>
> >>> Hi Chuck,
> >>>
> >>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> >>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> >>
> >> Possibly the macro that defines the maximum size of the reply
> >> is incorrect.
> >>
> >
> > This also breaks FS_LOCATION. I'm going to go on the limb here and say
> > that it probably breaks whatever else it modified.
>
> It modifies READ, READDIR, and READLINK. Are those broken?

I don't know how to test READLINK.. but I think READ/READDIR work OK
otherwise folks would have noticed it (I gather ACL and FS_LOCATION
testing doesn't happen frequently).

> > The question is: can't we just revert it??
>
> Why not "root cause" it first?

I'm trying :-/ I was just fishing to see how important the change was.

>
>
> >>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> Certain NFS results (eg. READLINK) might expect a data payload that
> >>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> >>>> is required to pad that payload so its length on the wire is a
> >>>> multiple of 4 bytes. The constants that define the maximum size of
> >>>> each NFS result do not appear to account for this extra word.
> >>>>
> >>>> In each case where the data payload is to be received into pages:
> >>>>
> >>>> - 1 word is added to the size of the receive buffer allocated by
> >>>> call_allocate
> >>>>
> >>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> >>>> extra buffer space falls into the rcv_buf's tail iovec
> >>>>
> >>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> >>>> is thus removed from the tail
> >>>>
> >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>> ---
> >>>> fs/nfs/nfs2xdr.c  |    6 +++---
> >>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> >>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> >>>> net/sunrpc/clnt.c |    6 +++++-
> >>>> net/sunrpc/xdr.c  |    2 ++
> >>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> >>>> index 1dcd0fe..a7ed29d 100644
> >>>> --- a/fs/nfs/nfs2xdr.c
> >>>> +++ b/fs/nfs/nfs2xdr.c
> >>>> @@ -56,11 +56,11 @@
> >>>>
> >>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> >>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> >>>> -#define NFS_readlinkres_sz     (2)
> >>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> >>>> +#define NFS_readlinkres_sz     (2+1)
> >>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> >>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> >>>> #define NFS_stat_sz            (1)
> >>>> -#define NFS_readdirres_sz      (1)
> >>>> +#define NFS_readdirres_sz      (1+1)
> >>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> >>>>
> >>>> static int nfs_stat_to_errno(enum nfs_stat);
> >>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> >>>> index a54dcf4..110358f 100644
> >>>> --- a/fs/nfs/nfs3xdr.c
> >>>> +++ b/fs/nfs/nfs3xdr.c
> >>>> @@ -69,13 +69,13 @@
> >>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> >>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> >>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> >>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> >>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> >>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> >>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> >>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> >>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> >>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> >>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> >>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> >>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> >>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> >>>> @@ -85,7 +85,7 @@
> >>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> >>>>                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> >>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> >>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> >>>>
> >>>> static int nfs3_stat_to_errno(enum nfs_stat);
> >>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> >>>>       result->op_status = status;
> >>>>       if (status != NFS3_OK)
> >>>>               goto out_status;
> >>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>>       error = decode_read3resok(xdr, result);
> >>>> out:
> >>>>       return error;
> >>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >>>> index d0fa18d..6d9d5e2 100644
> >>>> --- a/fs/nfs/nfs4xdr.c
> >>>> +++ b/fs/nfs/nfs4xdr.c
> >>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>                                nfs4_fattr_bitmap_maxsz)
> >>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> >>>>                                encode_stateid_maxsz + 3)
> >>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> >>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> >>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> >>>>                                2 + encode_verifier_maxsz + 5 + \
> >>>>                               nfs4_label_maxsz)
> >>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> >>>> -                                decode_verifier_maxsz)
> >>>> +                                decode_verifier_maxsz + 1)
> >>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> >>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> >>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> >>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> >>>>                                encode_stateid_maxsz + 4)
> >>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> >>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> >>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> >>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> >>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> >>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> >>>>                                encode_stateid_maxsz + 3)
> >>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> >>>> #define encode_fs_locations_maxsz \
> >>>>                               (encode_getattr_maxsz)
> >>>> #define decode_fs_locations_maxsz \
> >>>> -                               (0)
> >>>> +                               (1)
> >>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >>>>
> >>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>                               1 /* opaque devaddr4 length */ + \
> >>>>                                 /* devaddr4 payload is read into page */ \
> >>>>                               1 /* notification bitmap length */ + \
> >>>> -                               1 /* notification bitmap, word 0 */)
> >>>> +                               1 /* notification bitmap, word 0 */ + \
> >>>> +                               1 /* possible XDR padding */)
> >>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> >>>>                               encode_stateid_maxsz)
> >>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> >>>>                               decode_stateid_maxsz + \
> >>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> >>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> >>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> >>>>                               2 /* offset */ + \
> >>>>                               2 /* length */ + \
> >>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >>>> index f780605..4ea38b0 100644
> >>>> --- a/net/sunrpc/clnt.c
> >>>> +++ b/net/sunrpc/clnt.c
> >>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> >>>>                            unsigned int base, unsigned int len,
> >>>>                            unsigned int hdrsize)
> >>>> {
> >>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> >>>> +       /* Subtract one to force an extra word of buffer space for the
> >>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> >>>> +        */
> >>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> >>>> +
> >>>>       xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> >>>>       trace_rpc_reply_pages(req);
> >>>> }
> >>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >>>> index 7cca515..aa8177d 100644
> >>>> --- a/net/sunrpc/xdr.c
> >>>> +++ b/net/sunrpc/xdr.c
> >>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> >>>>
> >>>>       tail->iov_base = buf + offset;
> >>>>       tail->iov_len = buflen - offset;
> >>>> +       if ((xdr->page_len & 3) == 0)
> >>>> +               tail->iov_len -= sizeof(__be32);
> >>>>
> >>>>       xdr->buflen += len;
> >>>> }
> >>>>
> >>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

Chuck Lever April 5, 2019, 7:42 p.m. UTC | #6

> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> 
>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>> 
>>>>> Hi Chuck,
>>>>> 
>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
>>>> 
>>>> Possibly the macro that defines the maximum size of the reply
>>>> is incorrect.
>>>> 
>>> 
>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
>>> that it probably breaks whatever else it modified.
>> 
>> It modifies READ, READDIR, and READLINK. Are those broken?
> 
> I don't know how to test READLINK.. but I think READ/READDIR work OK
> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> testing doesn't happen frequently).

I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
tests in my automated unit tests.


>>> The question is: can't we just revert it??
>> 
>> Why not "root cause" it first?
> 
> I'm trying :-/ I was just fishing to see how important the change was.

Try reverting just this hunk:

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index d0fa18d..6d9d5e2 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
#define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
#define encode_getacl_maxsz	(encode_getattr_maxsz)
#define decode_getacl_maxsz	(op_decode_hdr_maxsz + \
-				 nfs4_fattr_bitmap_maxsz + 1)
+				 nfs4_fattr_bitmap_maxsz + 1 + 1)
#define encode_setacl_maxsz	(op_encode_hdr_maxsz + \
				 encode_stateid_maxsz + 3)
#define decode_setacl_maxsz	(decode_setattr_maxsz)
#define encode_fs_locations_maxsz \
				(encode_getattr_maxsz)
#define decode_fs_locations_maxsz \
-				(0)
+				(1)
#define encode_secinfo_maxsz	(op_encode_hdr_maxsz + nfs4_name_maxsz)
#define decode_secinfo_maxsz	(op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))


>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>> 
>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
>>>>>> is required to pad that payload so its length on the wire is a
>>>>>> multiple of 4 bytes. The constants that define the maximum size of
>>>>>> each NFS result do not appear to account for this extra word.
>>>>>> 
>>>>>> In each case where the data payload is to be received into pages:
>>>>>> 
>>>>>> - 1 word is added to the size of the receive buffer allocated by
>>>>>> call_allocate
>>>>>> 
>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>>>>>> extra buffer space falls into the rcv_buf's tail iovec
>>>>>> 
>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>>>>>> is thus removed from the tail
>>>>>> 
>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>> ---
>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>>>>>> net/sunrpc/clnt.c |    6 +++++-
>>>>>> net/sunrpc/xdr.c  |    2 ++
>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
>>>>>> 
>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
>>>>>> index 1dcd0fe..a7ed29d 100644
>>>>>> --- a/fs/nfs/nfs2xdr.c
>>>>>> +++ b/fs/nfs/nfs2xdr.c
>>>>>> @@ -56,11 +56,11 @@
>>>>>> 
>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
>>>>>> -#define NFS_readlinkres_sz     (2)
>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
>>>>>> +#define NFS_readlinkres_sz     (2+1)
>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
>>>>>> #define NFS_stat_sz            (1)
>>>>>> -#define NFS_readdirres_sz      (1)
>>>>>> +#define NFS_readdirres_sz      (1+1)
>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
>>>>>> 
>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
>>>>>> index a54dcf4..110358f 100644
>>>>>> --- a/fs/nfs/nfs3xdr.c
>>>>>> +++ b/fs/nfs/nfs3xdr.c
>>>>>> @@ -69,13 +69,13 @@
>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
>>>>>> @@ -85,7 +85,7 @@
>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>>>>>>                              XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>>>>>> 
>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>>>>>>      result->op_status = status;
>>>>>>      if (status != NFS3_OK)
>>>>>>              goto out_status;
>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>>      error = decode_read3resok(xdr, result);
>>>>>> out:
>>>>>>      return error;
>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>>>> index d0fa18d..6d9d5e2 100644
>>>>>> --- a/fs/nfs/nfs4xdr.c
>>>>>> +++ b/fs/nfs/nfs4xdr.c
>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>                               nfs4_fattr_bitmap_maxsz)
>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>>>>>>                               encode_stateid_maxsz + 3)
>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>>>>>>                               2 + encode_verifier_maxsz + 5 + \
>>>>>>                              nfs4_label_maxsz)
>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
>>>>>> -                                decode_verifier_maxsz)
>>>>>> +                                decode_verifier_maxsz + 1)
>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>>>>>>                               encode_stateid_maxsz + 4)
>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>>>>>>                               encode_stateid_maxsz + 3)
>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
>>>>>> #define encode_fs_locations_maxsz \
>>>>>>                              (encode_getattr_maxsz)
>>>>>> #define decode_fs_locations_maxsz \
>>>>>> -                               (0)
>>>>>> +                               (1)
>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>>>>>> 
>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>                              1 /* opaque devaddr4 length */ + \
>>>>>>                                /* devaddr4 payload is read into page */ \
>>>>>>                              1 /* notification bitmap length */ + \
>>>>>> -                               1 /* notification bitmap, word 0 */)
>>>>>> +                               1 /* notification bitmap, word 0 */ + \
>>>>>> +                               1 /* possible XDR padding */)
>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>>>>>>                              encode_stateid_maxsz)
>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>>>>>>                              decode_stateid_maxsz + \
>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>>>>>>                              2 /* offset */ + \
>>>>>>                              2 /* length */ + \
>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>>>> index f780605..4ea38b0 100644
>>>>>> --- a/net/sunrpc/clnt.c
>>>>>> +++ b/net/sunrpc/clnt.c
>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>>>>>>                           unsigned int base, unsigned int len,
>>>>>>                           unsigned int hdrsize)
>>>>>> {
>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
>>>>>> +       /* Subtract one to force an extra word of buffer space for the
>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
>>>>>> +        */
>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
>>>>>> +
>>>>>>      xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>>>>>>      trace_rpc_reply_pages(req);
>>>>>> }
>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>>>>> index 7cca515..aa8177d 100644
>>>>>> --- a/net/sunrpc/xdr.c
>>>>>> +++ b/net/sunrpc/xdr.c
>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>>>>>> 
>>>>>>      tail->iov_base = buf + offset;
>>>>>>      tail->iov_len = buflen - offset;
>>>>>> +       if ((xdr->page_len & 3) == 0)
>>>>>> +               tail->iov_len -= sizeof(__be32);
>>>>>> 
>>>>>>      xdr->buflen += len;
>>>>>> }
>>>>>> 
>>>> 
>>>> --
>>>> Chuck Lever
>> 
>> --
>> Chuck Lever

--
Chuck Lever

Olga Kornievskaia April 8, 2019, 2:36 p.m. UTC | #7

On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>
> >>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>>>
> >>>>> Hi Chuck,
> >>>>>
> >>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> >>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> >>>>
> >>>> Possibly the macro that defines the maximum size of the reply
> >>>> is incorrect.
> >>>>
> >>>
> >>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> >>> that it probably breaks whatever else it modified.
> >>
> >> It modifies READ, READDIR, and READLINK. Are those broken?
> >
> > I don't know how to test READLINK.. but I think READ/READDIR work OK
> > otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> > testing doesn't happen frequently).
>
> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
> tests in my automated unit tests.
>
>
> >>> The question is: can't we just revert it??
> >>
> >> Why not "root cause" it first?
> >
> > I'm trying :-/ I was just fishing to see how important the change was.
>
> Try reverting just this hunk:

That doesn't help. It seems to be this piece that's causing issues
hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1

With this there is an extra byte (in front) in the buffer when (ACL)
operation is decoded.

>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index d0fa18d..6d9d5e2 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> #define encode_getacl_maxsz     (encode_getattr_maxsz)
> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
> -                                nfs4_fattr_bitmap_maxsz + 1)
> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
>                                  encode_stateid_maxsz + 3)
> #define decode_setacl_maxsz     (decode_setattr_maxsz)
> #define encode_fs_locations_maxsz \
>                                 (encode_getattr_maxsz)
> #define decode_fs_locations_maxsz \
> -                               (0)
> +                               (1)
> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>
>
> >>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>
> >>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
> >>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> >>>>>> is required to pad that payload so its length on the wire is a
> >>>>>> multiple of 4 bytes. The constants that define the maximum size of
> >>>>>> each NFS result do not appear to account for this extra word.
> >>>>>>
> >>>>>> In each case where the data payload is to be received into pages:
> >>>>>>
> >>>>>> - 1 word is added to the size of the receive buffer allocated by
> >>>>>> call_allocate
> >>>>>>
> >>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> >>>>>> extra buffer space falls into the rcv_buf's tail iovec
> >>>>>>
> >>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> >>>>>> is thus removed from the tail
> >>>>>>
> >>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>>> ---
> >>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
> >>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> >>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> >>>>>> net/sunrpc/clnt.c |    6 +++++-
> >>>>>> net/sunrpc/xdr.c  |    2 ++
> >>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> >>>>>> index 1dcd0fe..a7ed29d 100644
> >>>>>> --- a/fs/nfs/nfs2xdr.c
> >>>>>> +++ b/fs/nfs/nfs2xdr.c
> >>>>>> @@ -56,11 +56,11 @@
> >>>>>>
> >>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> >>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> >>>>>> -#define NFS_readlinkres_sz     (2)
> >>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> >>>>>> +#define NFS_readlinkres_sz     (2+1)
> >>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> >>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> >>>>>> #define NFS_stat_sz            (1)
> >>>>>> -#define NFS_readdirres_sz      (1)
> >>>>>> +#define NFS_readdirres_sz      (1+1)
> >>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> >>>>>>
> >>>>>> static int nfs_stat_to_errno(enum nfs_stat);
> >>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> >>>>>> index a54dcf4..110358f 100644
> >>>>>> --- a/fs/nfs/nfs3xdr.c
> >>>>>> +++ b/fs/nfs/nfs3xdr.c
> >>>>>> @@ -69,13 +69,13 @@
> >>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> >>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> >>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> >>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> >>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> >>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> >>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> >>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> >>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> >>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> >>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> >>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> >>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> >>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> >>>>>> @@ -85,7 +85,7 @@
> >>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> >>>>>>                              XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> >>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> >>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> >>>>>>
> >>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
> >>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> >>>>>>      result->op_status = status;
> >>>>>>      if (status != NFS3_OK)
> >>>>>>              goto out_status;
> >>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>>>>      error = decode_read3resok(xdr, result);
> >>>>>> out:
> >>>>>>      return error;
> >>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >>>>>> index d0fa18d..6d9d5e2 100644
> >>>>>> --- a/fs/nfs/nfs4xdr.c
> >>>>>> +++ b/fs/nfs/nfs4xdr.c
> >>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>>                               nfs4_fattr_bitmap_maxsz)
> >>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> >>>>>>                               encode_stateid_maxsz + 3)
> >>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> >>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> >>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> >>>>>>                               2 + encode_verifier_maxsz + 5 + \
> >>>>>>                              nfs4_label_maxsz)
> >>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> >>>>>> -                                decode_verifier_maxsz)
> >>>>>> +                                decode_verifier_maxsz + 1)
> >>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> >>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> >>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> >>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> >>>>>>                               encode_stateid_maxsz + 4)
> >>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> >>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> >>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> >>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> >>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> >>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> >>>>>>                               encode_stateid_maxsz + 3)
> >>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> >>>>>> #define encode_fs_locations_maxsz \
> >>>>>>                              (encode_getattr_maxsz)
> >>>>>> #define decode_fs_locations_maxsz \
> >>>>>> -                               (0)
> >>>>>> +                               (1)
> >>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >>>>>>
> >>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>>                              1 /* opaque devaddr4 length */ + \
> >>>>>>                                /* devaddr4 payload is read into page */ \
> >>>>>>                              1 /* notification bitmap length */ + \
> >>>>>> -                               1 /* notification bitmap, word 0 */)
> >>>>>> +                               1 /* notification bitmap, word 0 */ + \
> >>>>>> +                               1 /* possible XDR padding */)
> >>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> >>>>>>                              encode_stateid_maxsz)
> >>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> >>>>>>                              decode_stateid_maxsz + \
> >>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> >>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> >>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> >>>>>>                              2 /* offset */ + \
> >>>>>>                              2 /* length */ + \
> >>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >>>>>> index f780605..4ea38b0 100644
> >>>>>> --- a/net/sunrpc/clnt.c
> >>>>>> +++ b/net/sunrpc/clnt.c
> >>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> >>>>>>                           unsigned int base, unsigned int len,
> >>>>>>                           unsigned int hdrsize)
> >>>>>> {
> >>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> >>>>>> +       /* Subtract one to force an extra word of buffer space for the
> >>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> >>>>>> +        */
> >>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> >>>>>> +
> >>>>>>      xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> >>>>>>      trace_rpc_reply_pages(req);
> >>>>>> }
> >>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >>>>>> index 7cca515..aa8177d 100644
> >>>>>> --- a/net/sunrpc/xdr.c
> >>>>>> +++ b/net/sunrpc/xdr.c
> >>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> >>>>>>
> >>>>>>      tail->iov_base = buf + offset;
> >>>>>>      tail->iov_len = buflen - offset;
> >>>>>> +       if ((xdr->page_len & 3) == 0)
> >>>>>> +               tail->iov_len -= sizeof(__be32);
> >>>>>>
> >>>>>>      xdr->buflen += len;
> >>>>>> }
> >>>>>>
> >>>>
> >>>> --
> >>>> Chuck Lever
> >>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

Chuck Lever April 8, 2019, 2:43 p.m. UTC | #8

> On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> 
>>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>> 
>>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>> 
>>>>>>> Hi Chuck,
>>>>>>> 
>>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
>>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
>>>>>> 
>>>>>> Possibly the macro that defines the maximum size of the reply
>>>>>> is incorrect.
>>>>>> 
>>>>> 
>>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
>>>>> that it probably breaks whatever else it modified.
>>>> 
>>>> It modifies READ, READDIR, and READLINK. Are those broken?
>>> 
>>> I don't know how to test READLINK.. but I think READ/READDIR work OK
>>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
>>> testing doesn't happen frequently).
>> 
>> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
>> tests in my automated unit tests.
>> 
>> 
>>>>> The question is: can't we just revert it??
>>>> 
>>>> Why not "root cause" it first?
>>> 
>>> I'm trying :-/ I was just fishing to see how important the change was.
>> 
>> Try reverting just this hunk:
> 
> That doesn't help. It seems to be this piece that's causing issues
> hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
> 
> With this there is an extra byte (in front) in the buffer when (ACL)
> operation is decoded.

How do you know there isn't a latent bug in the getfacl decoder?

How are you reproducing this issue? I can try it here later today.


>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index d0fa18d..6d9d5e2 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>> #define encode_getacl_maxsz     (encode_getattr_maxsz)
>> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
>> -                                nfs4_fattr_bitmap_maxsz + 1)
>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
>>                                 encode_stateid_maxsz + 3)
>> #define decode_setacl_maxsz     (decode_setattr_maxsz)
>> #define encode_fs_locations_maxsz \
>>                                (encode_getattr_maxsz)
>> #define decode_fs_locations_maxsz \
>> -                               (0)
>> +                               (1)
>> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
>> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>> 
>> 
>>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
>>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
>>>>>>>> is required to pad that payload so its length on the wire is a
>>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
>>>>>>>> each NFS result do not appear to account for this extra word.
>>>>>>>> 
>>>>>>>> In each case where the data payload is to be received into pages:
>>>>>>>> 
>>>>>>>> - 1 word is added to the size of the receive buffer allocated by
>>>>>>>> call_allocate
>>>>>>>> 
>>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
>>>>>>>> 
>>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>>>>>>>> is thus removed from the tail
>>>>>>>> 
>>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>> ---
>>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
>>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
>>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>>>>>>>> net/sunrpc/clnt.c |    6 +++++-
>>>>>>>> net/sunrpc/xdr.c  |    2 ++
>>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
>>>>>>>> index 1dcd0fe..a7ed29d 100644
>>>>>>>> --- a/fs/nfs/nfs2xdr.c
>>>>>>>> +++ b/fs/nfs/nfs2xdr.c
>>>>>>>> @@ -56,11 +56,11 @@
>>>>>>>> 
>>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
>>>>>>>> -#define NFS_readlinkres_sz     (2)
>>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
>>>>>>>> +#define NFS_readlinkres_sz     (2+1)
>>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
>>>>>>>> #define NFS_stat_sz            (1)
>>>>>>>> -#define NFS_readdirres_sz      (1)
>>>>>>>> +#define NFS_readdirres_sz      (1+1)
>>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
>>>>>>>> 
>>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
>>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
>>>>>>>> index a54dcf4..110358f 100644
>>>>>>>> --- a/fs/nfs/nfs3xdr.c
>>>>>>>> +++ b/fs/nfs/nfs3xdr.c
>>>>>>>> @@ -69,13 +69,13 @@
>>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
>>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
>>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
>>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
>>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
>>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
>>>>>>>> @@ -85,7 +85,7 @@
>>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>>>>>>>>                             XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
>>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>>>>>>>> 
>>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
>>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>>>>>>>>     result->op_status = status;
>>>>>>>>     if (status != NFS3_OK)
>>>>>>>>             goto out_status;
>>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>>>>     error = decode_read3resok(xdr, result);
>>>>>>>> out:
>>>>>>>>     return error;
>>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>>>>>> index d0fa18d..6d9d5e2 100644
>>>>>>>> --- a/fs/nfs/nfs4xdr.c
>>>>>>>> +++ b/fs/nfs/nfs4xdr.c
>>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>>                              nfs4_fattr_bitmap_maxsz)
>>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>>>>>>>>                              encode_stateid_maxsz + 3)
>>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
>>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>>>>>>>>                              2 + encode_verifier_maxsz + 5 + \
>>>>>>>>                             nfs4_label_maxsz)
>>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
>>>>>>>> -                                decode_verifier_maxsz)
>>>>>>>> +                                decode_verifier_maxsz + 1)
>>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
>>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
>>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>>>>>>>>                              encode_stateid_maxsz + 4)
>>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
>>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
>>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
>>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
>>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>>>>>>>>                              encode_stateid_maxsz + 3)
>>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
>>>>>>>> #define encode_fs_locations_maxsz \
>>>>>>>>                             (encode_getattr_maxsz)
>>>>>>>> #define decode_fs_locations_maxsz \
>>>>>>>> -                               (0)
>>>>>>>> +                               (1)
>>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>>>>>>>> 
>>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>>                             1 /* opaque devaddr4 length */ + \
>>>>>>>>                               /* devaddr4 payload is read into page */ \
>>>>>>>>                             1 /* notification bitmap length */ + \
>>>>>>>> -                               1 /* notification bitmap, word 0 */)
>>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
>>>>>>>> +                               1 /* possible XDR padding */)
>>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>>>>>>>>                             encode_stateid_maxsz)
>>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>>>>>>>>                             decode_stateid_maxsz + \
>>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
>>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>>>>>>>>                             2 /* offset */ + \
>>>>>>>>                             2 /* length */ + \
>>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>>>>>> index f780605..4ea38b0 100644
>>>>>>>> --- a/net/sunrpc/clnt.c
>>>>>>>> +++ b/net/sunrpc/clnt.c
>>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>>>>>>>>                          unsigned int base, unsigned int len,
>>>>>>>>                          unsigned int hdrsize)
>>>>>>>> {
>>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
>>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
>>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
>>>>>>>> +        */
>>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
>>>>>>>> +
>>>>>>>>     xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>>>>>>>>     trace_rpc_reply_pages(req);
>>>>>>>> }
>>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>>>>>>> index 7cca515..aa8177d 100644
>>>>>>>> --- a/net/sunrpc/xdr.c
>>>>>>>> +++ b/net/sunrpc/xdr.c
>>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>>>>>>>> 
>>>>>>>>     tail->iov_base = buf + offset;
>>>>>>>>     tail->iov_len = buflen - offset;
>>>>>>>> +       if ((xdr->page_len & 3) == 0)
>>>>>>>> +               tail->iov_len -= sizeof(__be32);
>>>>>>>> 
>>>>>>>>     xdr->buflen += len;
>>>>>>>> }
>>>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> Chuck Lever
>>>> 
>>>> --
>>>> Chuck Lever
>> 
>> --
>> Chuck Lever

--
Chuck Lever

Olga Kornievskaia April 8, 2019, 3:21 p.m. UTC | #9

On Mon, Apr 8, 2019 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>
> >>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>>>
> >>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >>>>>>>
> >>>>>>> Hi Chuck,
> >>>>>>>
> >>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> >>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> >>>>>>
> >>>>>> Possibly the macro that defines the maximum size of the reply
> >>>>>> is incorrect.
> >>>>>>
> >>>>>
> >>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> >>>>> that it probably breaks whatever else it modified.
> >>>>
> >>>> It modifies READ, READDIR, and READLINK. Are those broken?
> >>>
> >>> I don't know how to test READLINK.. but I think READ/READDIR work OK
> >>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> >>> testing doesn't happen frequently).
> >>
> >> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
> >> tests in my automated unit tests.
> >>
> >>
> >>>>> The question is: can't we just revert it??
> >>>>
> >>>> Why not "root cause" it first?
> >>>
> >>> I'm trying :-/ I was just fishing to see how important the change was.
> >>
> >> Try reverting just this hunk:
> >
> > That doesn't help. It seems to be this piece that's causing issues
> > hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
> >
> > With this there is an extra byte (in front) in the buffer when (ACL)
> > operation is decoded.
>
> How do you know there isn't a latent bug in the getfacl decoder?

I don't. All I know is that it passed tests before and now it doesn't.

> How are you reproducing this issue? I can try it here later today.

The issue was found running xfstest nfs/001. However, you don't need
that: (1) mount (2) nfs4_getfacl <file>

To understand a patch, does it fix a problem with READLINK or is the
an optimization?

>
>
> >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >> index d0fa18d..6d9d5e2 100644
> >> --- a/fs/nfs/nfs4xdr.c
> >> +++ b/fs/nfs/nfs4xdr.c
> >> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >> #define encode_getacl_maxsz     (encode_getattr_maxsz)
> >> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
> >> -                                nfs4_fattr_bitmap_maxsz + 1)
> >> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> >> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
> >>                                 encode_stateid_maxsz + 3)
> >> #define decode_setacl_maxsz     (decode_setattr_maxsz)
> >> #define encode_fs_locations_maxsz \
> >>                                (encode_getattr_maxsz)
> >> #define decode_fs_locations_maxsz \
> >> -                               (0)
> >> +                               (1)
> >> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >>
> >>
> >>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>>>
> >>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
> >>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> >>>>>>>> is required to pad that payload so its length on the wire is a
> >>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
> >>>>>>>> each NFS result do not appear to account for this extra word.
> >>>>>>>>
> >>>>>>>> In each case where the data payload is to be received into pages:
> >>>>>>>>
> >>>>>>>> - 1 word is added to the size of the receive buffer allocated by
> >>>>>>>> call_allocate
> >>>>>>>>
> >>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> >>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
> >>>>>>>>
> >>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> >>>>>>>> is thus removed from the tail
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>>>>> ---
> >>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
> >>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> >>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> >>>>>>>> net/sunrpc/clnt.c |    6 +++++-
> >>>>>>>> net/sunrpc/xdr.c  |    2 ++
> >>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> >>>>>>>> index 1dcd0fe..a7ed29d 100644
> >>>>>>>> --- a/fs/nfs/nfs2xdr.c
> >>>>>>>> +++ b/fs/nfs/nfs2xdr.c
> >>>>>>>> @@ -56,11 +56,11 @@
> >>>>>>>>
> >>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> >>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> >>>>>>>> -#define NFS_readlinkres_sz     (2)
> >>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> >>>>>>>> +#define NFS_readlinkres_sz     (2+1)
> >>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> >>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> >>>>>>>> #define NFS_stat_sz            (1)
> >>>>>>>> -#define NFS_readdirres_sz      (1)
> >>>>>>>> +#define NFS_readdirres_sz      (1+1)
> >>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> >>>>>>>>
> >>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
> >>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> >>>>>>>> index a54dcf4..110358f 100644
> >>>>>>>> --- a/fs/nfs/nfs3xdr.c
> >>>>>>>> +++ b/fs/nfs/nfs3xdr.c
> >>>>>>>> @@ -69,13 +69,13 @@
> >>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> >>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> >>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> >>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> >>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> >>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> >>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> >>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> >>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> >>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> >>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> >>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> >>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> >>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> >>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> >>>>>>>> @@ -85,7 +85,7 @@
> >>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> >>>>>>>>                             XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> >>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> >>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> >>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> >>>>>>>>
> >>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
> >>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> >>>>>>>>     result->op_status = status;
> >>>>>>>>     if (status != NFS3_OK)
> >>>>>>>>             goto out_status;
> >>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> >>>>>>>>     error = decode_read3resok(xdr, result);
> >>>>>>>> out:
> >>>>>>>>     return error;
> >>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >>>>>>>> index d0fa18d..6d9d5e2 100644
> >>>>>>>> --- a/fs/nfs/nfs4xdr.c
> >>>>>>>> +++ b/fs/nfs/nfs4xdr.c
> >>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>>>>                              nfs4_fattr_bitmap_maxsz)
> >>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> >>>>>>>>                              encode_stateid_maxsz + 3)
> >>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> >>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> >>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> >>>>>>>>                              2 + encode_verifier_maxsz + 5 + \
> >>>>>>>>                             nfs4_label_maxsz)
> >>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> >>>>>>>> -                                decode_verifier_maxsz)
> >>>>>>>> +                                decode_verifier_maxsz + 1)
> >>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> >>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> >>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> >>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> >>>>>>>>                              encode_stateid_maxsz + 4)
> >>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> >>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> >>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> >>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> >>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> >>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> >>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> >>>>>>>>                              encode_stateid_maxsz + 3)
> >>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> >>>>>>>> #define encode_fs_locations_maxsz \
> >>>>>>>>                             (encode_getattr_maxsz)
> >>>>>>>> #define decode_fs_locations_maxsz \
> >>>>>>>> -                               (0)
> >>>>>>>> +                               (1)
> >>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> >>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> >>>>>>>>
> >>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> >>>>>>>>                             1 /* opaque devaddr4 length */ + \
> >>>>>>>>                               /* devaddr4 payload is read into page */ \
> >>>>>>>>                             1 /* notification bitmap length */ + \
> >>>>>>>> -                               1 /* notification bitmap, word 0 */)
> >>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
> >>>>>>>> +                               1 /* possible XDR padding */)
> >>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> >>>>>>>>                             encode_stateid_maxsz)
> >>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> >>>>>>>>                             decode_stateid_maxsz + \
> >>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> >>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> >>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> >>>>>>>>                             2 /* offset */ + \
> >>>>>>>>                             2 /* length */ + \
> >>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >>>>>>>> index f780605..4ea38b0 100644
> >>>>>>>> --- a/net/sunrpc/clnt.c
> >>>>>>>> +++ b/net/sunrpc/clnt.c
> >>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> >>>>>>>>                          unsigned int base, unsigned int len,
> >>>>>>>>                          unsigned int hdrsize)
> >>>>>>>> {
> >>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> >>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
> >>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> >>>>>>>> +        */
> >>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> >>>>>>>> +
> >>>>>>>>     xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> >>>>>>>>     trace_rpc_reply_pages(req);
> >>>>>>>> }
> >>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >>>>>>>> index 7cca515..aa8177d 100644
> >>>>>>>> --- a/net/sunrpc/xdr.c
> >>>>>>>> +++ b/net/sunrpc/xdr.c
> >>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> >>>>>>>>
> >>>>>>>>     tail->iov_base = buf + offset;
> >>>>>>>>     tail->iov_len = buflen - offset;
> >>>>>>>> +       if ((xdr->page_len & 3) == 0)
> >>>>>>>> +               tail->iov_len -= sizeof(__be32);
> >>>>>>>>
> >>>>>>>>     xdr->buflen += len;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Chuck Lever
> >>>>
> >>>> --
> >>>> Chuck Lever
> >>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

Olga Kornievskaia April 8, 2019, 3:26 p.m. UTC | #10

On Mon, Apr 8, 2019 at 11:21 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Mon, Apr 8, 2019 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> >
> >
> > > On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > >
> > > On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >>
> > >>
> > >>
> > >>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > >>>
> > >>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > >>>>>
> > >>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > >>>>>>>
> > >>>>>>> Hi Chuck,
> > >>>>>>>
> > >>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> > >>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> > >>>>>>
> > >>>>>> Possibly the macro that defines the maximum size of the reply
> > >>>>>> is incorrect.
> > >>>>>>
> > >>>>>
> > >>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> > >>>>> that it probably breaks whatever else it modified.
> > >>>>
> > >>>> It modifies READ, READDIR, and READLINK. Are those broken?
> > >>>
> > >>> I don't know how to test READLINK.. but I think READ/READDIR work OK
> > >>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> > >>> testing doesn't happen frequently).
> > >>
> > >> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
> > >> tests in my automated unit tests.
> > >>
> > >>
> > >>>>> The question is: can't we just revert it??
> > >>>>
> > >>>> Why not "root cause" it first?
> > >>>
> > >>> I'm trying :-/ I was just fishing to see how important the change was.
> > >>
> > >> Try reverting just this hunk:
> > >
> > > That doesn't help. It seems to be this piece that's causing issues
> > > hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
> > >
> > > With this there is an extra byte (in front) in the buffer when (ACL)
> > > operation is decoded.
> >
> > How do you know there isn't a latent bug in the getfacl decoder?
>
> I don't. All I know is that it passed tests before and now it doesn't.

Also this bug will have to be in both getfacl and fs_location code.
What they both share is xd_enter_page() code that now with new
semantics makes the buffer point to the wrong place.

> > How are you reproducing this issue? I can try it here later today.
>
> The issue was found running xfstest nfs/001. However, you don't need
> that: (1) mount (2) nfs4_getfacl <file>
>
> To understand a patch, does it fix a problem with READLINK or is the
> an optimization?
>
> >
> >
> > >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > >> index d0fa18d..6d9d5e2 100644
> > >> --- a/fs/nfs/nfs4xdr.c
> > >> +++ b/fs/nfs/nfs4xdr.c
> > >> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > >> #define encode_getacl_maxsz     (encode_getattr_maxsz)
> > >> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
> > >> -                                nfs4_fattr_bitmap_maxsz + 1)
> > >> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > >> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
> > >>                                 encode_stateid_maxsz + 3)
> > >> #define decode_setacl_maxsz     (decode_setattr_maxsz)
> > >> #define encode_fs_locations_maxsz \
> > >>                                (encode_getattr_maxsz)
> > >> #define decode_fs_locations_maxsz \
> > >> -                               (0)
> > >> +                               (1)
> > >> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > >> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > >>
> > >>
> > >>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >>>>>>>>
> > >>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
> > >>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> > >>>>>>>> is required to pad that payload so its length on the wire is a
> > >>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
> > >>>>>>>> each NFS result do not appear to account for this extra word.
> > >>>>>>>>
> > >>>>>>>> In each case where the data payload is to be received into pages:
> > >>>>>>>>
> > >>>>>>>> - 1 word is added to the size of the receive buffer allocated by
> > >>>>>>>> call_allocate
> > >>>>>>>>
> > >>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> > >>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
> > >>>>>>>>
> > >>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> > >>>>>>>> is thus removed from the tail
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > >>>>>>>> ---
> > >>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
> > >>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> > >>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> > >>>>>>>> net/sunrpc/clnt.c |    6 +++++-
> > >>>>>>>> net/sunrpc/xdr.c  |    2 ++
> > >>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> > >>>>>>>>
> > >>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> > >>>>>>>> index 1dcd0fe..a7ed29d 100644
> > >>>>>>>> --- a/fs/nfs/nfs2xdr.c
> > >>>>>>>> +++ b/fs/nfs/nfs2xdr.c
> > >>>>>>>> @@ -56,11 +56,11 @@
> > >>>>>>>>
> > >>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> > >>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> > >>>>>>>> -#define NFS_readlinkres_sz     (2)
> > >>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> > >>>>>>>> +#define NFS_readlinkres_sz     (2+1)
> > >>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> > >>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> > >>>>>>>> #define NFS_stat_sz            (1)
> > >>>>>>>> -#define NFS_readdirres_sz      (1)
> > >>>>>>>> +#define NFS_readdirres_sz      (1+1)
> > >>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> > >>>>>>>>
> > >>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
> > >>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> > >>>>>>>> index a54dcf4..110358f 100644
> > >>>>>>>> --- a/fs/nfs/nfs3xdr.c
> > >>>>>>>> +++ b/fs/nfs/nfs3xdr.c
> > >>>>>>>> @@ -69,13 +69,13 @@
> > >>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> > >>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> > >>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> > >>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> > >>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> > >>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> > >>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> > >>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> > >>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > >>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> > >>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > >>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> > >>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> > >>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> > >>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> > >>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> > >>>>>>>> @@ -85,7 +85,7 @@
> > >>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> > >>>>>>>>                             XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > >>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> > >>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > >>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> > >>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> > >>>>>>>>
> > >>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
> > >>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> > >>>>>>>>     result->op_status = status;
> > >>>>>>>>     if (status != NFS3_OK)
> > >>>>>>>>             goto out_status;
> > >>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > >>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > >>>>>>>>     error = decode_read3resok(xdr, result);
> > >>>>>>>> out:
> > >>>>>>>>     return error;
> > >>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > >>>>>>>> index d0fa18d..6d9d5e2 100644
> > >>>>>>>> --- a/fs/nfs/nfs4xdr.c
> > >>>>>>>> +++ b/fs/nfs/nfs4xdr.c
> > >>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >>>>>>>>                              nfs4_fattr_bitmap_maxsz)
> > >>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> > >>>>>>>>                              encode_stateid_maxsz + 3)
> > >>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> > >>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> > >>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> > >>>>>>>>                              2 + encode_verifier_maxsz + 5 + \
> > >>>>>>>>                             nfs4_label_maxsz)
> > >>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> > >>>>>>>> -                                decode_verifier_maxsz)
> > >>>>>>>> +                                decode_verifier_maxsz + 1)
> > >>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> > >>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> > >>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> > >>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> > >>>>>>>>                              encode_stateid_maxsz + 4)
> > >>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> > >>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > >>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> > >>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> > >>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> > >>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > >>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> > >>>>>>>>                              encode_stateid_maxsz + 3)
> > >>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> > >>>>>>>> #define encode_fs_locations_maxsz \
> > >>>>>>>>                             (encode_getattr_maxsz)
> > >>>>>>>> #define decode_fs_locations_maxsz \
> > >>>>>>>> -                               (0)
> > >>>>>>>> +                               (1)
> > >>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > >>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > >>>>>>>>
> > >>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > >>>>>>>>                             1 /* opaque devaddr4 length */ + \
> > >>>>>>>>                               /* devaddr4 payload is read into page */ \
> > >>>>>>>>                             1 /* notification bitmap length */ + \
> > >>>>>>>> -                               1 /* notification bitmap, word 0 */)
> > >>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
> > >>>>>>>> +                               1 /* possible XDR padding */)
> > >>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> > >>>>>>>>                             encode_stateid_maxsz)
> > >>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> > >>>>>>>>                             decode_stateid_maxsz + \
> > >>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> > >>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> > >>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> > >>>>>>>>                             2 /* offset */ + \
> > >>>>>>>>                             2 /* length */ + \
> > >>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > >>>>>>>> index f780605..4ea38b0 100644
> > >>>>>>>> --- a/net/sunrpc/clnt.c
> > >>>>>>>> +++ b/net/sunrpc/clnt.c
> > >>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> > >>>>>>>>                          unsigned int base, unsigned int len,
> > >>>>>>>>                          unsigned int hdrsize)
> > >>>>>>>> {
> > >>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> > >>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
> > >>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> > >>>>>>>> +        */
> > >>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> > >>>>>>>> +
> > >>>>>>>>     xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> > >>>>>>>>     trace_rpc_reply_pages(req);
> > >>>>>>>> }
> > >>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > >>>>>>>> index 7cca515..aa8177d 100644
> > >>>>>>>> --- a/net/sunrpc/xdr.c
> > >>>>>>>> +++ b/net/sunrpc/xdr.c
> > >>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> > >>>>>>>>
> > >>>>>>>>     tail->iov_base = buf + offset;
> > >>>>>>>>     tail->iov_len = buflen - offset;
> > >>>>>>>> +       if ((xdr->page_len & 3) == 0)
> > >>>>>>>> +               tail->iov_len -= sizeof(__be32);
> > >>>>>>>>
> > >>>>>>>>     xdr->buflen += len;
> > >>>>>>>> }
> > >>>>>>>>
> > >>>>>>
> > >>>>>> --
> > >>>>>> Chuck Lever
> > >>>>
> > >>>> --
> > >>>> Chuck Lever
> > >>
> > >> --
> > >> Chuck Lever
> >
> > --
> > Chuck Lever
> >
> >
> >

Olga Kornievskaia April 8, 2019, 3:50 p.m. UTC | #11

On Mon, Apr 8, 2019 at 11:26 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Mon, Apr 8, 2019 at 11:21 AM Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Mon, Apr 8, 2019 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >
> > >
> > >
> > > > On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > >
> > > > On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > >>>
> > > >>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > >>>>>
> > > >>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > >>>>>>>
> > > >>>>>>> Hi Chuck,
> > > >>>>>>>
> > > >>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> > > >>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> > > >>>>>>
> > > >>>>>> Possibly the macro that defines the maximum size of the reply
> > > >>>>>> is incorrect.
> > > >>>>>>
> > > >>>>>
> > > >>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> > > >>>>> that it probably breaks whatever else it modified.
> > > >>>>
> > > >>>> It modifies READ, READDIR, and READLINK. Are those broken?
> > > >>>
> > > >>> I don't know how to test READLINK.. but I think READ/READDIR work OK
> > > >>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> > > >>> testing doesn't happen frequently).
> > > >>
> > > >> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
> > > >> tests in my automated unit tests.
> > > >>
> > > >>
> > > >>>>> The question is: can't we just revert it??
> > > >>>>
> > > >>>> Why not "root cause" it first?
> > > >>>
> > > >>> I'm trying :-/ I was just fishing to see how important the change was.
> > > >>
> > > >> Try reverting just this hunk:
> > > >
> > > > That doesn't help. It seems to be this piece that's causing issues
> > > > hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
> > > >
> > > > With this there is an extra byte (in front) in the buffer when (ACL)
> > > > operation is decoded.
> > >
> > > How do you know there isn't a latent bug in the getfacl decoder?
> >
> > I don't. All I know is that it passed tests before and now it doesn't.
>
> Also this bug will have to be in both getfacl and fs_location code.
> What they both share is xd_enter_page() code that now with new
> semantics makes the buffer point to the wrong place.
>
> > > How are you reproducing this issue? I can try it here later today.
> >
> > The issue was found running xfstest nfs/001. However, you don't need
> > that: (1) mount (2) nfs4_getfacl <file>
> >
> > To understand a patch, does it fix a problem with READLINK or is the
> > an optimization?

So this "fixes" it but this don't look really good.

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index cfcabc3..f2a5553 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c

@@ -5280,6 +5280,7 @@ static int decode_getacl(struct xdr_stream *xdr,
struct rpc_rqst *req,
  goto out;
  xdr_enter_page(xdr, xdr->buf->page_len);
+ xdr->p++;

  /* Calculate the offset of the page data */
  pg_offset = xdr->buf->head[0].iov_len;
@@ -6949,6 +6950,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
  goto out;
  if (res->migration) {
  xdr_enter_page(xdr, PAGE_SIZE);
+ xdr->p++;
  status = decode_getfattr_generic(xdr,
  &res->fs_locations->fattr,
  NULL, res->fs_locations,
@@ -6962,6 +6964,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
  if (status)
  goto out;
  xdr_enter_page(xdr, PAGE_SIZE);
+ xdr->p++;
  status = decode_getfattr_generic(xdr,
  &res->fs_locations->fattr,
  NULL, res->fs_locations,


> >
> > >
> > >
> > > >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > >> index d0fa18d..6d9d5e2 100644
> > > >> --- a/fs/nfs/nfs4xdr.c
> > > >> +++ b/fs/nfs/nfs4xdr.c
> > > >> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > >> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > > >> #define encode_getacl_maxsz     (encode_getattr_maxsz)
> > > >> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
> > > >> -                                nfs4_fattr_bitmap_maxsz + 1)
> > > >> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > > >> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
> > > >>                                 encode_stateid_maxsz + 3)
> > > >> #define decode_setacl_maxsz     (decode_setattr_maxsz)
> > > >> #define encode_fs_locations_maxsz \
> > > >>                                (encode_getattr_maxsz)
> > > >> #define decode_fs_locations_maxsz \
> > > >> -                               (0)
> > > >> +                               (1)
> > > >> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > > >> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > > >>
> > > >>
> > > >>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > >>>>>>>>
> > > >>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
> > > >>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> > > >>>>>>>> is required to pad that payload so its length on the wire is a
> > > >>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
> > > >>>>>>>> each NFS result do not appear to account for this extra word.
> > > >>>>>>>>
> > > >>>>>>>> In each case where the data payload is to be received into pages:
> > > >>>>>>>>
> > > >>>>>>>> - 1 word is added to the size of the receive buffer allocated by
> > > >>>>>>>> call_allocate
> > > >>>>>>>>
> > > >>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> > > >>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
> > > >>>>>>>>
> > > >>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> > > >>>>>>>> is thus removed from the tail
> > > >>>>>>>>
> > > >>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > >>>>>>>> ---
> > > >>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
> > > >>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> > > >>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> > > >>>>>>>> net/sunrpc/clnt.c |    6 +++++-
> > > >>>>>>>> net/sunrpc/xdr.c  |    2 ++
> > > >>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> > > >>>>>>>>
> > > >>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> > > >>>>>>>> index 1dcd0fe..a7ed29d 100644
> > > >>>>>>>> --- a/fs/nfs/nfs2xdr.c
> > > >>>>>>>> +++ b/fs/nfs/nfs2xdr.c
> > > >>>>>>>> @@ -56,11 +56,11 @@
> > > >>>>>>>>
> > > >>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> > > >>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> > > >>>>>>>> -#define NFS_readlinkres_sz     (2)
> > > >>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> > > >>>>>>>> +#define NFS_readlinkres_sz     (2+1)
> > > >>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> > > >>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> > > >>>>>>>> #define NFS_stat_sz            (1)
> > > >>>>>>>> -#define NFS_readdirres_sz      (1)
> > > >>>>>>>> +#define NFS_readdirres_sz      (1+1)
> > > >>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> > > >>>>>>>>
> > > >>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
> > > >>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> > > >>>>>>>> index a54dcf4..110358f 100644
> > > >>>>>>>> --- a/fs/nfs/nfs3xdr.c
> > > >>>>>>>> +++ b/fs/nfs/nfs3xdr.c
> > > >>>>>>>> @@ -69,13 +69,13 @@
> > > >>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> > > >>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> > > >>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> > > >>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> > > >>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> > > >>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> > > >>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> > > >>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> > > >>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > > >>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> > > >>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > > >>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> > > >>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> > > >>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> > > >>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> > > >>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> > > >>>>>>>> @@ -85,7 +85,7 @@
> > > >>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> > > >>>>>>>>                             XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > > >>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> > > >>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > > >>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> > > >>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> > > >>>>>>>>
> > > >>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
> > > >>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> > > >>>>>>>>     result->op_status = status;
> > > >>>>>>>>     if (status != NFS3_OK)
> > > >>>>>>>>             goto out_status;
> > > >>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > > >>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > > >>>>>>>>     error = decode_read3resok(xdr, result);
> > > >>>>>>>> out:
> > > >>>>>>>>     return error;
> > > >>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > >>>>>>>> index d0fa18d..6d9d5e2 100644
> > > >>>>>>>> --- a/fs/nfs/nfs4xdr.c
> > > >>>>>>>> +++ b/fs/nfs/nfs4xdr.c
> > > >>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > >>>>>>>>                              nfs4_fattr_bitmap_maxsz)
> > > >>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> > > >>>>>>>>                              encode_stateid_maxsz + 3)
> > > >>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> > > >>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> > > >>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> > > >>>>>>>>                              2 + encode_verifier_maxsz + 5 + \
> > > >>>>>>>>                             nfs4_label_maxsz)
> > > >>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> > > >>>>>>>> -                                decode_verifier_maxsz)
> > > >>>>>>>> +                                decode_verifier_maxsz + 1)
> > > >>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> > > >>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> > > >>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> > > >>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> > > >>>>>>>>                              encode_stateid_maxsz + 4)
> > > >>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> > > >>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > >>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > > >>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> > > >>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> > > >>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> > > >>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > > >>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> > > >>>>>>>>                              encode_stateid_maxsz + 3)
> > > >>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> > > >>>>>>>> #define encode_fs_locations_maxsz \
> > > >>>>>>>>                             (encode_getattr_maxsz)
> > > >>>>>>>> #define decode_fs_locations_maxsz \
> > > >>>>>>>> -                               (0)
> > > >>>>>>>> +                               (1)
> > > >>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > > >>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > > >>>>>>>>
> > > >>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > >>>>>>>>                             1 /* opaque devaddr4 length */ + \
> > > >>>>>>>>                               /* devaddr4 payload is read into page */ \
> > > >>>>>>>>                             1 /* notification bitmap length */ + \
> > > >>>>>>>> -                               1 /* notification bitmap, word 0 */)
> > > >>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
> > > >>>>>>>> +                               1 /* possible XDR padding */)
> > > >>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> > > >>>>>>>>                             encode_stateid_maxsz)
> > > >>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> > > >>>>>>>>                             decode_stateid_maxsz + \
> > > >>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> > > >>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> > > >>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> > > >>>>>>>>                             2 /* offset */ + \
> > > >>>>>>>>                             2 /* length */ + \
> > > >>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > >>>>>>>> index f780605..4ea38b0 100644
> > > >>>>>>>> --- a/net/sunrpc/clnt.c
> > > >>>>>>>> +++ b/net/sunrpc/clnt.c
> > > >>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> > > >>>>>>>>                          unsigned int base, unsigned int len,
> > > >>>>>>>>                          unsigned int hdrsize)
> > > >>>>>>>> {
> > > >>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> > > >>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
> > > >>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> > > >>>>>>>> +        */
> > > >>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> > > >>>>>>>> +
> > > >>>>>>>>     xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> > > >>>>>>>>     trace_rpc_reply_pages(req);
> > > >>>>>>>> }
> > > >>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > > >>>>>>>> index 7cca515..aa8177d 100644
> > > >>>>>>>> --- a/net/sunrpc/xdr.c
> > > >>>>>>>> +++ b/net/sunrpc/xdr.c
> > > >>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> > > >>>>>>>>
> > > >>>>>>>>     tail->iov_base = buf + offset;
> > > >>>>>>>>     tail->iov_len = buflen - offset;
> > > >>>>>>>> +       if ((xdr->page_len & 3) == 0)
> > > >>>>>>>> +               tail->iov_len -= sizeof(__be32);
> > > >>>>>>>>
> > > >>>>>>>>     xdr->buflen += len;
> > > >>>>>>>> }
> > > >>>>>>>>
> > > >>>>>>
> > > >>>>>> --
> > > >>>>>> Chuck Lever
> > > >>>>
> > > >>>> --
> > > >>>> Chuck Lever
> > > >>
> > > >> --
> > > >> Chuck Lever
> > >
> > > --
> > > Chuck Lever
> > >
> > >
> > >

Olga Kornievskaia April 8, 2019, 4:02 p.m. UTC | #12

On Mon, Apr 8, 2019 at 11:50 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Mon, Apr 8, 2019 at 11:26 AM Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Mon, Apr 8, 2019 at 11:21 AM Olga Kornievskaia <aglo@umich.edu> wrote:
> > >
> > > On Mon, Apr 8, 2019 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > >
> > > >
> > > >
> > > > > On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > >
> > > > > On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > >>>
> > > > >>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > >>>>>
> > > > >>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > >>>>>>>
> > > > >>>>>>> Hi Chuck,
> > > > >>>>>>>
> > > > >>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
> > > > >>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
> > > > >>>>>>
> > > > >>>>>> Possibly the macro that defines the maximum size of the reply
> > > > >>>>>> is incorrect.
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
> > > > >>>>> that it probably breaks whatever else it modified.
> > > > >>>>
> > > > >>>> It modifies READ, READDIR, and READLINK. Are those broken?
> > > > >>>
> > > > >>> I don't know how to test READLINK.. but I think READ/READDIR work OK
> > > > >>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
> > > > >>> testing doesn't happen frequently).
> > > > >>
> > > > >> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
> > > > >> tests in my automated unit tests.
> > > > >>
> > > > >>
> > > > >>>>> The question is: can't we just revert it??
> > > > >>>>
> > > > >>>> Why not "root cause" it first?
> > > > >>>
> > > > >>> I'm trying :-/ I was just fishing to see how important the change was.
> > > > >>
> > > > >> Try reverting just this hunk:
> > > > >
> > > > > That doesn't help. It seems to be this piece that's causing issues
> > > > > hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
> > > > >
> > > > > With this there is an extra byte (in front) in the buffer when (ACL)
> > > > > operation is decoded.
> > > >
> > > > How do you know there isn't a latent bug in the getfacl decoder?
> > >
> > > I don't. All I know is that it passed tests before and now it doesn't.
> >
> > Also this bug will have to be in both getfacl and fs_location code.
> > What they both share is xd_enter_page() code that now with new
> > semantics makes the buffer point to the wrong place.

READLINK uses xdr_read_pages() function which sets the xdr->p and
accounts for padding which xdr_enter_page() does not.

> >
> > > > How are you reproducing this issue? I can try it here later today.
> > >
> > > The issue was found running xfstest nfs/001. However, you don't need
> > > that: (1) mount (2) nfs4_getfacl <file>
> > >
> > > To understand a patch, does it fix a problem with READLINK or is the
> > > an optimization?
>
> So this "fixes" it but this don't look really good.
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index cfcabc3..f2a5553 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
>
> @@ -5280,6 +5280,7 @@ static int decode_getacl(struct xdr_stream *xdr,
> struct rpc_rqst *req,
>   goto out;
>   xdr_enter_page(xdr, xdr->buf->page_len);
> + xdr->p++;
>
>   /* Calculate the offset of the page data */
>   pg_offset = xdr->buf->head[0].iov_len;
> @@ -6949,6 +6950,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
>   goto out;
>   if (res->migration) {
>   xdr_enter_page(xdr, PAGE_SIZE);
> + xdr->p++;
>   status = decode_getfattr_generic(xdr,
>   &res->fs_locations->fattr,
>   NULL, res->fs_locations,
> @@ -6962,6 +6964,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
>   if (status)
>   goto out;
>   xdr_enter_page(xdr, PAGE_SIZE);
> + xdr->p++;
>   status = decode_getfattr_generic(xdr,
>   &res->fs_locations->fattr,
>   NULL, res->fs_locations,
>
>
> > >
> > > >
> > > >
> > > > >> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > > >> index d0fa18d..6d9d5e2 100644
> > > > >> --- a/fs/nfs/nfs4xdr.c
> > > > >> +++ b/fs/nfs/nfs4xdr.c
> > > > >> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > > >> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > > > >> #define encode_getacl_maxsz     (encode_getattr_maxsz)
> > > > >> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
> > > > >> -                                nfs4_fattr_bitmap_maxsz + 1)
> > > > >> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > > > >> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
> > > > >>                                 encode_stateid_maxsz + 3)
> > > > >> #define decode_setacl_maxsz     (decode_setattr_maxsz)
> > > > >> #define encode_fs_locations_maxsz \
> > > > >>                                (encode_getattr_maxsz)
> > > > >> #define decode_fs_locations_maxsz \
> > > > >> -                               (0)
> > > > >> +                               (1)
> > > > >> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > > > >> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > > > >>
> > > > >>
> > > > >>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
> > > > >>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
> > > > >>>>>>>> is required to pad that payload so its length on the wire is a
> > > > >>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
> > > > >>>>>>>> each NFS result do not appear to account for this extra word.
> > > > >>>>>>>>
> > > > >>>>>>>> In each case where the data payload is to be received into pages:
> > > > >>>>>>>>
> > > > >>>>>>>> - 1 word is added to the size of the receive buffer allocated by
> > > > >>>>>>>> call_allocate
> > > > >>>>>>>>
> > > > >>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
> > > > >>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
> > > > >>>>>>>>
> > > > >>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
> > > > >>>>>>>> is thus removed from the tail
> > > > >>>>>>>>
> > > > >>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > >>>>>>>> ---
> > > > >>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
> > > > >>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
> > > > >>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
> > > > >>>>>>>> net/sunrpc/clnt.c |    6 +++++-
> > > > >>>>>>>> net/sunrpc/xdr.c  |    2 ++
> > > > >>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
> > > > >>>>>>>>
> > > > >>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> > > > >>>>>>>> index 1dcd0fe..a7ed29d 100644
> > > > >>>>>>>> --- a/fs/nfs/nfs2xdr.c
> > > > >>>>>>>> +++ b/fs/nfs/nfs2xdr.c
> > > > >>>>>>>> @@ -56,11 +56,11 @@
> > > > >>>>>>>>
> > > > >>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
> > > > >>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
> > > > >>>>>>>> -#define NFS_readlinkres_sz     (2)
> > > > >>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
> > > > >>>>>>>> +#define NFS_readlinkres_sz     (2+1)
> > > > >>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
> > > > >>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
> > > > >>>>>>>> #define NFS_stat_sz            (1)
> > > > >>>>>>>> -#define NFS_readdirres_sz      (1)
> > > > >>>>>>>> +#define NFS_readdirres_sz      (1+1)
> > > > >>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
> > > > >>>>>>>>
> > > > >>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
> > > > >>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> > > > >>>>>>>> index a54dcf4..110358f 100644
> > > > >>>>>>>> --- a/fs/nfs/nfs3xdr.c
> > > > >>>>>>>> +++ b/fs/nfs/nfs3xdr.c
> > > > >>>>>>>> @@ -69,13 +69,13 @@
> > > > >>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
> > > > >>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
> > > > >>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
> > > > >>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
> > > > >>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
> > > > >>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
> > > > >>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
> > > > >>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
> > > > >>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > > > >>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
> > > > >>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
> > > > >>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
> > > > >>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
> > > > >>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
> > > > >>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
> > > > >>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
> > > > >>>>>>>> @@ -85,7 +85,7 @@
> > > > >>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
> > > > >>>>>>>>                             XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > > > >>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
> > > > >>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
> > > > >>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
> > > > >>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
> > > > >>>>>>>>
> > > > >>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
> > > > >>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
> > > > >>>>>>>>     result->op_status = status;
> > > > >>>>>>>>     if (status != NFS3_OK)
> > > > >>>>>>>>             goto out_status;
> > > > >>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > > > >>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
> > > > >>>>>>>>     error = decode_read3resok(xdr, result);
> > > > >>>>>>>> out:
> > > > >>>>>>>>     return error;
> > > > >>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > > >>>>>>>> index d0fa18d..6d9d5e2 100644
> > > > >>>>>>>> --- a/fs/nfs/nfs4xdr.c
> > > > >>>>>>>> +++ b/fs/nfs/nfs4xdr.c
> > > > >>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > > >>>>>>>>                              nfs4_fattr_bitmap_maxsz)
> > > > >>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
> > > > >>>>>>>>                              encode_stateid_maxsz + 3)
> > > > >>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
> > > > >>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
> > > > >>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
> > > > >>>>>>>>                              2 + encode_verifier_maxsz + 5 + \
> > > > >>>>>>>>                             nfs4_label_maxsz)
> > > > >>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
> > > > >>>>>>>> -                                decode_verifier_maxsz)
> > > > >>>>>>>> +                                decode_verifier_maxsz + 1)
> > > > >>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
> > > > >>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
> > > > >>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
> > > > >>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
> > > > >>>>>>>>                              encode_stateid_maxsz + 4)
> > > > >>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
> > > > >>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > > >>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
> > > > >>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
> > > > >>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
> > > > >>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
> > > > >>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
> > > > >>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
> > > > >>>>>>>>                              encode_stateid_maxsz + 3)
> > > > >>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
> > > > >>>>>>>> #define encode_fs_locations_maxsz \
> > > > >>>>>>>>                             (encode_getattr_maxsz)
> > > > >>>>>>>> #define decode_fs_locations_maxsz \
> > > > >>>>>>>> -                               (0)
> > > > >>>>>>>> +                               (1)
> > > > >>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
> > > > >>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
> > > > >>>>>>>>
> > > > >>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> > > > >>>>>>>>                             1 /* opaque devaddr4 length */ + \
> > > > >>>>>>>>                               /* devaddr4 payload is read into page */ \
> > > > >>>>>>>>                             1 /* notification bitmap length */ + \
> > > > >>>>>>>> -                               1 /* notification bitmap, word 0 */)
> > > > >>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
> > > > >>>>>>>> +                               1 /* possible XDR padding */)
> > > > >>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
> > > > >>>>>>>>                             encode_stateid_maxsz)
> > > > >>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
> > > > >>>>>>>>                             decode_stateid_maxsz + \
> > > > >>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
> > > > >>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
> > > > >>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
> > > > >>>>>>>>                             2 /* offset */ + \
> > > > >>>>>>>>                             2 /* length */ + \
> > > > >>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > > >>>>>>>> index f780605..4ea38b0 100644
> > > > >>>>>>>> --- a/net/sunrpc/clnt.c
> > > > >>>>>>>> +++ b/net/sunrpc/clnt.c
> > > > >>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
> > > > >>>>>>>>                          unsigned int base, unsigned int len,
> > > > >>>>>>>>                          unsigned int hdrsize)
> > > > >>>>>>>> {
> > > > >>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
> > > > >>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
> > > > >>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
> > > > >>>>>>>> +        */
> > > > >>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
> > > > >>>>>>>> +
> > > > >>>>>>>>     xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
> > > > >>>>>>>>     trace_rpc_reply_pages(req);
> > > > >>>>>>>> }
> > > > >>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > > > >>>>>>>> index 7cca515..aa8177d 100644
> > > > >>>>>>>> --- a/net/sunrpc/xdr.c
> > > > >>>>>>>> +++ b/net/sunrpc/xdr.c
> > > > >>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
> > > > >>>>>>>>
> > > > >>>>>>>>     tail->iov_base = buf + offset;
> > > > >>>>>>>>     tail->iov_len = buflen - offset;
> > > > >>>>>>>> +       if ((xdr->page_len & 3) == 0)
> > > > >>>>>>>> +               tail->iov_len -= sizeof(__be32);
> > > > >>>>>>>>
> > > > >>>>>>>>     xdr->buflen += len;
> > > > >>>>>>>> }
> > > > >>>>>>>>
> > > > >>>>>>
> > > > >>>>>> --
> > > > >>>>>> Chuck Lever
> > > > >>>>
> > > > >>>> --
> > > > >>>> Chuck Lever
> > > > >>
> > > > >> --
> > > > >> Chuck Lever
> > > >
> > > > --
> > > > Chuck Lever
> > > >
> > > >
> > > >

Chuck Lever April 8, 2019, 4:29 p.m. UTC | #13

> On Apr 8, 2019, at 11:50 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Mon, Apr 8, 2019 at 11:26 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>> 
>> On Mon, Apr 8, 2019 at 11:21 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>>> 
>>> On Mon, Apr 8, 2019 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Apr 8, 2019, at 10:36 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>> 
>>>>> On Fri, Apr 5, 2019 at 3:42 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On Apr 5, 2019, at 3:27 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>> 
>>>>>>> On Fri, Apr 5, 2019 at 3:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> On Apr 5, 2019, at 3:17 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>>>> 
>>>>>>>>> On Fri, Apr 5, 2019 at 1:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> On Apr 5, 2019, at 1:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>>>>>> 
>>>>>>>>>>> Hi Chuck,
>>>>>>>>>>> 
>>>>>>>>>>> This patch break ACLs. After applying this patch nfs4_getfacl fails
>>>>>>>>>>> (it fails within xdr and returns ENOTSUPP). Any ideas why?
>>>>>>>>>> 
>>>>>>>>>> Possibly the macro that defines the maximum size of the reply
>>>>>>>>>> is incorrect.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> This also breaks FS_LOCATION. I'm going to go on the limb here and say
>>>>>>>>> that it probably breaks whatever else it modified.
>>>>>>>> 
>>>>>>>> It modifies READ, READDIR, and READLINK. Are those broken?
>>>>>>> 
>>>>>>> I don't know how to test READLINK.. but I think READ/READDIR work OK
>>>>>>> otherwise folks would have noticed it (I gather ACL and FS_LOCATION
>>>>>>> testing doesn't happen frequently).
>>>>>> 
>>>>>> I guess I don't have any NFSv4 ACL or FS_LOCATIONS regressions
>>>>>> tests in my automated unit tests.
>>>>>> 
>>>>>> 
>>>>>>>>> The question is: can't we just revert it??
>>>>>>>> 
>>>>>>>> Why not "root cause" it first?
>>>>>>> 
>>>>>>> I'm trying :-/ I was just fishing to see how important the change was.
>>>>>> 
>>>>>> Try reverting just this hunk:
>>>>> 
>>>>> That doesn't help. It seems to be this piece that's causing issues
>>>>> hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1
>>>>> 
>>>>> With this there is an extra byte (in front) in the buffer when (ACL)
>>>>> operation is decoded.
>>>> 
>>>> How do you know there isn't a latent bug in the getfacl decoder?
>>> 
>>> I don't. All I know is that it passed tests before and now it doesn't.
>> 
>> Also this bug will have to be in both getfacl and fs_location code.
>> What they both share is xd_enter_page() code that now with new
>> semantics makes the buffer point to the wrong place.
>> 
>>>> How are you reproducing this issue? I can try it here later today.
>>> 
>>> The issue was found running xfstest nfs/001. However, you don't need
>>> that: (1) mount (2) nfs4_getfacl <file>
>>> 
>>> To understand a patch, does it fix a problem with READLINK or is the
>>> an optimization?
> 
> So this "fixes" it but this don't look really good.
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index cfcabc3..f2a5553 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> 
> @@ -5280,6 +5280,7 @@ static int decode_getacl(struct xdr_stream *xdr,
> struct rpc_rqst *req,
>  goto out;
>  xdr_enter_page(xdr, xdr->buf->page_len);
> + xdr->p++;
> 
>  /* Calculate the offset of the page data */
>  pg_offset = xdr->buf->head[0].iov_len;
> @@ -6949,6 +6950,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
>  goto out;
>  if (res->migration) {
>  xdr_enter_page(xdr, PAGE_SIZE);
> + xdr->p++;
>  status = decode_getfattr_generic(xdr,
>  &res->fs_locations->fattr,
>  NULL, res->fs_locations,
> @@ -6962,6 +6964,7 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
>  if (status)
>  goto out;
>  xdr_enter_page(xdr, PAGE_SIZE);
> + xdr->p++;
>  status = decode_getfattr_generic(xdr,
>  &res->fs_locations->fattr,
>  NULL, res->fs_locations,

Your workaround appears to advance xdr->p one quad into the first page in
the xdr_buf. We really want that XDR data item to fall right at offset 0
of that page.

I'm looking into it.


>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>>>> index d0fa18d..6d9d5e2 100644
>>>>>> --- a/fs/nfs/nfs4xdr.c
>>>>>> +++ b/fs/nfs/nfs4xdr.c
>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>>>>>> #define encode_getacl_maxsz     (encode_getattr_maxsz)
>>>>>> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>>>>>> #define encode_setacl_maxsz     (op_encode_hdr_maxsz + \
>>>>>>                                encode_stateid_maxsz + 3)
>>>>>> #define decode_setacl_maxsz     (decode_setattr_maxsz)
>>>>>> #define encode_fs_locations_maxsz \
>>>>>>                               (encode_getattr_maxsz)
>>>>>> #define decode_fs_locations_maxsz \
>>>>>> -                               (0)
>>>>>> +                               (1)
>>>>>> #define encode_secinfo_maxsz    (op_encode_hdr_maxsz + nfs4_name_maxsz)
>>>>>> #define decode_secinfo_maxsz    (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>>>>>> 
>>>>>> 
>>>>>>>>>>> On Mon, Feb 11, 2019 at 11:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> Certain NFS results (eg. READLINK) might expect a data payload that
>>>>>>>>>>>> is not an exact multiple of 4 bytes. In this case, XDR encoding
>>>>>>>>>>>> is required to pad that payload so its length on the wire is a
>>>>>>>>>>>> multiple of 4 bytes. The constants that define the maximum size of
>>>>>>>>>>>> each NFS result do not appear to account for this extra word.
>>>>>>>>>>>> 
>>>>>>>>>>>> In each case where the data payload is to be received into pages:
>>>>>>>>>>>> 
>>>>>>>>>>>> - 1 word is added to the size of the receive buffer allocated by
>>>>>>>>>>>> call_allocate
>>>>>>>>>>>> 
>>>>>>>>>>>> - rpc_inline_rcv_pages subtracts 1 word from @hdrsize so that the
>>>>>>>>>>>> extra buffer space falls into the rcv_buf's tail iovec
>>>>>>>>>>>> 
>>>>>>>>>>>> - If buf->pagelen is word-aligned, an XDR pad is not needed and
>>>>>>>>>>>> is thus removed from the tail
>>>>>>>>>>>> 
>>>>>>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> fs/nfs/nfs2xdr.c  |    6 +++---
>>>>>>>>>>>> fs/nfs/nfs3xdr.c  |   10 +++++-----
>>>>>>>>>>>> fs/nfs/nfs4xdr.c  |   15 ++++++++-------
>>>>>>>>>>>> net/sunrpc/clnt.c |    6 +++++-
>>>>>>>>>>>> net/sunrpc/xdr.c  |    2 ++
>>>>>>>>>>>> 5 files changed, 23 insertions(+), 16 deletions(-)
>>>>>>>>>>>> 
>>>>>>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
>>>>>>>>>>>> index 1dcd0fe..a7ed29d 100644
>>>>>>>>>>>> --- a/fs/nfs/nfs2xdr.c
>>>>>>>>>>>> +++ b/fs/nfs/nfs2xdr.c
>>>>>>>>>>>> @@ -56,11 +56,11 @@
>>>>>>>>>>>> 
>>>>>>>>>>>> #define NFS_attrstat_sz                (1+NFS_fattr_sz)
>>>>>>>>>>>> #define NFS_diropres_sz                (1+NFS_fhandle_sz+NFS_fattr_sz)
>>>>>>>>>>>> -#define NFS_readlinkres_sz     (2)
>>>>>>>>>>>> -#define NFS_readres_sz         (1+NFS_fattr_sz+1)
>>>>>>>>>>>> +#define NFS_readlinkres_sz     (2+1)
>>>>>>>>>>>> +#define NFS_readres_sz         (1+NFS_fattr_sz+1+1)
>>>>>>>>>>>> #define NFS_writeres_sz         (NFS_attrstat_sz)
>>>>>>>>>>>> #define NFS_stat_sz            (1)
>>>>>>>>>>>> -#define NFS_readdirres_sz      (1)
>>>>>>>>>>>> +#define NFS_readdirres_sz      (1+1)
>>>>>>>>>>>> #define NFS_statfsres_sz       (1+NFS_info_sz)
>>>>>>>>>>>> 
>>>>>>>>>>>> static int nfs_stat_to_errno(enum nfs_stat);
>>>>>>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
>>>>>>>>>>>> index a54dcf4..110358f 100644
>>>>>>>>>>>> --- a/fs/nfs/nfs3xdr.c
>>>>>>>>>>>> +++ b/fs/nfs/nfs3xdr.c
>>>>>>>>>>>> @@ -69,13 +69,13 @@
>>>>>>>>>>>> #define NFS3_removeres_sz      (NFS3_setattrres_sz)
>>>>>>>>>>>> #define NFS3_lookupres_sz      (1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
>>>>>>>>>>>> #define NFS3_accessres_sz      (1+NFS3_post_op_attr_sz+1)
>>>>>>>>>>>> -#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1)
>>>>>>>>>>>> -#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3)
>>>>>>>>>>>> +#define NFS3_readlinkres_sz    (1+NFS3_post_op_attr_sz+1+1)
>>>>>>>>>>>> +#define NFS3_readres_sz                (1+NFS3_post_op_attr_sz+3+1)
>>>>>>>>>>>> #define NFS3_writeres_sz       (1+NFS3_wcc_data_sz+4)
>>>>>>>>>>>> #define NFS3_createres_sz      (1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>>>>>>>> #define NFS3_renameres_sz      (1+(2 * NFS3_wcc_data_sz))
>>>>>>>>>>>> #define NFS3_linkres_sz                (1+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
>>>>>>>>>>>> -#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2)
>>>>>>>>>>>> +#define NFS3_readdirres_sz     (1+NFS3_post_op_attr_sz+2+1)
>>>>>>>>>>>> #define NFS3_fsstatres_sz      (1+NFS3_post_op_attr_sz+13)
>>>>>>>>>>>> #define NFS3_fsinfores_sz      (1+NFS3_post_op_attr_sz+12)
>>>>>>>>>>>> #define NFS3_pathconfres_sz    (1+NFS3_post_op_attr_sz+6)
>>>>>>>>>>>> @@ -85,7 +85,7 @@
>>>>>>>>>>>> #define ACL3_setaclargs_sz     (NFS3_fh_sz+1+ \
>>>>>>>>>>>>                            XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>>>>>>>> #define ACL3_getaclres_sz      (1+NFS3_post_op_attr_sz+1+ \
>>>>>>>>>>>> -                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE))
>>>>>>>>>>>> +                               XDR_QUADLEN(NFS_ACL_INLINE_BUFSIZE)+1)
>>>>>>>>>>>> #define ACL3_setaclres_sz      (1+NFS3_post_op_attr_sz)
>>>>>>>>>>>> 
>>>>>>>>>>>> static int nfs3_stat_to_errno(enum nfs_stat);
>>>>>>>>>>>> @@ -1629,7 +1629,7 @@ static int nfs3_xdr_dec_read3res(struct rpc_rqst *req, struct xdr_stream *xdr,
>>>>>>>>>>>>    result->op_status = status;
>>>>>>>>>>>>    if (status != NFS3_OK)
>>>>>>>>>>>>            goto out_status;
>>>>>>>>>>>> -       result->replen = 3 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>>>>>>>> +       result->replen = 4 + ((xdr_stream_pos(xdr) - pos) >> 2);
>>>>>>>>>>>>    error = decode_read3resok(xdr, result);
>>>>>>>>>>>> out:
>>>>>>>>>>>>    return error;
>>>>>>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>>>>>>>>>> index d0fa18d..6d9d5e2 100644
>>>>>>>>>>>> --- a/fs/nfs/nfs4xdr.c
>>>>>>>>>>>> +++ b/fs/nfs/nfs4xdr.c
>>>>>>>>>>>> @@ -215,14 +215,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>>>>>>                             nfs4_fattr_bitmap_maxsz)
>>>>>>>>>>>> #define encode_read_maxsz      (op_encode_hdr_maxsz + \
>>>>>>>>>>>>                             encode_stateid_maxsz + 3)
>>>>>>>>>>>> -#define decode_read_maxsz      (op_decode_hdr_maxsz + 2)
>>>>>>>>>>>> +#define decode_read_maxsz      (op_decode_hdr_maxsz + 2 + 1)
>>>>>>>>>>>> #define encode_readdir_maxsz   (op_encode_hdr_maxsz + \
>>>>>>>>>>>>                             2 + encode_verifier_maxsz + 5 + \
>>>>>>>>>>>>                            nfs4_label_maxsz)
>>>>>>>>>>>> #define decode_readdir_maxsz   (op_decode_hdr_maxsz + \
>>>>>>>>>>>> -                                decode_verifier_maxsz)
>>>>>>>>>>>> +                                decode_verifier_maxsz + 1)
>>>>>>>>>>>> #define encode_readlink_maxsz  (op_encode_hdr_maxsz)
>>>>>>>>>>>> -#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1)
>>>>>>>>>>>> +#define decode_readlink_maxsz  (op_decode_hdr_maxsz + 1 + 1)
>>>>>>>>>>>> #define encode_write_maxsz     (op_encode_hdr_maxsz + \
>>>>>>>>>>>>                             encode_stateid_maxsz + 4)
>>>>>>>>>>>> #define decode_write_maxsz     (op_decode_hdr_maxsz + \
>>>>>>>>>>>> @@ -284,14 +284,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>>>>>> #define decode_delegreturn_maxsz (op_decode_hdr_maxsz)
>>>>>>>>>>>> #define encode_getacl_maxsz    (encode_getattr_maxsz)
>>>>>>>>>>>> #define decode_getacl_maxsz    (op_decode_hdr_maxsz + \
>>>>>>>>>>>> -                                nfs4_fattr_bitmap_maxsz + 1)
>>>>>>>>>>>> +                                nfs4_fattr_bitmap_maxsz + 1 + 1)
>>>>>>>>>>>> #define encode_setacl_maxsz    (op_encode_hdr_maxsz + \
>>>>>>>>>>>>                             encode_stateid_maxsz + 3)
>>>>>>>>>>>> #define decode_setacl_maxsz    (decode_setattr_maxsz)
>>>>>>>>>>>> #define encode_fs_locations_maxsz \
>>>>>>>>>>>>                            (encode_getattr_maxsz)
>>>>>>>>>>>> #define decode_fs_locations_maxsz \
>>>>>>>>>>>> -                               (0)
>>>>>>>>>>>> +                               (1)
>>>>>>>>>>>> #define encode_secinfo_maxsz   (op_encode_hdr_maxsz + nfs4_name_maxsz)
>>>>>>>>>>>> #define decode_secinfo_maxsz   (op_decode_hdr_maxsz + 1 + ((NFS_MAX_SECFLAVORS * (16 + GSS_OID_MAX_LEN)) / 4))
>>>>>>>>>>>> 
>>>>>>>>>>>> @@ -392,12 +392,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
>>>>>>>>>>>>                            1 /* opaque devaddr4 length */ + \
>>>>>>>>>>>>                              /* devaddr4 payload is read into page */ \
>>>>>>>>>>>>                            1 /* notification bitmap length */ + \
>>>>>>>>>>>> -                               1 /* notification bitmap, word 0 */)
>>>>>>>>>>>> +                               1 /* notification bitmap, word 0 */ + \
>>>>>>>>>>>> +                               1 /* possible XDR padding */)
>>>>>>>>>>>> #define encode_layoutget_maxsz (op_encode_hdr_maxsz + 10 + \
>>>>>>>>>>>>                            encode_stateid_maxsz)
>>>>>>>>>>>> #define decode_layoutget_maxsz (op_decode_hdr_maxsz + 8 + \
>>>>>>>>>>>>                            decode_stateid_maxsz + \
>>>>>>>>>>>> -                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE))
>>>>>>>>>>>> +                               XDR_QUADLEN(PNFS_LAYOUT_MAXSIZE) + 1)
>>>>>>>>>>>> #define encode_layoutcommit_maxsz (op_encode_hdr_maxsz +          \
>>>>>>>>>>>>                            2 /* offset */ + \
>>>>>>>>>>>>                            2 /* length */ + \
>>>>>>>>>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>>>>>>>>>> index f780605..4ea38b0 100644
>>>>>>>>>>>> --- a/net/sunrpc/clnt.c
>>>>>>>>>>>> +++ b/net/sunrpc/clnt.c
>>>>>>>>>>>> @@ -1177,7 +1177,11 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>>>>>>>>>>>>                         unsigned int base, unsigned int len,
>>>>>>>>>>>>                         unsigned int hdrsize)
>>>>>>>>>>>> {
>>>>>>>>>>>> -       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack;
>>>>>>>>>>>> +       /* Subtract one to force an extra word of buffer space for the
>>>>>>>>>>>> +        * payload's XDR pad to fall into the rcv_buf's tail iovec.
>>>>>>>>>>>> +        */
>>>>>>>>>>>> +       hdrsize += RPC_REPHDRSIZE + req->rq_cred->cr_auth->au_rslack - 1;
>>>>>>>>>>>> +
>>>>>>>>>>>>    xdr_inline_pages(&req->rq_rcv_buf, hdrsize << 2, pages, base, len);
>>>>>>>>>>>>    trace_rpc_reply_pages(req);
>>>>>>>>>>>> }
>>>>>>>>>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>>>>>>>>>>> index 7cca515..aa8177d 100644
>>>>>>>>>>>> --- a/net/sunrpc/xdr.c
>>>>>>>>>>>> +++ b/net/sunrpc/xdr.c
>>>>>>>>>>>> @@ -189,6 +189,8 @@ __be32 *xdr_encode_opaque(__be32 *p, const void *ptr, unsigned int nbytes)
>>>>>>>>>>>> 
>>>>>>>>>>>>    tail->iov_base = buf + offset;
>>>>>>>>>>>>    tail->iov_len = buflen - offset;
>>>>>>>>>>>> +       if ((xdr->page_len & 3) == 0)
>>>>>>>>>>>> +               tail->iov_len -= sizeof(__be32);
>>>>>>>>>>>> 
>>>>>>>>>>>>    xdr->buflen += len;
>>>>>>>>>>>> }
>>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> --
>>>>>>>>>> Chuck Lever
>>>>>>>> 
>>>>>>>> --
>>>>>>>> Chuck Lever
>>>>>> 
>>>>>> --
>>>>>> Chuck Lever
>>>> 
>>>> --
>>>> Chuck Lever

--
Chuck Lever

[v1,20/23] NFS: Account for XDR pad of buf->pages

Commit Message

Comments

Patch