From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from daytona.panasas.com ([67.152.220.89]:40665 "EHLO daytona.panasas.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756214Ab0KRMur (ORCPT ); Thu, 18 Nov 2010 07:50:47 -0500 Message-ID: <4CE52124.6020906@panasas.com> Date: Thu, 18 Nov 2010 14:50:44 +0200 From: Boaz Harrosh To: Chuck Lever CC: trond.myklebust@netapp.com, linux-nfs@vger.kernel.org Subject: Re: [PATCH 01/17] NFS: Introduce new-style XDR encoding functions for NFSv2 References: <20101117174112.29177.69734.stgit@matisse.1015granger.net> <20101117174339.29177.12341.stgit@matisse.1015granger.net> In-Reply-To: <20101117174339.29177.12341.stgit@matisse.1015granger.net> Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On 11/17/2010 07:43 PM, Chuck Lever wrote: > We're interested in taking advantage of the safety benefits of > xdr_streams. These data structures allow more careful checking for > buffer overflow while encoding. More careful type checking is also > introduced in the new functions. > > For efficiency, we also eventually want to be able to pass xdr_streams > from call_encode() to all XDR encoding functions, rather than building > an xdr_stream in every XDR encoding function in the kernel. To do > this means all encoders must be ready to handle a passed-in > xdr_stream. > > We follow the modern paradigm for encoders of BUGging on error and > always returning a zero status code. > > Static helper functions are left without the "inline" directive. This > allows the compiler to choose automatically how to optimize these for > size or speed. > > Signed-off-by: Chuck Lever > Tested-by: J. Bruce Fields > --- > > fs/nfs/nfs2xdr.c | 406 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 403 insertions(+), 3 deletions(-) > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > index e6bf457..77a3eb9 100644 > --- a/fs/nfs/nfs2xdr.c > +++ b/fs/nfs/nfs2xdr.c > @@ -61,6 +61,23 @@ > #define NFS_readdirres_sz (1) > #define NFS_statfsres_sz (1+NFS_info_sz) > > + > +/* > + * While encoding arguments, set up the reply buffer in advance to > + * receive reply data directly into the page cache. > + */ > +static void prepare_reply_buffer(struct rpc_rqst *req, struct page **pages, > + unsigned int base, unsigned int len, > + unsigned int bufsize) > +{ > + struct rpc_auth *auth = req->rq_cred->cr_auth; > + unsigned int replen; > + > + replen = RPC_REPHDRSIZE + auth->au_rslack + bufsize; > + xdr_inline_pages(&req->rq_rcv_buf, replen << 2, pages, base, len); > +} > + > + > /* > * Common NFS XDR functions as inlines > */ > @@ -81,7 +98,7 @@ xdr_decode_fhandle(__be32 *p, struct nfs_fh *fhandle) > } > > static inline __be32* > -xdr_encode_time(__be32 *p, struct timespec *timep) > +xdr_encode_time(__be32 *p, const struct timespec *timep) > { > *p++ = htonl(timep->tv_sec); Please use cpu_to_beXX like below? Boaz > /* Convert nanoseconds into microseconds */ > @@ -90,7 +107,7 @@ xdr_encode_time(__be32 *p, struct timespec *timep) > } > > static inline __be32* > -xdr_encode_current_server_time(__be32 *p, struct timespec *timep) > +xdr_encode_current_server_time(__be32 *p, const struct timespec *timep) > { > /* > * Passing the invalid value useconds=1000000 is a > @@ -174,6 +191,136 @@ xdr_encode_sattr(__be32 *p, struct iattr *attr) > } > > /* > + * Encode/decode NFSv2 basic data types > + * > + * Basic NFSv2 data types are defined in section 2.3 of RFC 1094: > + * "NFS: Network File System Protocol Specification". > + * > + * Not all basic data types have their own encoding and decoding > + * functions. For run-time efficiency, some data types are encoded > + * or decoded inline. > + */ > + > +/* > + * 2.3.3. fhandle > + * > + * typedef opaque fhandle[FHSIZE]; > + */ > +static void encode_fhandle(struct xdr_stream *xdr, const struct nfs_fh *fh) > +{ > + __be32 *p; > + > + BUG_ON(fh->size != NFS2_FHSIZE); > + p = xdr_reserve_space(xdr, NFS2_FHSIZE); > + memcpy(p, fh->data, NFS2_FHSIZE); > +} > + > +/* > + * 2.3.6. sattr > + * > + * struct sattr { > + * unsigned int mode; > + * unsigned int uid; > + * unsigned int gid; > + * unsigned int size; > + * timeval atime; > + * timeval mtime; > + * }; > + */ > + > +#define NFS2_SATTR_NOT_SET (0xffffffff) > + > +static __be32 *xdr_time_not_set(__be32 *p) > +{ > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + return p; > +} > + > +static void encode_sattr(struct xdr_stream *xdr, const struct iattr *attr) > +{ > + __be32 *p; > + > + p = xdr_reserve_space(xdr, NFS_sattr_sz << 2); > + > + if (attr->ia_valid & ATTR_MODE) > + *p++ = cpu_to_be32(attr->ia_mode); > + else > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + if (attr->ia_valid & ATTR_UID) > + *p++ = cpu_to_be32(attr->ia_uid); > + else > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + if (attr->ia_valid & ATTR_GID) > + *p++ = cpu_to_be32(attr->ia_gid); > + else > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + if (attr->ia_valid & ATTR_SIZE) > + *p++ = cpu_to_be32((u32)attr->ia_size); > + else > + *p++ = cpu_to_be32(NFS2_SATTR_NOT_SET); > + > + if (attr->ia_valid & ATTR_ATIME_SET) > + p = xdr_encode_time(p, &attr->ia_atime); > + else if (attr->ia_valid & ATTR_ATIME) > + p = xdr_encode_current_server_time(p, &attr->ia_atime); > + else > + p = xdr_time_not_set(p); > + if (attr->ia_valid & ATTR_MTIME_SET) > + xdr_encode_time(p, &attr->ia_mtime); > + else if (attr->ia_valid & ATTR_MTIME) > + xdr_encode_current_server_time(p, &attr->ia_mtime); > + else > + xdr_time_not_set(p); > +} > + > +/* > + * 2.3.7. filename > + * > + * typedef string filename; > + */ > +static void encode_filename(struct xdr_stream *xdr, > + const char *name, u32 length) > +{ > + __be32 *p; > + > + BUG_ON(length > NFS2_MAXNAMLEN); > + p = xdr_reserve_space(xdr, 4 + length); > + xdr_encode_opaque(p, name, length); > +} > + > +/* > + * 2.3.8. path > + * > + * typedef string path; > + */ > +static void encode_path(struct xdr_stream *xdr, struct page **pages, u32 length) > +{ > + __be32 *p; > + > + BUG_ON(length > NFS2_MAXPATHLEN); > + p = xdr_reserve_space(xdr, 4); > + *p = cpu_to_be32(length); > + xdr_write_pages(xdr, pages, 0, length); > +} > + > +/* > + * 2.3.10. diropargs > + * > + * struct diropargs { > + * fhandle dir; > + * filename name; > + * }; > + */ > +static void encode_diropargs(struct xdr_stream *xdr, const struct nfs_fh *fh, > + const char *name, u32 length) > +{ > + encode_fhandle(xdr, fh); > + encode_filename(xdr, name, length); > +} > + > + > +/* > * NFS encode functions > */ > /* > @@ -188,6 +335,16 @@ nfs_xdr_fhandle(struct rpc_rqst *req, __be32 *p, struct nfs_fh *fh) > return 0; > } > > +static int nfs2_xdr_enc_fhandle(struct rpc_rqst *req, __be32 *p, > + const struct nfs_fh *fh) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_fhandle(&xdr, fh); > + return 0; > +} > + > /* > * Encode SETATTR arguments > */ > @@ -201,6 +358,25 @@ nfs_xdr_sattrargs(struct rpc_rqst *req, __be32 *p, struct nfs_sattrargs *args) > } > > /* > + * 2.2.3. sattrargs > + * > + * struct sattrargs { > + * fhandle file; > + * sattr attributes; > + * }; > + */ > +static int nfs2_xdr_enc_sattrargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_sattrargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_fhandle(&xdr, args->fh); > + encode_sattr(&xdr, args->sattr); > + return 0; > +} > + > +/* > * Encode directory ops argument > * LOOKUP, RMDIR > */ > @@ -213,6 +389,16 @@ nfs_xdr_diropargs(struct rpc_rqst *req, __be32 *p, struct nfs_diropargs *args) > return 0; > } > > +static int nfs2_xdr_enc_diropargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_diropargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_diropargs(&xdr, args->fh, args->name, args->len); > + return 0; > +} > + > /* > * Encode REMOVE argument > */ > @@ -225,6 +411,18 @@ nfs_xdr_removeargs(struct rpc_rqst *req, __be32 *p, const struct nfs_removeargs > return 0; > } > > +static int nfs2_xdr_enc_readlinkargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_readlinkargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_fhandle(&xdr, args->fh); > + prepare_reply_buffer(req, args->pages, args->pgbase, > + args->pglen, NFS_readlinkres_sz); > + return 0; > +} > + > /* > * Arguments to a READ call. Since we read data directly into the page > * cache, we also set up the reply iovec here so that iov[1] points > @@ -253,6 +451,44 @@ nfs_xdr_readargs(struct rpc_rqst *req, __be32 *p, struct nfs_readargs *args) > } > > /* > + * 2.2.7. readargs > + * > + * struct readargs { > + * fhandle file; > + * unsigned offset; > + * unsigned count; > + * unsigned totalcount; > + * }; > + */ > +static void encode_readargs(struct xdr_stream *xdr, > + const struct nfs_readargs *args) > +{ > + u32 offset = args->offset; > + u32 count = args->count; > + __be32 *p; > + > + encode_fhandle(xdr, args->fh); > + > + p = xdr_reserve_space(xdr, 4 + 4 + 4); > + *p++ = cpu_to_be32(offset); > + *p++ = cpu_to_be32(count); > + *p = cpu_to_be32(count); > +} > + > +static int nfs2_xdr_enc_readargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_readargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_readargs(&xdr, args); > + prepare_reply_buffer(req, args->pages, args->pgbase, > + args->count, NFS_readres_sz); > + req->rq_rcv_buf.flags |= XDRBUF_READ; > + return 0; > +} > + > +/* > * Decode READ reply > */ > static int > @@ -318,6 +554,47 @@ nfs_xdr_writeargs(struct rpc_rqst *req, __be32 *p, struct nfs_writeargs *args) > } > > /* > + * 2.2.9. writeargs > + * > + * struct writeargs { > + * fhandle file; > + * unsigned beginoffset; > + * unsigned offset; > + * unsigned totalcount; > + * nfsdata data; > + * }; > + */ > +static void encode_writeargs(struct xdr_stream *xdr, > + const struct nfs_writeargs *args) > +{ > + u32 offset = args->offset; > + u32 count = args->count; > + __be32 *p; > + > + encode_fhandle(xdr, args->fh); > + > + p = xdr_reserve_space(xdr, 4 + 4 + 4 + 4); > + *p++ = cpu_to_be32(offset); > + *p++ = cpu_to_be32(offset); > + *p++ = cpu_to_be32(count); > + > + /* nfsdata */ > + *p = cpu_to_be32(count); > + xdr_write_pages(xdr, args->pages, args->pgbase, count); > +} > + > +static int nfs2_xdr_enc_writeargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_writeargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_writeargs(&xdr, args); > + xdr.buf->flags |= XDRBUF_WRITE; > + return 0; > +} > + > +/* > * Encode create arguments > * CREATE, MKDIR > */ > @@ -332,6 +609,35 @@ nfs_xdr_createargs(struct rpc_rqst *req, __be32 *p, struct nfs_createargs *args) > } > > /* > + * 2.2.10. createargs > + * > + * struct createargs { > + * diropargs where; > + * sattr attributes; > + * }; > + */ > +static int nfs2_xdr_enc_createargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_createargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_diropargs(&xdr, args->fh, args->name, args->len); > + encode_sattr(&xdr, args->sattr); > + return 0; > +} > + > +static int nfs2_xdr_enc_removeargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_removeargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_diropargs(&xdr, args->fh, args->name.name, args->name.len); > + return 0; > +} > + > +/* > * Encode RENAME arguments > */ > static int > @@ -346,6 +652,27 @@ nfs_xdr_renameargs(struct rpc_rqst *req, __be32 *p, struct nfs_renameargs *args) > } > > /* > + * 2.2.12. renameargs > + * > + * struct renameargs { > + * diropargs from; > + * diropargs to; > + * }; > + */ > +static int nfs2_xdr_enc_renameargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_renameargs *args) > +{ > + const struct qstr *old = args->old_name; > + const struct qstr *new = args->new_name; > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_diropargs(&xdr, args->old_dir, old->name, old->len); > + encode_diropargs(&xdr, args->new_dir, new->name, new->len); > + return 0; > +} > + > +/* > * Encode LINK arguments > */ > static int > @@ -359,6 +686,25 @@ nfs_xdr_linkargs(struct rpc_rqst *req, __be32 *p, struct nfs_linkargs *args) > } > > /* > + * 2.2.13. linkargs > + * > + * struct linkargs { > + * fhandle from; > + * diropargs to; > + * }; > + */ > +static int nfs2_xdr_enc_linkargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_linkargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_fhandle(&xdr, args->fromfh); > + encode_diropargs(&xdr, args->tofh, args->toname, args->tolen); > + return 0; > +} > + > +/* > * Encode SYMLINK arguments > */ > static int > @@ -388,6 +734,27 @@ nfs_xdr_symlinkargs(struct rpc_rqst *req, __be32 *p, struct nfs_symlinkargs *arg > } > > /* > + * 2.2.14. symlinkargs > + * > + * struct symlinkargs { > + * diropargs from; > + * path to; > + * sattr attributes; > + * }; > + */ > +static int nfs2_xdr_enc_symlinkargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_symlinkargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_diropargs(&xdr, args->fromfh, args->fromname, args->fromlen); > + encode_path(&xdr, args->pages, args->pathlen); > + encode_sattr(&xdr, args->sattr); > + return 0; > +} > + > +/* > * Encode arguments to readdir call > */ > static int > @@ -409,6 +776,39 @@ nfs_xdr_readdirargs(struct rpc_rqst *req, __be32 *p, struct nfs_readdirargs *arg > } > > /* > + * 2.2.17. readdirargs > + * > + * struct readdirargs { > + * fhandle dir; > + * nfscookie cookie; > + * unsigned count; > + * }; > + */ > +static void encode_readdirargs(struct xdr_stream *xdr, > + const struct nfs_readdirargs *args) > +{ > + __be32 *p; > + > + encode_fhandle(xdr, args->fh); > + > + p = xdr_reserve_space(xdr, 4 + 4); > + *p++ = cpu_to_be32(args->cookie); > + *p = cpu_to_be32(args->count); > +} > + > +static int nfs2_xdr_enc_readdirargs(struct rpc_rqst *req, __be32 *p, > + const struct nfs_readdirargs *args) > +{ > + struct xdr_stream xdr; > + > + xdr_init_encode(&xdr, &req->rq_snd_buf, p); > + encode_readdirargs(&xdr, args); > + prepare_reply_buffer(req, args->pages, 0, > + args->count, NFS_readdirres_sz); > + return 0; > +} > + > +/* > * Decode the result of a readdir call. > * We're not really decoding anymore, we just leave the buffer untouched > * and only check that it is syntactically correct. > @@ -696,7 +1096,7 @@ nfs_stat_to_errno(int stat) > #define PROC(proc, argtype, restype, timer) \ > [NFSPROC_##proc] = { \ > .p_proc = NFSPROC_##proc, \ > - .p_encode = (kxdrproc_t) nfs_xdr_##argtype, \ > + .p_encode = (kxdrproc_t)nfs2_xdr_enc_##argtype, \ > .p_decode = (kxdrproc_t) nfs_xdr_##restype, \ > .p_arglen = NFS_##argtype##_sz, \ > .p_replen = NFS_##restype##_sz, \ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >