On Thu, 2023-11-09 at 08:45 -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> nfsd_cache_csum() currently assumes that the server's RPC layer has
> been advancing rq_arg.head[0].iov_base as it decodes an incoming
> request, because that's the way it used to work. On entry, it
> expects that buf->head[0].iov_base points to the start of the NFS
> header, and excludes the already-decoded RPC header.
>
> Ever since the RPC layer was converted over to use xdr_stream (in
> v6.3), however, head[0].iov_base now points to the start of the RPC
> header during all processing. It no longer points at the NFS Call
> header when execution arrives at nfsd_cache_csum().
>
> In a retransmitted RPC the XID and the NFS header are supposed to
> be the same as the original message, but the contents of the
> retransmitted RPC header can be different. For example, for krb5,
> the GSS sequence number will be different between the two. Thus if
> the RPC header is included in the DRC checksum computation, the
> checksum of the retransmitted message might not match the checksum
> of the original message, even though the NFS part of these messages
> is identical.
>
> If the DRC fails to recognize a retransmit, it permits the server to
> execute that RPC Call again. That breaks retransmits of idempotent
> procedures such as RENAME or REMOVE -- the retransmitted RPC Call
> will get a different result.
>
> Note that I am not marking this commit with a Fixes: tag:
>
> o The RPC-related commits that broke the DRC are too numerous
> o The fix should not be automatically backported, as any backport
> of it needs to be thoroughly tested
>
> However, it might be appropriate to consider applying this fix to
> v6.3 and later kernels, if someone can make it fit cleanly and test
> it properly. This patch applies with fuzz to v6.6, but does not
> apply cleanly to earlier kernels.
>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/cache.h | 4 ++--
> fs/nfsd/nfscache.c | 55 +++++++++++++++++++++++++++++++++-------------------
> fs/nfsd/nfssvc.c | 10 +++++++++
> 3 files changed, 46 insertions(+), 23 deletions(-)
>
> This fix is kind of ugly. Looking for comments or suggestions for
> improvement. Two further clean-ups occurred to me:
>
> - Set up a parms struct in nfsd_dispatch() that is passed to
> nfsd_cache_lookup() and nfsd_cache_update() that carries all
> of this extraneous garbage
> - Move nfsd_cache_csum to net/sunrpc/xdr.c since it assumes
> details about the how the message appears in the xdr_buf
>
>
> diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h
> index 929248c6ca84..4cbe0434cbb8 100644
> --- a/fs/nfsd/cache.h
> +++ b/fs/nfsd/cache.h
> @@ -84,8 +84,8 @@ int nfsd_net_reply_cache_init(struct nfsd_net *nn);
> void nfsd_net_reply_cache_destroy(struct nfsd_net *nn);
> int nfsd_reply_cache_init(struct nfsd_net *);
> void nfsd_reply_cache_shutdown(struct nfsd_net *);
> -int nfsd_cache_lookup(struct svc_rqst *rqstp,
> - struct nfsd_cacherep **cacherep);
> +int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
> + unsigned int len, struct nfsd_cacherep **cacherep);
> void nfsd_cache_update(struct svc_rqst *rqstp, struct nfsd_cacherep *rp,
> int cachetype, __be32 *statp);
> int nfsd_reply_cache_stats_show(struct seq_file *m, void *v);
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index fd56a52aa5fb..761896c44e77 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -370,32 +370,44 @@ nfsd_reply_cache_scan(struct shrinker *shrink, struct shrink_control *sc)
> }
>
> /*
> - * Walk an xdr_buf and get a CRC for at most the first RC_CSUMLEN bytes
> + * Compute a weak checksum of the leading bytes of an NFS procedure
> + * call header. csum_partial() computes a TCP checksum as specified
> + * in RFC 793.
> + *
> + * To avoid assumptions about how the RPC message is laid out in
> + * @buf and what else it might contain (eg, a GSS MIC suffix), the
> + * caller passes the exact location and length of the NFS Call
> + * header.
> */
Maybe make this a kerneldoc comment?
> -static __wsum
> -nfsd_cache_csum(struct svc_rqst *rqstp)
> +static __wsum nfsd_cache_csum(struct xdr_buf *buf, unsigned int start,
> + unsigned int remaining)
> {
> + unsigned int base, len;
> + struct xdr_buf subbuf;
> + __wsum csum = 0;
> + void *p;
> int idx;
> - unsigned int base;
> - __wsum csum;
> - struct xdr_buf *buf = &rqstp->rq_arg;
> - const unsigned char *p = buf->head[0].iov_base;
> - size_t csum_len = min_t(size_t, buf->head[0].iov_len + buf->page_len,
> - RC_CSUMLEN);
> - size_t len = min(buf->head[0].iov_len, csum_len);
> +
> + if (remaining > RC_CSUMLEN)
> + remaining = RC_CSUMLEN;
> + if (xdr_buf_subsegment(buf, &subbuf, start, remaining))
> + return csum;
>
> /* rq_arg.head first */
> - csum = csum_partial(p, len, 0);
> - csum_len -= len;
> + if (subbuf.head[0].iov_len) {
> + len = min_t(unsigned int, subbuf.head[0].iov_len, remaining);
> + csum = csum_partial(subbuf.head[0].iov_base, len, csum);
> + remaining -= len;
> + }
>
> /* Continue into page array */
> - idx = buf->page_base / PAGE_SIZE;
> - base = buf->page_base & ~PAGE_MASK;
> - while (csum_len) {
> - p = page_address(buf->pages[idx]) + base;
> - len = min_t(size_t, PAGE_SIZE - base, csum_len);
> + idx = subbuf.page_base / PAGE_SIZE;
> + base = subbuf.page_base & ~PAGE_MASK;
> + while (remaining) {
> + p = page_address(subbuf.pages[idx]) + base;
> + len = min_t(unsigned int, PAGE_SIZE - base, remaining);
> csum = csum_partial(p, len, csum);
> - csum_len -= len;
> + remaining -= len;
> base = 0;
> ++idx;
> }
> @@ -466,6 +478,8 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct nfsd_cacherep *key,
> /**
> * nfsd_cache_lookup - Find an entry in the duplicate reply cache
> * @rqstp: Incoming Call to find
> + * @start: location in @rqstp->rq_arg of the NFS Call header
> + * @len: length of NFS Call header in bytes
> * @cacherep: OUT: DRC entry for this request
> *
> * Try to find an entry matching the current call in the cache. When none
> @@ -479,7 +493,8 @@ nfsd_cache_insert(struct nfsd_drc_bucket *b, struct nfsd_cacherep *key,
> * %RC_REPLY: Reply from cache
> * %RC_DROPIT: Do not process the request further
> */
> -int nfsd_cache_lookup(struct svc_rqst *rqstp, struct nfsd_cacherep **cacherep)
> +int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
> + unsigned int len, struct nfsd_cacherep **cacherep)
> {
> struct nfsd_net *nn;
> struct nfsd_cacherep *rp, *found;
> @@ -495,7 +510,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, struct nfsd_cacherep **cacherep)
> goto out;
> }
>
> - csum = nfsd_cache_csum(rqstp);
> + csum = nfsd_cache_csum(&rqstp->rq_arg, start, len);
>
> /*
> * Since the common case is a cache miss followed by an insert,
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index ef126969a7ce..1d6e33c6c4fe 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -980,6 +980,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> const struct svc_procedure *proc = rqstp->rq_procinfo;
> __be32 *statp = rqstp->rq_accept_statp;
> struct nfsd_cacherep *rp;
> + unsigned int start, len;
>
> /*
> * Give the xdr decoder a chance to change this if it wants
> @@ -987,6 +988,13 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> */
> rqstp->rq_cachetype = proc->pc_cachetype;
>
> + /*
> + * ->pc_decode advances the argument stream past the NFS
> + * Call header, so grab the header's starting location and
> + * size now for the call to nfsd_cache_lookup().
> + */
> + start = xdr_stream_pos(&rqstp->rq_arg_stream);
> + len = xdr_stream_remaining(&rqstp->rq_arg_stream);
> if (!proc->pc_decode(rqstp, &rqstp->rq_arg_stream))
> goto out_decode_err;
>
> @@ -1000,7 +1008,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> smp_store_release(&rqstp->rq_status_counter, rqstp->rq_status_counter | 1);
>
> rp = NULL;
> - switch (nfsd_cache_lookup(rqstp, &rp)) {
> + switch (nfsd_cache_lookup(rqstp, start, len, &rp)) {
> case RC_DOIT:
> break;
> case RC_REPLY:
>
>
This looks pretty reasonable to me, even if it is a bit ugly. Since the
need is nfsd-specific, passing down these values is preferable to adding
them to struct svc_rqst.
--
Jeff Layton <jlayton@xxxxxxxxxx>
