by Kinglong Mee

[permalink] [raw]

Subject: Re: [PATCH 01/43] nfsd4: embed xdr_stream in nfsd4_compoundres

On 5/12/2014 04:52, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> This is a mechanical transformation with no change in behavior.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 12 +++++-----
> fs/nfsd/nfs4state.c | 8 +++----
> fs/nfsd/nfs4xdr.c | 65 ++++++++++++++++++++++++++-------------------------
> fs/nfsd/xdr4.h | 4 +---
> 4 files changed, 44 insertions(+), 45 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 2c1ee70..46370f5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1268,13 +1268,13 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
> u32 plen = 0;
> __be32 status;
>
> - resp->xbuf = &rqstp->rq_res;
> - resp->p = rqstp->rq_res.head[0].iov_base +
> + resp->xdr.buf = &rqstp->rq_res;
> + resp->xdr.p = rqstp->rq_res.head[0].iov_base +
> rqstp->rq_res.head[0].iov_len;

I'd like using a temp pointer as (struct xdr_stream *resp_xdr = &resp->xdr;).

thanks,
Kinglong Mee

> - resp->tagp = resp->p;
> + resp->tagp = resp->xdr.p;
> /* reserve space for: taglen, tag, and opcnt */
> - resp->p += 2 + XDR_QUADLEN(args->taglen);
> - resp->end = rqstp->rq_res.head[0].iov_base + PAGE_SIZE;
> + resp->xdr.p += 2 + XDR_QUADLEN(args->taglen);
> + resp->xdr.end = rqstp->rq_res.head[0].iov_base + PAGE_SIZE;
> resp->taglen = args->taglen;
> resp->tag = args->tag;
> resp->opcnt = 0;
> @@ -1326,7 +1326,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
> * failed response to the next operation. If we don't
> * have enough room, fail with ERR_RESOURCE.
> */
> - slack_bytes = (char *)resp->end - (char *)resp->p;
> + slack_bytes = (char *)resp->xdr.end - (char *)resp->xdr.p;
> if (slack_bytes < COMPOUND_SLACK_SPACE
> + COMPOUND_ERR_SLACK_SPACE) {
> BUG_ON(slack_bytes < COMPOUND_ERR_SLACK_SPACE);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fac2683..05cc3eb 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1569,10 +1569,10 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
> slot->sl_datalen = 0;
> return;
> }
> - slot->sl_datalen = (char *)resp->p - (char *)resp->cstate.datap;
> + slot->sl_datalen = (char *)resp->xdr.p - (char *)resp->cstate.datap;
> base = (char *)resp->cstate.datap -
> - (char *)resp->xbuf->head[0].iov_base;
> - if (read_bytes_from_xdr_buf(resp->xbuf, base, slot->sl_data,
> + (char *)resp->xdr.buf->head[0].iov_base;
> + if (read_bytes_from_xdr_buf(resp->xdr.buf, base, slot->sl_data,
> slot->sl_datalen))
> WARN("%s: sessions DRC could not cache compound\n", __func__);
> return;
> @@ -1626,7 +1626,7 @@ nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
> memcpy(resp->cstate.datap, slot->sl_data, slot->sl_datalen);
>
> resp->opcnt = slot->sl_opcnt;
> - resp->p = resp->cstate.datap + XDR_QUADLEN(slot->sl_datalen);
> + resp->xdr.p = resp->cstate.datap + XDR_QUADLEN(slot->sl_datalen);
> status = slot->sl_status;
>
> return status;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 18881f3..ef65ffc 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1747,10 +1747,10 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
> }
>
> #define RESERVE_SPACE(nbytes) do { \
> - p = resp->p; \
> - BUG_ON(p + XDR_QUADLEN(nbytes) > resp->end); \
> + p = resp->xdr.p; \
> + BUG_ON(p + XDR_QUADLEN(nbytes) > resp->xdr.end); \
> } while (0)
> -#define ADJUST_ARGS() resp->p = p
> +#define ADJUST_ARGS() resp->xdr.p = p
>
> /* Encode as an array of strings the string given with components
> * separated @sep, escaped with esc_enter and esc_exit.
> @@ -2751,9 +2751,9 @@ nfsd4_encode_getattr(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
> if (nfserr)
> return nfserr;
>
> - buflen = resp->end - resp->p - (COMPOUND_ERR_SLACK_SPACE >> 2);
> + buflen = resp->xdr.end - resp->xdr.p - (COMPOUND_ERR_SLACK_SPACE >> 2);
> nfserr = nfsd4_encode_fattr(fhp, fhp->fh_export, fhp->fh_dentry,
> - &resp->p, buflen, getattr->ga_bmval,
> + &resp->xdr.p, buflen, getattr->ga_bmval,
> resp->rqstp, 0);
> return nfserr;
> }
> @@ -2953,7 +2953,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>
> if (nfserr)
> return nfserr;
> - if (resp->xbuf->page_len)
> + if (resp->xdr.buf->page_len)
> return nfserr_resource;
>
> RESERVE_SPACE(8); /* eof flag and byte count */
> @@ -2991,18 +2991,18 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
> WRITE32(eof);
> WRITE32(maxcount);
> ADJUST_ARGS();
> - resp->xbuf->head[0].iov_len = (char*)p
> - - (char*)resp->xbuf->head[0].iov_base;
> - resp->xbuf->page_len = maxcount;
> + resp->xdr.buf->head[0].iov_len = (char*)p
> + - (char*)resp->xdr.buf->head[0].iov_base;
> + resp->xdr.buf->page_len = maxcount;
>
> /* Use rest of head for padding and remaining ops: */
> - resp->xbuf->tail[0].iov_base = p;
> - resp->xbuf->tail[0].iov_len = 0;
> + resp->xdr.buf->tail[0].iov_base = p;
> + resp->xdr.buf->tail[0].iov_len = 0;
> if (maxcount&3) {
> RESERVE_SPACE(4);
> WRITE32(0);
> - resp->xbuf->tail[0].iov_base += maxcount&3;
> - resp->xbuf->tail[0].iov_len = 4 - (maxcount&3);
> + resp->xdr.buf->tail[0].iov_base += maxcount&3;
> + resp->xdr.buf->tail[0].iov_len = 4 - (maxcount&3);
> ADJUST_ARGS();
> }
> return 0;
> @@ -3017,7 +3017,7 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd
>
> if (nfserr)
> return nfserr;
> - if (resp->xbuf->page_len)
> + if (resp->xdr.buf->page_len)
> return nfserr_resource;
> if (!*resp->rqstp->rq_next_page)
> return nfserr_resource;
> @@ -3041,18 +3041,18 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd
>
> WRITE32(maxcount);
> ADJUST_ARGS();
> - resp->xbuf->head[0].iov_len = (char*)p
> - - (char*)resp->xbuf->head[0].iov_base;
> - resp->xbuf->page_len = maxcount;
> + resp->xdr.buf->head[0].iov_len = (char*)p
> + - (char*)resp->xdr.buf->head[0].iov_base;
> + resp->xdr.buf->page_len = maxcount;
>
> /* Use rest of head for padding and remaining ops: */
> - resp->xbuf->tail[0].iov_base = p;
> - resp->xbuf->tail[0].iov_len = 0;
> + resp->xdr.buf->tail[0].iov_base = p;
> + resp->xdr.buf->tail[0].iov_len = 0;
> if (maxcount&3) {
> RESERVE_SPACE(4);
> WRITE32(0);
> - resp->xbuf->tail[0].iov_base += maxcount&3;
> - resp->xbuf->tail[0].iov_len = 4 - (maxcount&3);
> + resp->xdr.buf->tail[0].iov_base += maxcount&3;
> + resp->xdr.buf->tail[0].iov_len = 4 - (maxcount&3);
> ADJUST_ARGS();
> }
> return 0;
> @@ -3068,7 +3068,7 @@ nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
>
> if (nfserr)
> return nfserr;
> - if (resp->xbuf->page_len)
> + if (resp->xdr.buf->page_len)
> return nfserr_resource;
> if (!*resp->rqstp->rq_next_page)
> return nfserr_resource;
> @@ -3080,7 +3080,8 @@ nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
> WRITE32(0);
> WRITE32(0);
> ADJUST_ARGS();
> - resp->xbuf->head[0].iov_len = ((char*)resp->p) - (char*)resp->xbuf->head[0].iov_base;
> + resp->xdr.buf->head[0].iov_len = ((char*)resp->xdr.p)
> + - (char*)resp->xdr.buf->head[0].iov_base;
> tailbase = p;
>
> maxcount = PAGE_SIZE;
> @@ -3121,14 +3122,14 @@ nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
> p = readdir->buffer;
> *p++ = 0; /* no more entries */
> *p++ = htonl(readdir->common.err == nfserr_eof);
> - resp->xbuf->page_len = ((char*)p) -
> + resp->xdr.buf->page_len = ((char*)p) -
> (char*)page_address(*(resp->rqstp->rq_next_page-1));
>
> /* Use rest of head for padding and remaining ops: */
> - resp->xbuf->tail[0].iov_base = tailbase;
> - resp->xbuf->tail[0].iov_len = 0;
> - resp->p = resp->xbuf->tail[0].iov_base;
> - resp->end = resp->p + (PAGE_SIZE - resp->xbuf->head[0].iov_len)/4;
> + resp->xdr.buf->tail[0].iov_base = tailbase;
> + resp->xdr.buf->tail[0].iov_len = 0;
> + resp->xdr.p = resp->xdr.buf->tail[0].iov_base;
> + resp->xdr.end = resp->xdr.p + (PAGE_SIZE - resp->xdr.buf->head[0].iov_len)/4;
>
> return 0;
> err_no_verf:
> @@ -3587,10 +3588,10 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 pad)
> session = resp->cstate.session;
>
> if (xb->page_len == 0) {
> - length = (char *)resp->p - (char *)xb->head[0].iov_base + pad;
> + length = (char *)resp->xdr.p - (char *)xb->head[0].iov_base + pad;
> } else {
> if (xb->tail[0].iov_base && xb->tail[0].iov_len > 0)
> - tlen = (char *)resp->p - (char *)xb->tail[0].iov_base;
> + tlen = (char *)resp->xdr.p - (char *)xb->tail[0].iov_base;
>
> length = xb->head[0].iov_len + xb->page_len + tlen + pad;
> }
> @@ -3629,7 +3630,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> op->status = nfsd4_check_resp_size(resp, 0);
> if (so) {
> so->so_replay.rp_status = op->status;
> - so->so_replay.rp_buflen = (char *)resp->p - (char *)(statp+1);
> + so->so_replay.rp_buflen = (char *)resp->xdr.p - (char *)(statp+1);
> memcpy(so->so_replay.rp_buf, statp+1, so->so_replay.rp_buflen);
> }
> status:
> @@ -3731,7 +3732,7 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
> iov = &rqstp->rq_res.tail[0];
> else
> iov = &rqstp->rq_res.head[0];
> - iov->iov_len = ((char*)resp->p) - (char*)iov->iov_base;
> + iov->iov_len = ((char*)resp->xdr.p) - (char*)iov->iov_base;
> BUG_ON(iov->iov_len > PAGE_SIZE);
> if (nfsd4_has_session(cs)) {
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 5ea7df3..6884d70 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -506,9 +506,7 @@ struct nfsd4_compoundargs {
>
> struct nfsd4_compoundres {
> /* scratch variables for XDR encode */
> - __be32 * p;
> - __be32 * end;
> - struct xdr_buf * xbuf;
> + struct xdr_stream xdr;
> struct svc_rqst * rqstp;
>
> u32 taglen;
>

2014-05-13 14:47:47

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH 04/43] nfsd4: reserve head space for krb5 integ/priv info

On Mon, May 12, 2014 at 10:05:53PM -0700, Christoph Hellwig wrote:
> On Mon, May 12, 2014 at 05:45:45PM -0400, J. Bruce Fields wrote:
> > Yes. At the end of this series we have RPC_MAX_AUTH_SIZE scattered
> > around in a few different places. Rather than have each place have some
> > flavor-specific logic I think I'd like the auth code to set an
> > rq_auth_slack field in the struct svc_rqst for code like this to use.
>
> That sounds pretty reasonable to me.

Here's an attempt.

(The limit still could be tightened a lot. RPC_MAX_AUTH_SIZE really has
nothing to do with the amount of extra space required for krb5i/p, it's
just a random constant that we happen to know is plenty large enough.)

--b.

commit a5f2429b2756a66c35aab463a2784f334718719f
Author: J. Bruce Fields <[email protected]>
Date: Mon May 12 18:10:58 2014 -0400

nfsd4: better reservation of head space for krb5

RPC_MAX_AUTH_SIZE is scattered around several places. Better to set it
once in the auth code, where this kind of estimate should be made. And
while we're at it we can leave it zero when we're not using krb5i or
krb5p.

Signed-off-by: J. Bruce Fields <[email protected]>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3ab29ba..d1885a1 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1211,13 +1211,13 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
xdr->buf = buf;
xdr->iov = head;
xdr->p = head->iov_base + head->iov_len;
- xdr->end = head->iov_base + PAGE_SIZE - 2 * RPC_MAX_AUTH_SIZE;
+ xdr->end = head->iov_base + PAGE_SIZE - rqstp->rq_auth_slack;
/* Tail and page_len should be zero at this point: */
buf->len = buf->head[0].iov_len;
xdr->scratch.iov_len = 0;
xdr->page_ptr = buf->pages;
buf->buflen = PAGE_SIZE * (1 + rqstp->rq_page_end - buf->pages)
- - 2 * RPC_MAX_AUTH_SIZE;
+ - rqstp->rq_auth_slack;
}

/*
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2526426..7336462 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2284,7 +2284,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
session->se_fchannel.maxresp_sz;
status = (seq->cachethis) ? nfserr_rep_too_big_to_cache :
nfserr_rep_too_big;
- if (xdr_restrict_buflen(xdr, buflen - 2 * RPC_MAX_AUTH_SIZE))
+ if (xdr_restrict_buflen(xdr, buflen - rqstp->rq_auth_slack))
goto out_put_session;
svc_reserve(rqstp, buflen);

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 53731b7..7a8259c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1628,7 +1628,8 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
DECODE_HEAD;
struct nfsd4_op *op;
bool cachethis = false;
- int max_reply = 2 * RPC_MAX_AUTH_SIZE; /* uh, kind of a guess */
+ int auth_slack = argp->rqstp->rq_auth_slack;
+ int max_reply = auth_slack;
int readcount = 0;
int readbytes = 0;
int i;
@@ -1693,7 +1694,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
svc_reserve(argp->rqstp, max_reply + readbytes);
argp->rqstp->rq_cachetype = cachethis ? RC_REPLBUFF : RC_NOCACHE;

- if (readcount > 1 || max_reply > PAGE_SIZE - 2*RPC_MAX_AUTH_SIZE)
+ if (readcount > 1 || max_reply > PAGE_SIZE - auth_slack)
argp->rqstp->rq_splice_ok = false;

DECODE_TAIL;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 39c50e1..b987bd9 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -259,7 +259,10 @@ struct svc_rqst {
void * rq_argp; /* decoded arguments */
void * rq_resp; /* xdr'd results */
void * rq_auth_data; /* flavor-specific data */
-
+ int rq_auth_slack; /* extra space xdr code
+ * should leave in head
+ * for krb5i, krb5p.
+ */
int rq_reserved; /* space on socket outq
* reserved for this request
*/
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 0f73f45..4ce5ecce 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1503,6 +1503,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp, __be32 *authp)
if (unwrap_integ_data(rqstp, &rqstp->rq_arg,
gc->gc_seq, rsci->mechctx))
goto garbage_args;
+ rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE;
break;
case RPC_GSS_SVC_PRIVACY:
/* placeholders for length and seq. number: */
@@ -1511,6 +1512,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp, __be32 *authp)
if (unwrap_priv_data(rqstp, &rqstp->rq_arg,
gc->gc_seq, rsci->mechctx))
goto garbage_args;
+ rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE * 2;
break;
default:
goto auth_err;
diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
index 2af7b0c..79c0f34 100644
--- a/net/sunrpc/svcauth.c
+++ b/net/sunrpc/svcauth.c
@@ -54,6 +54,8 @@ svc_authenticate(struct svc_rqst *rqstp, __be32 *authp)
}
spin_unlock(&authtab_lock);

+ rqstp->rq_auth_slack = 0;
+
rqstp->rq_authop = aops;
return aops->accept(rqstp, authp);
}