2011-10-27 18:49:18

by Benny Halevy

[permalink] [raw]
Subject: [RFC] nfsd4: DEBUG: XDR_ERROR()

From: Benny Halevy <[email protected]>

Signed-off-by: Benny Halevy <[email protected]>
---

Bruce, would you consider this patch to help
debug xdr parsing errors by dprinting the line where the
xdr error detected in a more relevant place?

Benny

fs/nfsd/nfs4xdr.c | 76 +++++++++++++++++++++++++---------------------------
1 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2f894c6..d5fd7d0 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -79,6 +79,12 @@
return 0;
}

+#define XDR_ERROR() do { \
+ dprintk("NFSD: xdr error (%s:%d)\n", \
+ __FILE__, __LINE__); \
+ goto xdr_error; \
+} while (0)
+
#define DECODE_HEAD \
__be32 *p; \
__be32 status
@@ -87,8 +93,6 @@
out: \
return status; \
xdr_error: \
- dprintk("NFSD: xdr error (%s:%d)\n", \
- __FILE__, __LINE__); \
status = nfserr_bad_xdr; \
goto out

@@ -109,11 +113,8 @@
#define SAVEMEM(x,nbytes) do { \
if (!(x = (p==argp->tmp || p == argp->tmpp) ? \
savemem(argp, p, nbytes) : \
- (char *)p)) { \
- dprintk("NFSD: xdr error (%s:%d)\n", \
- __FILE__, __LINE__); \
- goto xdr_error; \
- } \
+ (char *)p)) \
+ XDR_ERROR(); \
p += XDR_QUADLEN(nbytes); \
} while (0)
#define COPYMEM(x,nbytes) do { \
@@ -126,11 +127,8 @@
if (nbytes <= (u32)((char *)argp->end - (char *)argp->p)) { \
p = argp->p; \
argp->p += XDR_QUADLEN(nbytes); \
- } else if (!(p = read_buf(argp, nbytes))) { \
- dprintk("NFSD: xdr error (%s:%d)\n", \
- __FILE__, __LINE__); \
- goto xdr_error; \
- } \
+ } else if (!(p = read_buf(argp, nbytes))) \
+ XDR_ERROR(); \
} while (0)

static void save_buf(struct nfsd4_compoundargs *argp, struct nfsd4_saved_compoundargs *savep)
@@ -243,7 +241,7 @@ static int zero_clientid(clientid_t *clid)
READ_BUF(4);
READ32(bmlen);
if (bmlen > 1000)
- goto xdr_error;
+ XDR_ERROR();

READ_BUF(bmlen << 2);
if (bmlen > 0)
@@ -373,7 +371,7 @@ static int zero_clientid(clientid_t *clid)
iattr->ia_valid |= ATTR_ATIME;
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}
}
if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
@@ -399,7 +397,7 @@ static int zero_clientid(clientid_t *clid)
iattr->ia_valid |= ATTR_MTIME;
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}
}
if (bmval[0] & ~NFSD_WRITEABLE_ATTRS_WORD0
@@ -407,7 +405,7 @@ static int zero_clientid(clientid_t *clid)
|| bmval[2] & ~NFSD_WRITEABLE_ATTRS_WORD2)
READ_BUF(expected_len - len);
else if (len != expected_len)
- goto xdr_error;
+ XDR_ERROR();

DECODE_TAIL;

@@ -556,7 +554,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
READ_BUF(28);
READ32(lock->lk_type);
if ((lock->lk_type < NFS4_READ_LT) || (lock->lk_type > NFS4_WRITEW_LT))
- goto xdr_error;
+ XDR_ERROR();
READ32(lock->lk_reclaim);
READ64(lock->lk_offset);
READ64(lock->lk_length);
@@ -593,7 +591,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
READ_BUF(32);
READ32(lockt->lt_type);
if((lockt->lt_type < NFS4_READ_LT) || (lockt->lt_type > NFS4_WRITEW_LT))
- goto xdr_error;
+ XDR_ERROR();
READ64(lockt->lt_offset);
READ64(lockt->lt_length);
COPYMEM(&lockt->lt_clientid, 8);
@@ -612,7 +610,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
READ_BUF(8);
READ32(locku->lu_type);
if ((locku->lu_type < NFS4_READ_LT) || (locku->lu_type > NFS4_WRITEW_LT))
- goto xdr_error;
+ XDR_ERROR();
READ32(locku->lu_seqid);
status = nfsd4_decode_stateid(argp, &locku->lu_stateid);
if (status)
@@ -742,15 +740,15 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
status = nfsd4_decode_share_access(argp, &open->op_share_access,
&open->op_deleg_want, &dummy);
if (status)
- goto xdr_error;
+ XDR_ERROR();
status = nfsd4_decode_share_deny(argp, &open->op_share_deny);
if (status)
- goto xdr_error;
+ XDR_ERROR();
READ_BUF(sizeof(clientid_t));
COPYMEM(&open->op_clientid, sizeof(clientid_t));
status = nfsd4_decode_opaque(argp, &open->op_owner);
if (status)
- goto xdr_error;
+ XDR_ERROR();
READ_BUF(4);
READ32(open->op_create);
switch (open->op_create) {
@@ -773,7 +771,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
break;
case NFS4_CREATE_EXCLUSIVE4_1:
if (argp->minorversion < 1)
- goto xdr_error;
+ XDR_ERROR();
READ_BUF(8);
COPYMEM(open->op_verf.data, 8);
status = nfsd4_decode_fattr(argp, open->op_bmval,
@@ -782,11 +780,11 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
goto out;
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}

/* open_claim */
@@ -820,18 +818,18 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
case NFS4_OPEN_CLAIM_FH:
case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
if (argp->minorversion < 1)
- goto xdr_error;
+ XDR_ERROR();
/* void */
break;
case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
if (argp->minorversion < 1)
- goto xdr_error;
+ XDR_ERROR();
status = nfsd4_decode_stateid(argp, &open->op_delegate_stateid);
if (status)
return status;
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}

DECODE_TAIL;
@@ -879,7 +877,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ_BUF(4);
READ32(putfh->pf_fhlen);
if (putfh->pf_fhlen > NFS4_FHSIZE)
- goto xdr_error;
+ XDR_ERROR();
READ_BUF(putfh->pf_fhlen);
SAVEMEM(putfh->pf_fhval, putfh->pf_fhlen);

@@ -1093,7 +1091,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ64(write->wr_offset);
READ32(write->wr_stable_how);
if (write->wr_stable_how > 2)
- goto xdr_error;
+ XDR_ERROR();
READ32(write->wr_buflen);

/* Sorry .. no magic macros for this.. *
@@ -1104,7 +1102,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
if (avail + argp->pagelen < write->wr_buflen) {
dprintk("NFSD: xdr error (%s:%d)\n",
__FILE__, __LINE__);
- goto xdr_error;
+ XDR_ERROR();
}
argp->rqstp->rq_vec[0].iov_base = p;
argp->rqstp->rq_vec[0].iov_len = avail;
@@ -1221,7 +1219,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ32(dummy);
break;
default:
- goto xdr_error;
+ XDR_ERROR();
}

/* Ignore Implementation ID */
@@ -1229,7 +1227,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ32(dummy);

if (dummy > 1)
- goto xdr_error;
+ XDR_ERROR();

if (dummy == 1) {
/* nii_domain */
@@ -1281,7 +1279,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ32(sess->fore_channel.rdma_attrs);
} else if (sess->fore_channel.nr_rdma_attrs > 1) {
dprintk("Too many fore channel attr bitmaps!\n");
- goto xdr_error;
+ XDR_ERROR();
}

/* Back channel attrs */
@@ -1298,7 +1296,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
READ32(sess->back_channel.rdma_attrs);
} else if (sess->back_channel.nr_rdma_attrs > 1) {
dprintk("Too many back channel attr bitmaps!\n");
- goto xdr_error;
+ XDR_ERROR();
}

READ_BUF(8);
@@ -1410,7 +1408,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne

nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
- goto xdr_error;
+ XDR_ERROR();

test_stateid->ts_saved_args = argp;
save_buf(argp, &test_stateid->ts_savedp);
@@ -1588,16 +1586,16 @@ struct nfsd4_minorversion_ops {
READ32(argp->opcnt);

if (argp->taglen > NFSD4_MAX_TAGLEN)
- goto xdr_error;
+ XDR_ERROR();
if (argp->opcnt > 100)
- goto xdr_error;
+ XDR_ERROR();

if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL);
if (!argp->ops) {
argp->ops = argp->iops;
dprintk("nfsd: couldn't allocate room for COMPOUND\n");
- goto xdr_error;
+ XDR_ERROR();
}
}

--
1.7.6



2011-10-29 05:25:48

by Boaz Harrosh

[permalink] [raw]
Subject: Re: [RFC] nfsd4: DEBUG: XDR_ERROR()

On 10/28/2011 05:09 AM, Benny Halevy wrote:
>
> I agree with all of the above but its a project larger than I can
> chew right now. This patch just improves the existing mechanism, for
> development use, to help catch parser bugs rather than actual xdr errors.
>
<>

>>>
>>> +#define XDR_ERROR() do { \
>>> + dprintk("NFSD: xdr error (%s:%d)\n", \
>>> + __FILE__, __LINE__); \
>>> + goto xdr_error; \
>>> +} while (0)
>>> +

What about a compromise:

+#define GOTO_XDR_ERROR() do { \
+ dprintk("NFSD: xdr error (%s:%d)\n", \
+ __FILE__, __LINE__); \
+ goto xdr_error; \
+} while (0)

or even:

+#define XDR_ERROR_GOTO(xdr_error) do { \
+ dprintk("NFSD: xdr error (%s:%d)\n", \
+ __FILE__, __LINE__); \
+ goto xdr_error; \
+} while (0)

and XDR_ERROR_GOTO(xdr_error) every where.

So the patch below stays exactly the same size
but the goto is not masked out, and no one can
complain about things happening hidden underground.

Just My $0.017
Boaz

2011-10-28 11:34:44

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [RFC] nfsd4: DEBUG: XDR_ERROR()

Did I just get moved to Netapp without anyone telling me? Anyway:

On Thu, Oct 27, 2011 at 08:49:11PM +0200, Benny Halevy wrote:
> From: Benny Halevy <[email protected]>
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
>
> Bruce, would you consider this patch to help
> debug xdr parsing errors by dprinting the line where the
> xdr error detected in a more relevant place?

I don't object to the goal, but:

- I'd like to make this code less dependent on the preprocessor
rather than more.

- In particular I don't like having goto's and goto labels
hidden in macros--I think they make the control flow less
obvious.

- There have been exceptions (mea culpa!), but usually I think
that we should be able to catch xdr decoding problems early,
so having this debugging out in the field isn't as important
as debugging elsewhere.

--b.

>
> Benny
>
> fs/nfsd/nfs4xdr.c | 76 +++++++++++++++++++++++++---------------------------
> 1 files changed, 37 insertions(+), 39 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2f894c6..d5fd7d0 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -79,6 +79,12 @@
> return 0;
> }
>
> +#define XDR_ERROR() do { \
> + dprintk("NFSD: xdr error (%s:%d)\n", \
> + __FILE__, __LINE__); \
> + goto xdr_error; \
> +} while (0)
> +
> #define DECODE_HEAD \
> __be32 *p; \
> __be32 status
> @@ -87,8 +93,6 @@
> out: \
> return status; \
> xdr_error: \
> - dprintk("NFSD: xdr error (%s:%d)\n", \
> - __FILE__, __LINE__); \
> status = nfserr_bad_xdr; \
> goto out
>
> @@ -109,11 +113,8 @@
> #define SAVEMEM(x,nbytes) do { \
> if (!(x = (p==argp->tmp || p == argp->tmpp) ? \
> savemem(argp, p, nbytes) : \
> - (char *)p)) { \
> - dprintk("NFSD: xdr error (%s:%d)\n", \
> - __FILE__, __LINE__); \
> - goto xdr_error; \
> - } \
> + (char *)p)) \
> + XDR_ERROR(); \
> p += XDR_QUADLEN(nbytes); \
> } while (0)
> #define COPYMEM(x,nbytes) do { \
> @@ -126,11 +127,8 @@
> if (nbytes <= (u32)((char *)argp->end - (char *)argp->p)) { \
> p = argp->p; \
> argp->p += XDR_QUADLEN(nbytes); \
> - } else if (!(p = read_buf(argp, nbytes))) { \
> - dprintk("NFSD: xdr error (%s:%d)\n", \
> - __FILE__, __LINE__); \
> - goto xdr_error; \
> - } \
> + } else if (!(p = read_buf(argp, nbytes))) \
> + XDR_ERROR(); \
> } while (0)
>
> static void save_buf(struct nfsd4_compoundargs *argp, struct nfsd4_saved_compoundargs *savep)
> @@ -243,7 +241,7 @@ static int zero_clientid(clientid_t *clid)
> READ_BUF(4);
> READ32(bmlen);
> if (bmlen > 1000)
> - goto xdr_error;
> + XDR_ERROR();
>
> READ_BUF(bmlen << 2);
> if (bmlen > 0)
> @@ -373,7 +371,7 @@ static int zero_clientid(clientid_t *clid)
> iattr->ia_valid |= ATTR_ATIME;
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
> }
> if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
> @@ -399,7 +397,7 @@ static int zero_clientid(clientid_t *clid)
> iattr->ia_valid |= ATTR_MTIME;
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
> }
> if (bmval[0] & ~NFSD_WRITEABLE_ATTRS_WORD0
> @@ -407,7 +405,7 @@ static int zero_clientid(clientid_t *clid)
> || bmval[2] & ~NFSD_WRITEABLE_ATTRS_WORD2)
> READ_BUF(expected_len - len);
> else if (len != expected_len)
> - goto xdr_error;
> + XDR_ERROR();
>
> DECODE_TAIL;
>
> @@ -556,7 +554,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
> READ_BUF(28);
> READ32(lock->lk_type);
> if ((lock->lk_type < NFS4_READ_LT) || (lock->lk_type > NFS4_WRITEW_LT))
> - goto xdr_error;
> + XDR_ERROR();
> READ32(lock->lk_reclaim);
> READ64(lock->lk_offset);
> READ64(lock->lk_length);
> @@ -593,7 +591,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
> READ_BUF(32);
> READ32(lockt->lt_type);
> if((lockt->lt_type < NFS4_READ_LT) || (lockt->lt_type > NFS4_WRITEW_LT))
> - goto xdr_error;
> + XDR_ERROR();
> READ64(lockt->lt_offset);
> READ64(lockt->lt_length);
> COPYMEM(&lockt->lt_clientid, 8);
> @@ -612,7 +610,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
> READ_BUF(8);
> READ32(locku->lu_type);
> if ((locku->lu_type < NFS4_READ_LT) || (locku->lu_type > NFS4_WRITEW_LT))
> - goto xdr_error;
> + XDR_ERROR();
> READ32(locku->lu_seqid);
> status = nfsd4_decode_stateid(argp, &locku->lu_stateid);
> if (status)
> @@ -742,15 +740,15 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> status = nfsd4_decode_share_access(argp, &open->op_share_access,
> &open->op_deleg_want, &dummy);
> if (status)
> - goto xdr_error;
> + XDR_ERROR();
> status = nfsd4_decode_share_deny(argp, &open->op_share_deny);
> if (status)
> - goto xdr_error;
> + XDR_ERROR();
> READ_BUF(sizeof(clientid_t));
> COPYMEM(&open->op_clientid, sizeof(clientid_t));
> status = nfsd4_decode_opaque(argp, &open->op_owner);
> if (status)
> - goto xdr_error;
> + XDR_ERROR();
> READ_BUF(4);
> READ32(open->op_create);
> switch (open->op_create) {
> @@ -773,7 +771,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> break;
> case NFS4_CREATE_EXCLUSIVE4_1:
> if (argp->minorversion < 1)
> - goto xdr_error;
> + XDR_ERROR();
> READ_BUF(8);
> COPYMEM(open->op_verf.data, 8);
> status = nfsd4_decode_fattr(argp, open->op_bmval,
> @@ -782,11 +780,11 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> goto out;
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
>
> /* open_claim */
> @@ -820,18 +818,18 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> case NFS4_OPEN_CLAIM_FH:
> case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
> if (argp->minorversion < 1)
> - goto xdr_error;
> + XDR_ERROR();
> /* void */
> break;
> case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
> if (argp->minorversion < 1)
> - goto xdr_error;
> + XDR_ERROR();
> status = nfsd4_decode_stateid(argp, &open->op_delegate_stateid);
> if (status)
> return status;
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
>
> DECODE_TAIL;
> @@ -879,7 +877,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ_BUF(4);
> READ32(putfh->pf_fhlen);
> if (putfh->pf_fhlen > NFS4_FHSIZE)
> - goto xdr_error;
> + XDR_ERROR();
> READ_BUF(putfh->pf_fhlen);
> SAVEMEM(putfh->pf_fhval, putfh->pf_fhlen);
>
> @@ -1093,7 +1091,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ64(write->wr_offset);
> READ32(write->wr_stable_how);
> if (write->wr_stable_how > 2)
> - goto xdr_error;
> + XDR_ERROR();
> READ32(write->wr_buflen);
>
> /* Sorry .. no magic macros for this.. *
> @@ -1104,7 +1102,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> if (avail + argp->pagelen < write->wr_buflen) {
> dprintk("NFSD: xdr error (%s:%d)\n",
> __FILE__, __LINE__);
> - goto xdr_error;
> + XDR_ERROR();
> }
> argp->rqstp->rq_vec[0].iov_base = p;
> argp->rqstp->rq_vec[0].iov_len = avail;
> @@ -1221,7 +1219,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ32(dummy);
> break;
> default:
> - goto xdr_error;
> + XDR_ERROR();
> }
>
> /* Ignore Implementation ID */
> @@ -1229,7 +1227,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ32(dummy);
>
> if (dummy > 1)
> - goto xdr_error;
> + XDR_ERROR();
>
> if (dummy == 1) {
> /* nii_domain */
> @@ -1281,7 +1279,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ32(sess->fore_channel.rdma_attrs);
> } else if (sess->fore_channel.nr_rdma_attrs > 1) {
> dprintk("Too many fore channel attr bitmaps!\n");
> - goto xdr_error;
> + XDR_ERROR();
> }
>
> /* Back channel attrs */
> @@ -1298,7 +1296,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
> READ32(sess->back_channel.rdma_attrs);
> } else if (sess->back_channel.nr_rdma_attrs > 1) {
> dprintk("Too many back channel attr bitmaps!\n");
> - goto xdr_error;
> + XDR_ERROR();
> }
>
> READ_BUF(8);
> @@ -1410,7 +1408,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>
> nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
> if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
> - goto xdr_error;
> + XDR_ERROR();
>
> test_stateid->ts_saved_args = argp;
> save_buf(argp, &test_stateid->ts_savedp);
> @@ -1588,16 +1586,16 @@ struct nfsd4_minorversion_ops {
> READ32(argp->opcnt);
>
> if (argp->taglen > NFSD4_MAX_TAGLEN)
> - goto xdr_error;
> + XDR_ERROR();
> if (argp->opcnt > 100)
> - goto xdr_error;
> + XDR_ERROR();
>
> if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
> argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL);
> if (!argp->ops) {
> argp->ops = argp->iops;
> dprintk("nfsd: couldn't allocate room for COMPOUND\n");
> - goto xdr_error;
> + XDR_ERROR();
> }
> }
>
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2011-10-28 12:09:28

by Benny Halevy

[permalink] [raw]
Subject: Re: [RFC] nfsd4: DEBUG: XDR_ERROR()

On 2011-10-28 13:34, J. Bruce Fields wrote:
> Did I just get moved to Netapp without anyone telling me? Anyway:

I'm so sorry, I guess Red Hat rhymes with Net App, sort of... :-)

>
> On Thu, Oct 27, 2011 at 08:49:11PM +0200, Benny Halevy wrote:
>> From: Benny Halevy <[email protected]>
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>>
>> Bruce, would you consider this patch to help
>> debug xdr parsing errors by dprinting the line where the
>> xdr error detected in a more relevant place?
>
> I don't object to the goal, but:
>
> - I'd like to make this code less dependent on the preprocessor
> rather than more.
>
> - In particular I don't like having goto's and goto labels
> hidden in macros--I think they make the control flow less
> obvious.
>
> - There have been exceptions (mea culpa!), but usually I think
> that we should be able to catch xdr decoding problems early,
> so having this debugging out in the field isn't as important
> as debugging elsewhere.

I agree with all of the above but its a project larger than I can
chew right now. This patch just improves the existing mechanism, for
development use, to help catch parser bugs rather than actual xdr errors.

Benny

>
> --b.
>
>>
>> Benny
>>
>> fs/nfsd/nfs4xdr.c | 76 +++++++++++++++++++++++++---------------------------
>> 1 files changed, 37 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 2f894c6..d5fd7d0 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -79,6 +79,12 @@
>> return 0;
>> }
>>
>> +#define XDR_ERROR() do { \
>> + dprintk("NFSD: xdr error (%s:%d)\n", \
>> + __FILE__, __LINE__); \
>> + goto xdr_error; \
>> +} while (0)
>> +
>> #define DECODE_HEAD \
>> __be32 *p; \
>> __be32 status
>> @@ -87,8 +93,6 @@
>> out: \
>> return status; \
>> xdr_error: \
>> - dprintk("NFSD: xdr error (%s:%d)\n", \
>> - __FILE__, __LINE__); \
>> status = nfserr_bad_xdr; \
>> goto out
>>
>> @@ -109,11 +113,8 @@
>> #define SAVEMEM(x,nbytes) do { \
>> if (!(x = (p==argp->tmp || p == argp->tmpp) ? \
>> savemem(argp, p, nbytes) : \
>> - (char *)p)) { \
>> - dprintk("NFSD: xdr error (%s:%d)\n", \
>> - __FILE__, __LINE__); \
>> - goto xdr_error; \
>> - } \
>> + (char *)p)) \
>> + XDR_ERROR(); \
>> p += XDR_QUADLEN(nbytes); \
>> } while (0)
>> #define COPYMEM(x,nbytes) do { \
>> @@ -126,11 +127,8 @@
>> if (nbytes <= (u32)((char *)argp->end - (char *)argp->p)) { \
>> p = argp->p; \
>> argp->p += XDR_QUADLEN(nbytes); \
>> - } else if (!(p = read_buf(argp, nbytes))) { \
>> - dprintk("NFSD: xdr error (%s:%d)\n", \
>> - __FILE__, __LINE__); \
>> - goto xdr_error; \
>> - } \
>> + } else if (!(p = read_buf(argp, nbytes))) \
>> + XDR_ERROR(); \
>> } while (0)
>>
>> static void save_buf(struct nfsd4_compoundargs *argp, struct nfsd4_saved_compoundargs *savep)
>> @@ -243,7 +241,7 @@ static int zero_clientid(clientid_t *clid)
>> READ_BUF(4);
>> READ32(bmlen);
>> if (bmlen > 1000)
>> - goto xdr_error;
>> + XDR_ERROR();
>>
>> READ_BUF(bmlen << 2);
>> if (bmlen > 0)
>> @@ -373,7 +371,7 @@ static int zero_clientid(clientid_t *clid)
>> iattr->ia_valid |= ATTR_ATIME;
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>> }
>> if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
>> @@ -399,7 +397,7 @@ static int zero_clientid(clientid_t *clid)
>> iattr->ia_valid |= ATTR_MTIME;
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>> }
>> if (bmval[0] & ~NFSD_WRITEABLE_ATTRS_WORD0
>> @@ -407,7 +405,7 @@ static int zero_clientid(clientid_t *clid)
>> || bmval[2] & ~NFSD_WRITEABLE_ATTRS_WORD2)
>> READ_BUF(expected_len - len);
>> else if (len != expected_len)
>> - goto xdr_error;
>> + XDR_ERROR();
>>
>> DECODE_TAIL;
>>
>> @@ -556,7 +554,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
>> READ_BUF(28);
>> READ32(lock->lk_type);
>> if ((lock->lk_type < NFS4_READ_LT) || (lock->lk_type > NFS4_WRITEW_LT))
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ32(lock->lk_reclaim);
>> READ64(lock->lk_offset);
>> READ64(lock->lk_length);
>> @@ -593,7 +591,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
>> READ_BUF(32);
>> READ32(lockt->lt_type);
>> if((lockt->lt_type < NFS4_READ_LT) || (lockt->lt_type > NFS4_WRITEW_LT))
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ64(lockt->lt_offset);
>> READ64(lockt->lt_length);
>> COPYMEM(&lockt->lt_clientid, 8);
>> @@ -612,7 +610,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
>> READ_BUF(8);
>> READ32(locku->lu_type);
>> if ((locku->lu_type < NFS4_READ_LT) || (locku->lu_type > NFS4_WRITEW_LT))
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ32(locku->lu_seqid);
>> status = nfsd4_decode_stateid(argp, &locku->lu_stateid);
>> if (status)
>> @@ -742,15 +740,15 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> status = nfsd4_decode_share_access(argp, &open->op_share_access,
>> &open->op_deleg_want, &dummy);
>> if (status)
>> - goto xdr_error;
>> + XDR_ERROR();
>> status = nfsd4_decode_share_deny(argp, &open->op_share_deny);
>> if (status)
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ_BUF(sizeof(clientid_t));
>> COPYMEM(&open->op_clientid, sizeof(clientid_t));
>> status = nfsd4_decode_opaque(argp, &open->op_owner);
>> if (status)
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ_BUF(4);
>> READ32(open->op_create);
>> switch (open->op_create) {
>> @@ -773,7 +771,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> break;
>> case NFS4_CREATE_EXCLUSIVE4_1:
>> if (argp->minorversion < 1)
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ_BUF(8);
>> COPYMEM(open->op_verf.data, 8);
>> status = nfsd4_decode_fattr(argp, open->op_bmval,
>> @@ -782,11 +780,11 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> goto out;
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>>
>> /* open_claim */
>> @@ -820,18 +818,18 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> case NFS4_OPEN_CLAIM_FH:
>> case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
>> if (argp->minorversion < 1)
>> - goto xdr_error;
>> + XDR_ERROR();
>> /* void */
>> break;
>> case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
>> if (argp->minorversion < 1)
>> - goto xdr_error;
>> + XDR_ERROR();
>> status = nfsd4_decode_stateid(argp, &open->op_delegate_stateid);
>> if (status)
>> return status;
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>>
>> DECODE_TAIL;
>> @@ -879,7 +877,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ_BUF(4);
>> READ32(putfh->pf_fhlen);
>> if (putfh->pf_fhlen > NFS4_FHSIZE)
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ_BUF(putfh->pf_fhlen);
>> SAVEMEM(putfh->pf_fhval, putfh->pf_fhlen);
>>
>> @@ -1093,7 +1091,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ64(write->wr_offset);
>> READ32(write->wr_stable_how);
>> if (write->wr_stable_how > 2)
>> - goto xdr_error;
>> + XDR_ERROR();
>> READ32(write->wr_buflen);
>>
>> /* Sorry .. no magic macros for this.. *
>> @@ -1104,7 +1102,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> if (avail + argp->pagelen < write->wr_buflen) {
>> dprintk("NFSD: xdr error (%s:%d)\n",
>> __FILE__, __LINE__);
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>> argp->rqstp->rq_vec[0].iov_base = p;
>> argp->rqstp->rq_vec[0].iov_len = avail;
>> @@ -1221,7 +1219,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ32(dummy);
>> break;
>> default:
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>>
>> /* Ignore Implementation ID */
>> @@ -1229,7 +1227,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ32(dummy);
>>
>> if (dummy > 1)
>> - goto xdr_error;
>> + XDR_ERROR();
>>
>> if (dummy == 1) {
>> /* nii_domain */
>> @@ -1281,7 +1279,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ32(sess->fore_channel.rdma_attrs);
>> } else if (sess->fore_channel.nr_rdma_attrs > 1) {
>> dprintk("Too many fore channel attr bitmaps!\n");
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>>
>> /* Back channel attrs */
>> @@ -1298,7 +1296,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>> READ32(sess->back_channel.rdma_attrs);
>> } else if (sess->back_channel.nr_rdma_attrs > 1) {
>> dprintk("Too many back channel attr bitmaps!\n");
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>>
>> READ_BUF(8);
>> @@ -1410,7 +1408,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
>>
>> nbytes = test_stateid->ts_num_ids * sizeof(stateid_t);
>> if (nbytes > (u32)((char *)argp->end - (char *)argp->p))
>> - goto xdr_error;
>> + XDR_ERROR();
>>
>> test_stateid->ts_saved_args = argp;
>> save_buf(argp, &test_stateid->ts_savedp);
>> @@ -1588,16 +1586,16 @@ struct nfsd4_minorversion_ops {
>> READ32(argp->opcnt);
>>
>> if (argp->taglen > NFSD4_MAX_TAGLEN)
>> - goto xdr_error;
>> + XDR_ERROR();
>> if (argp->opcnt > 100)
>> - goto xdr_error;
>> + XDR_ERROR();
>>
>> if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
>> argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL);
>> if (!argp->ops) {
>> argp->ops = argp->iops;
>> dprintk("nfsd: couldn't allocate room for COMPOUND\n");
>> - goto xdr_error;
>> + XDR_ERROR();
>> }
>> }
>>
>> --
>> 1.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html