fix nfsd stateid encoding
Subsystems (2)
cover fix nfsd stateid encoding
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a uint32_t rather than as opaque.
Patch #1 fixes that for cb_recall.
Patch #2 fixes the deleg stateid returned by open.
The patches should apply to linux-2.6/master
commit 796aadeb1b2db9b5d463946766c5bbfd7717158c
Benny
PATCH 1/2 nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall
--- fs/nfsd/nfs4callback.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
fs/nfsd/nfs4callback.c +2 -1
@@ -222,7 +222,8 @@ encode_cb_recall(struct xdr_stream *xdr, struct nfs4_cb_recall *cb_rec)RESERVE_SPACE (12 +sizeof (cb_rec->cbr_stateid) + len);WRITE32 (OP_CB_RECALL ); -WRITEMEM (&cb_rec->cbr_stateid,sizeof (stateid_t)); +WRITE32 (cb_rec->cbr_stateid.si_generation); +WRITEMEM (&cb_rec->cbr_stateid.si_opaque,sizeof (stateid_opaque_t));WRITE32 (cb_rec->cbr_trunc);WRITE32 (len);WRITEMEM (cb_rec->cbr_fhval, len);
Signature · 1 line
1.5.6.5
PATCH 2/2 nfsd: properly xdr-encode deleg stateid returned from open
In reply to Benny Halevy--- fs/nfsd/nfs4xdr.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +6 -2
@@ -2158,7 +2158,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opbreak ;case NFS4_OPEN_DELEGATE_READ :RESERVE_SPACE (20 +sizeof (stateid_t)); -WRITEMEM (&open->op_delegate_stateid,sizeof (stateid_t)); +WRITE32 (open->op_delegate_stateid.si_generation); +WRITEMEM (&open->op_delegate_stateid.si_opaque, +sizeof (stateid_opaque_t));WRITE32 (open->op_recall);/*
@@ -2172,7 +2174,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opbreak ;case NFS4_OPEN_DELEGATE_WRITE :RESERVE_SPACE (32 +sizeof (stateid_t)); -WRITEMEM (&open->op_delegate_stateid,sizeof (stateid_t)); +WRITE32 (open->op_delegate_stateid.si_generation); +WRITEMEM (&open->op_delegate_stateid.si_opaque, +sizeof (stateid_opaque_t));WRITE32 (0 );/*
Signature · 1 line
1.5.6.5
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.
Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
--b.
Quoted 8 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
Quoted 17 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
Quoted 6 lines
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called by nfsd4_delegreturn to fail. And eventually, unhash_delegation wouldn't be called.
Hence, I think these patches are appropriate for 2.6.27 and even to older stable releases.
Benny
Quoted 14 lines
--b.
Quoted 7 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
_______________________________________________
pNFS mailing list
[email protected]
http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
Hi Benny-
Quoted 25 lines · Benny Halevy
Quoted 17 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
Quoted 6 lines
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Quoted 6 lines
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
I wonder, since you found three separate places where this is needed:
should you construct a helper function? Certainly there are already missed opportunities for sharing XDR encoding and decoding between callback and the forward channel.
Quoted 25 lines
Hence, I think these patches are appropriate for 2.6.27 and even to older stable releases.
Benny
Quoted 6 lines
--b.
Quoted 3 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.Quoted 8 lines
Quoted 4 lines
The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
_______________________________________________
pNFS mailing list
[email protected]
http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs--
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
Signature · 2 lines
"Officer. Ma'am. Squeaker." -- Mr. Incredible
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808110917y5a9a1950l1d905f081bd7a819-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 33 lines · Chuck Lever
Hi Benny-
Quoted 21 lines · Benny Halevy
Quoted 20 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Quoted 8 lines
Quoted 2 lines
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Quoted 5 lines
Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
I wonder, since you found three separate places where this is needed:
Note, I looked at all other places we encode or decode stateids and they seem ok at first glance.
Quoted 3 lines
should you construct a helper function? Certainly there are already missed opportunities for sharing XDR encoding and decoding between callback and the forward channel.
That's fine with me. In fact, my first implementation did exactly that but when I saw that everywhere else the encoding of si_generation followed by 12 opaque bytes is done inline I reverted to keeping the current convention. If everybody is ok with having such helper functions I'll be happy to send patches implementing them.
The scope of _this_, however, is less urgent and can easily be done for 2.6.28.
Benny
Quoted 27 lines
Quoted 24 lines
Hence, I think these patches are appropriate for 2.6.27 and even to older stable releases.
Benny
Quoted 5 lines
--b.
Quoted 3 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.Quoted 8 lines
Quoted 4 lines
The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
_______________________________________________
pNFS mailing list
[email protected]
http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs--
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
Quoted 35 lines · Chuck Lever
Hi Benny-
Quoted 25 lines · Benny Halevy
Quoted 17 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
Quoted 6 lines
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Quoted 6 lines
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Quoted 6 lines
Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
I wonder, since you found three separate places where this is needed:
should you construct a helper function?
A stateid encoder/decoder? Sure, that could be a good idea.
--b.
Quoted 3 lines
Certainly there are already
missed opportunities for sharing XDR encoding and decoding between callback and the forward channel.
Quoted 33 lines · J. Bruce Fields
Quoted 32 lines · Chuck Lever
Hi Benny-
Quoted 29 lines · Benny Halevy
Quoted 16 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
Quoted 6 lines
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Quoted 8 lines
Quoted 6 lines
Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
I wonder, since you found three separate places where this is needed:
should you construct a helper function?A stateid encoder/decoder? Sure, that could be a good idea.
Cool. Would you like me to rework the patches I've already sent or send a patch adding the helpers on top of them?
Benny
Quoted 6 lines
--b.
Quoted 3 lines
Certainly there are already
missed opportunities for sharing XDR encoding and decoding between callback and the forward channel.
Quoted 38 lines · Benny Halevy
Quoted 37 lines · J. Bruce Fields
Quoted 36 lines · Chuck Lever
Hi Benny-
Quoted 21 lines · Benny Halevy
Quoted 20 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Quoted 8 lines
Quoted 2 lines
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I wonder, since you found three separate places where this is needed:
should you construct a helper function?A stateid encoder/decoder? Sure, that could be a good idea.
Cool. Would you like me to rework the patches I've already sent or send a patch adding the helpers on top of them?
I'm fine with whichever you think makes the most sense.
--b.
Quoted 41 lines · J. Bruce Fields
Quoted 40 lines · Benny Halevy
Quoted 39 lines · J. Bruce Fields
Quoted 38 lines · Chuck Lever
Hi Benny-
Quoted 21 lines · Benny Halevy
Quoted 20 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Quoted 8 lines
Quoted 2 lines
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.Sounds plausible, good catch. Yet another reason we should have an easy-to-access delegation counter metric on both the client and server.
I wonder, since you found three separate places where this is needed:
should you construct a helper function?
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
A stateid encoder/decoder? Sure, that could be a good idea.
Cool. Would you like me to rework the patches I've already sent or send a patch adding the helpers on top of them?
I'm fine with whichever you think makes the most sense.
--b.
OK. It's better to separate the fixes from the refactoring of code. The 7-patch patchset in reply to this message does the following:
[PATCH 1/7] nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall
encoding fix for cb_recall.
There's no helper defined since there's only one use. I've defined a helper and used in the nfs41 series since there, there are 2 (cb_recall and cb_layoutrecall).
[PATCH 2/7] nfsd: properly xdr-encode deleg stateid returned from open [PATCH 3/7] nfsd: fix nfsd4_encode_open buffer space reservation
xdr encoding fixes
[PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function [PATCH 5/7] nfsd: don't declare p in ENCODE_SEQID_OP_HEAD
xdr encode_stateid refactoring
[PATCH 6/7] nfsd: properly xdr-decode NFS4_OPEN_CLAIM_DELEGATE_CUR stateid
xdr decoding fix
[PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
xdr decode_stateid refactoring
Benny
PATCH 1/7 nfsd: properly xdr-encode stateid4.seqid as uint32_t for cb_recall
--- fs/nfsd/nfs4callback.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
fs/nfsd/nfs4callback.c +2 -1
@@ -222,7 +222,8 @@ encode_cb_recall(struct xdr_stream *xdr, struct nfs4_cb_recall *cb_rec)RESERVE_SPACE (12 +sizeof (cb_rec->cbr_stateid) + len);WRITE32 (OP_CB_RECALL ); -WRITEMEM (&cb_rec->cbr_stateid,sizeof (stateid_t)); +WRITE32 (cb_rec->cbr_stateid.si_generation); +WRITEMEM (&cb_rec->cbr_stateid.si_opaque,sizeof (stateid_opaque_t));WRITE32 (cb_rec->cbr_trunc);WRITE32 (len);WRITEMEM (cb_rec->cbr_fhval, len);
Signature · 1 line
1.5.6.5
PATCH 2/7 nfsd: properly xdr-encode deleg stateid returned from open
In reply to Benny Halevy--- fs/nfsd/nfs4xdr.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +6 -2
@@ -2158,7 +2158,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opbreak ;case NFS4_OPEN_DELEGATE_READ :RESERVE_SPACE (20 +sizeof (stateid_t)); -WRITEMEM (&open->op_delegate_stateid,sizeof (stateid_t)); +WRITE32 (open->op_delegate_stateid.si_generation); +WRITEMEM (&open->op_delegate_stateid.si_opaque, +sizeof (stateid_opaque_t));WRITE32 (open->op_recall);/*
@@ -2172,7 +2174,9 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opbreak ;case NFS4_OPEN_DELEGATE_WRITE :RESERVE_SPACE (32 +sizeof (stateid_t)); -WRITEMEM (&open->op_delegate_stateid,sizeof (stateid_t)); +WRITE32 (open->op_delegate_stateid.si_generation); +WRITEMEM (&open->op_delegate_stateid.si_opaque, +sizeof (stateid_opaque_t));WRITE32 (0 );/*
Signature · 1 line
1.5.6.5
PATCH 3/7 nfsd: fix nfsd4_encode_open buffer space reservation
In reply to Benny Halevynfsd4_encode_open first reservation is currently for 36 + sizeof(stateid_t) while it writes after the stateid a cinfo (20 bytes) and 5 more 4-bytes words, for a total of 40 + sizeof(stateid_t).
--- fs/nfsd/nfs4xdr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfsd/nfs4xdr.c +1 -1
@@ -2142,7 +2142,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opif (nfserr) goto out; -RESERVE_SPACE (36 +sizeof (stateid_t)); +RESERVE_SPACE (40 +sizeof (stateid_t));WRITE32 (open->op_stateid.si_generation);WRITEMEM (&open->op_stateid.si_opaque,sizeof (stateid_opaque_t));WRITECINFO (open->op_cinfo);
Signature · 1 line
1.5.6.5
Re: [PATCH 3/7] nfsd: fix nfsd4_encode_open buffer space reservation
Quoted 3 lines · Benny Halevy
nfsd4_encode_openfirst reservation is currently for 36 +sizeof(stateid_t)while it writes after the stateid a cinfo (20 bytes) and 5 more 4-bytes words, for a total of 40 +sizeof(stateid_t).
Thanks, yep. I wish this was all a little more automated!
--b.
Quoted 22 lines
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)fs/nfsd/nfs4xdr.c +1
-1@@ -2142,7 +2142,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opif (nfserr) goto out; -RESERVE_SPACE (36 +sizeof (stateid_t)); +RESERVE_SPACE (40 +sizeof (stateid_t));WRITE32 (open->op_stateid.si_generation);WRITEMEM (&open->op_stateid.si_opaque,sizeof (stateid_opaque_t));WRITECINFO (open->op_cinfo);Signature · 1 line
1.5.6.5
PATCH 4/7 nfsd: nfs4xdr encode_stateid helper function
In reply to Benny Halevy--- fs/nfsd/nfs4xdr.c | 76 ++++++++++++++++++++++------------------------------ 1 files changed, 32 insertions(+), 44 deletions(-)
fs/nfsd/nfs4xdr.c +32 -44
@@ -1959,6 +1959,17 @@ fail:return -EINVAL ; } +static void +nfsd4_encode_stateid (struct nfsd4_compoundres *resp,stateid_t *sid) +{ +ENCODE_HEAD ; + +RESERVE_SPACE (sizeof (stateid_t)); +WRITE32 (sid->si_generation); +WRITEMEM (&sid->si_opaque,sizeof (stateid_opaque_t)); +ADJUST_ARGS (); +} +static __be32 nfsd4_encode_access (struct nfsd4_compoundres *resp,__be32 nfserr,struct nfsd4_access *access) {
@@ -1978,12 +1989,9 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c {ENCODE_SEQID_OP_HEAD ; -if (!nfserr) { -RESERVE_SPACE (sizeof (stateid_t)); -WRITE32 (close->cl_stateid.si_generation); -WRITEMEM (&close->cl_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS (); - } +if (!nfserr) +nfsd4_encode_stateid (resp, &close->cl_stateid); +ENCODE_SEQID_OP_TAIL (close->cl_stateowner);return nfserr; }
@@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo {ENCODE_SEQID_OP_HEAD ; -if (!nfserr) { -RESERVE_SPACE (4 +sizeof (stateid_t)); -WRITE32 (lock->lk_resp_stateid.si_generation); -WRITEMEM (&lock->lk_resp_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS (); - }else if (nfserr == nfserr_denied) +if (!nfserr) +nfsd4_encode_stateid (resp, &lock->lk_resp_stateid); +else if (nfserr == nfserr_denied)nfsd4_encode_lock_denied (resp, &lock->lk_denied);ENCODE_SEQID_OP_TAIL (lock->lk_replay_owner);
@@ -2108,13 +2113,9 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l {ENCODE_SEQID_OP_HEAD ; -if (!nfserr) { -RESERVE_SPACE (sizeof (stateid_t)); -WRITE32 (locku->lu_stateid.si_generation); -WRITEMEM (&locku->lu_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS (); - } - +if (!nfserr) +nfsd4_encode_stateid (resp, &locku->lu_stateid); +ENCODE_SEQID_OP_TAIL (locku->lu_stateowner);return nfserr; }
@@ -2142,9 +2143,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opif (nfserr) goto out; -RESERVE_SPACE (40 +sizeof (stateid_t)); -WRITE32 (open->op_stateid.si_generation); -WRITEMEM (&open->op_stateid.si_opaque,sizeof (stateid_opaque_t)); +nfsd4_encode_stateid (resp, &open->op_stateid); +RESERVE_SPACE (40 );WRITECINFO (open->op_cinfo);WRITE32 (open->op_rflags);WRITE32 (2 );
@@ -2157,10 +2157,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opcase NFS4_OPEN_DELEGATE_NONE :break ;case NFS4_OPEN_DELEGATE_READ : -RESERVE_SPACE (20 +sizeof (stateid_t)); -WRITE32 (open->op_delegate_stateid.si_generation); -WRITEMEM (&open->op_delegate_stateid.si_opaque, -sizeof (stateid_opaque_t)); +nfsd4_encode_stateid (resp, &open->op_delegate_stateid); +RESERVE_SPACE (20 );WRITE32 (open->op_recall);/*
@@ -2173,10 +2171,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_opADJUST_ARGS ();break ;case NFS4_OPEN_DELEGATE_WRITE : -RESERVE_SPACE (32 +sizeof (stateid_t)); -WRITE32 (open->op_delegate_stateid.si_generation); -WRITEMEM (&open->op_delegate_stateid.si_opaque, -sizeof (stateid_opaque_t)); +nfsd4_encode_stateid (resp, &open->op_delegate_stateid); +RESERVE_SPACE (32 );WRITE32 (0 );/*
@@ -2208,13 +2204,9 @@ static __be32 nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_confirm *oc) {ENCODE_SEQID_OP_HEAD ; - -if (!nfserr) { -RESERVE_SPACE (sizeof (stateid_t)); -WRITE32 (oc->oc_resp_stateid.si_generation); -WRITEMEM (&oc->oc_resp_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS (); - } + +if (!nfserr) +nfsd4_encode_stateid (resp, &oc->oc_resp_stateid);ENCODE_SEQID_OP_TAIL (oc->oc_stateowner);return nfserr;
@@ -2224,13 +2216,9 @@ static __be32 nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_downgrade *od) {ENCODE_SEQID_OP_HEAD ; - -if (!nfserr) { -RESERVE_SPACE (sizeof (stateid_t)); -WRITE32 (od->od_stateid.si_generation); -WRITEMEM (&od->od_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS (); - } + +if (!nfserr) +nfsd4_encode_stateid (resp, &od->od_stateid);ENCODE_SEQID_OP_TAIL (od->od_stateowner);return nfserr;
Signature · 1 line
1.5.6.5
Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function
Quoted 9 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 76 ++++++++++++++++++++++------------------------------ 1 files changed, 32 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0
...
fs/nfsd/nfs4xdr.c nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo · line 2087
Quoted 6 lines
fs/nfsd/nfs4xdr.c +0
-2@@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo {ENCODE_SEQID_OP_HEAD ; -if (!nfserr) { -RESERVE_SPACE (4 +sizeof (stateid_t));
Hm, so that was a little over before (a harmless error, but better to have it fixed).
--b.
Quoted 3 lines
fs/nfsd/nfs4xdr.c +0
-3-WRITE32 (lock->lk_resp_stateid.si_generation); -WRITEMEM (&lock->lk_resp_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS ();
Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function
Quoted 15 lines · J. Bruce Fields
Quoted 9 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 76 ++++++++++++++++++++++------------------------------ 1 files changed, 32 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0...
Quoted 4 lines
fs/nfsd/nfs4xdr.c +0
-0@@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo {ENCODE_SEQID_OP_HEAD ;
Quoted 5 lines
fs/nfsd/nfs4xdr.c nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo · line 2087
Quoted 2 lines
fs/nfsd/nfs4xdr.c +0
-2-if (!nfserr) { -RESERVE_SPACE (4 +sizeof (stateid_t));Hm, so that was a little over before (a harmless error, but better to have it fixed).
True. I can add a preliminary patch fixing that.
Benny
Quoted 6 lines
--b.
Quoted 3 lines
fs/nfsd/nfs4xdr.c +0
-3-WRITE32 (lock->lk_resp_stateid.si_generation); -WRITEMEM (&lock->lk_resp_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS ();
Re: [PATCH 4/7] nfsd: nfs4xdr encode_stateid helper function
Quoted 16 lines · Benny Halevy
Quoted 15 lines · J. Bruce Fields
Quoted 9 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 76 ++++++++++++++++++++++------------------------------ 1 files changed, 32 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0...
Quoted 4 lines
fs/nfsd/nfs4xdr.c +0
-0@@ -2083,12 +2091,9 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo {ENCODE_SEQID_OP_HEAD ;
Quoted 7 lines
Quoted 5 lines
fs/nfsd/nfs4xdr.c nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo · line 2087
Quoted 2 lines
fs/nfsd/nfs4xdr.c +0
-2-if (!nfserr) { -RESERVE_SPACE (4 +sizeof (stateid_t));Hm, so that was a little over before (a harmless error, but better to have it fixed).
True. I can add a preliminary patch fixing that.
Eh, it's minor. (In future, yes, I'd prefer things split out that way, but I don't think it's worth redoing for something this small.)
--b.
Quoted 9 lines
Benny
Quoted 6 lines
--b.
Quoted 3 lines
fs/nfsd/nfs4xdr.c +0
-3-WRITE32 (lock->lk_resp_stateid.si_generation); -WRITEMEM (&lock->lk_resp_stateid.si_opaque,sizeof (stateid_opaque_t)); -ADJUST_ARGS ();
PATCH 5/7 nfsd: don't declare p in ENCODE_SEQID_OP_HEAD
In reply to Benny HalevyAfter using the encode_stateid helper the "p" pointer declared by ENCODE_SEQID_OP_HEAD is warned as unused.
In the single site where it is still needed it can be declared separately using the ENCODE_HEAD macro.
--- fs/nfsd/nfs4xdr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfsd/nfs4xdr.c +1 -1
@@ -1192,7 +1192,6 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)* Header routine to setup seqid operation replay cache */ #define ENCODE_SEQID_OP_HEAD \ - __be32 *p; \ __be32 *save; \ \ save = resp->p;
@@ -2138,6 +2137,7 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_listatic __be32 nfsd4_encode_open (struct nfsd4_compoundres *resp,__be32 nfserr,struct nfsd4_open *open) { +ENCODE_HEAD ;ENCODE_SEQID_OP_HEAD ;if (nfserr)
Signature · 1 line
1.5.6.5
PATCH 6/7 nfsd: properly xdr-decode NFS4_OPEN_CLAIM_DELEGATE_CUR stateid
In reply to Benny Halevy--- fs/nfsd/nfs4xdr.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
fs/nfsd/nfs4xdr.c +3 -1
@@ -679,7 +679,9 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)break ;case NFS4_OPEN_CLAIM_DELEGATE_CUR :READ_BUF (sizeof (stateid_t) +4 ); -COPYMEM (&open->op_delegate_stateid,sizeof (stateid_t)); +READ32 (open->op_delegate_stateid.si_generation); +COPYMEM (&open->op_delegate_stateid.si_opaque, +sizeof (stateid_opaque_t));READ32 (open->op_fname.len);READ_BUF (open->op_fname.len);SAVEMEM (open->op_fname.data, open->op_fname.len);
Signature · 1 line
1.5.6.5
PATCH 7/7 nfsd: nfs4xdr decode_stateid helper function
In reply to Benny Halevy--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)
fs/nfsd/nfs4xdr.c +55 -44
@@ -413,6 +413,18 @@ out_nfserr: }static __be32 +nfsd4_decode_stateid (struct nfsd4_compoundargs *argp,stateid_t *sid) +{ +DECODE_HEAD ; + +READ_BUF (sizeof (stateid_t)); +READ32 (sid->si_generation); +COPYMEM (&sid->si_opaque,sizeof (stateid_opaque_t)); + +DECODE_TAIL ; +} + +static __be32 nfsd4_decode_access (struct nfsd4_compoundargs *argp,struct nfsd4_access *access) {DECODE_HEAD ;
@@ -429,10 +441,9 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)DECODE_HEAD ; close->cl_stateowner =NULL ; -READ_BUF (4 +sizeof (stateid_t)); +READ_BUF (4 );READ32 (close->cl_seqid); -READ32 (close->cl_stateid.si_generation); -COPYMEM (&close->cl_stateid.si_opaque,sizeof (stateid_opaque_t)); +return nfsd4_decode_stateid (argp, &close->cl_stateid);DECODE_TAIL ; }
@@ -493,13 +504,7 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *createstatic inline __be32 nfsd4_decode_delegreturn (struct nfsd4_compoundargs *argp,struct nfsd4_delegreturn *dr) { -DECODE_HEAD ; - -READ_BUF (sizeof (stateid_t)); -READ32 (dr->dr_stateid.si_generation); -COPYMEM (&dr->dr_stateid.si_opaque,sizeof (stateid_opaque_t)); - -DECODE_TAIL ; +return nfsd4_decode_stateid (argp, &dr->dr_stateid); } static inline __be32
@@ -542,20 +547,22 @@ nfsd4_decode_lock(struct nfsd4_compoundargs *argp, struct nfsd4_lock *lock)READ32 (lock->lk_is_new);if (lock->lk_is_new) { -READ_BUF (36 ); +READ_BUF (4 );READ32 (lock->lk_new_open_seqid); -READ32 (lock->lk_new_open_stateid.si_generation); - -COPYMEM (&lock->lk_new_open_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &lock->lk_new_open_stateid); +if (status) +return status; +READ_BUF (8 +sizeof (clientid_t));READ32 (lock->lk_new_lock_seqid);COPYMEM (&lock->lk_new_clientid,sizeof (clientid_t));READ32 (lock->lk_new_owner.len);READ_BUF (lock->lk_new_owner.len);READMEM (lock->lk_new_owner.data, lock->lk_new_owner.len); }else { -READ_BUF (20 ); -READ32 (lock->lk_old_lock_stateid.si_generation); -COPYMEM (&lock->lk_old_lock_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &lock->lk_old_lock_stateid); +if (status) +return status; +READ_BUF (4 );READ32 (lock->lk_old_lock_seqid); }
@@ -587,13 +594,15 @@ nfsd4_decode_locku(struct nfsd4_compoundargs *argp, struct nfsd4_locku *locku)DECODE_HEAD ; locku->lu_stateowner =NULL ; -READ_BUF (24 +sizeof (stateid_t)); +READ_BUF (8 );READ32 (locku->lu_type);if ((locku->lu_type <NFS4_READ_LT ) || (locku->lu_type >NFS4_WRITEW_LT )) goto xdr_error;READ32 (locku->lu_seqid); -READ32 (locku->lu_stateid.si_generation); -COPYMEM (&locku->lu_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &locku->lu_stateid); +if (status) +return status; +READ_BUF (16 );READ64 (locku->lu_offset);READ64 (locku->lu_length);
@@ -678,10 +687,10 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)READ32 (open->op_delegate_type);break ;case NFS4_OPEN_CLAIM_DELEGATE_CUR : -READ_BUF (sizeof (stateid_t) +4 ); -READ32 (open->op_delegate_stateid.si_generation); -COPYMEM (&open->op_delegate_stateid.si_opaque, -sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &open->op_delegate_stateid); +if (status) +return status; +READ_BUF (4 );READ32 (open->op_fname.len);READ_BUF (open->op_fname.len);SAVEMEM (open->op_fname.data, open->op_fname.len);
@@ -701,9 +710,10 @@ nfsd4_decode_open_confirm(struct nfsd4_compoundargs *argp, struct nfsd4_open_conDECODE_HEAD ; open_conf->oc_stateowner =NULL ; -READ_BUF (4 +sizeof (stateid_t)); -READ32 (open_conf->oc_req_stateid.si_generation); -COPYMEM (&open_conf->oc_req_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &open_conf->oc_req_stateid); +if (status) +return status; +READ_BUF (4 );READ32 (open_conf->oc_seqid);DECODE_TAIL ;
@@ -715,9 +725,10 @@ nfsd4_decode_open_downgrade(struct nfsd4_compoundargs *argp, struct nfsd4_open_dDECODE_HEAD ; open_down->od_stateowner =NULL ; -READ_BUF (12 +sizeof (stateid_t)); -READ32 (open_down->od_stateid.si_generation); -COPYMEM (&open_down->od_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &open_down->od_stateid); +if (status) +return status; +READ_BUF (12 );READ32 (open_down->od_seqid);READ32 (open_down->od_share_access);READ32 (open_down->od_share_deny);
@@ -745,9 +756,10 @@ nfsd4_decode_read(struct nfsd4_compoundargs *argp, struct nfsd4_read *read) {DECODE_HEAD ; -READ_BUF (sizeof (stateid_t) +12 ); -READ32 (read->rd_stateid.si_generation); -COPYMEM (&read->rd_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &read->rd_stateid); +if (status) +return status; +READ_BUF (12 );READ64 (read->rd_offset);READ32 (read->rd_length);
@@ -836,15 +848,13 @@ nfsd4_decode_secinfo(struct nfsd4_compoundargs *argp,static __be32 nfsd4_decode_setattr (struct nfsd4_compoundargs *argp,struct nfsd4_setattr *setattr) { -DECODE_HEAD ; - -READ_BUF (sizeof (stateid_t)); -READ32 (setattr->sa_stateid.si_generation); -COPYMEM (&setattr->sa_stateid.si_opaque,sizeof (stateid_opaque_t)); -if ((status =nfsd4_decode_fattr (argp, setattr->sa_bmval, &setattr->sa_iattr, &setattr->sa_acl))) - goto out; +__be32 status; -DECODE_TAIL ; + status =nfsd4_decode_stateid (argp, &setattr->sa_stateid); +if (status) +return status; +return nfsd4_decode_fattr (argp, setattr->sa_bmval, + &setattr->sa_iattr, &setattr->sa_acl); } static __be32
@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );READ64 (write->wr_offset);READ32 (write->wr_stable_how);if (write->wr_stable_how >2 )
Signature · 1 line
1.5.6.5
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 11 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32
fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 10 lines
fs/nfsd/nfs4xdr.c +4
-3@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );
How did that 20 become a 16?
OK, I guess this is another case of a preexisting arithmetic error. Anywhere that error would have had immediate consequences, but here thanks to the hand-coded write-data decoding, the incorrect argp->p isn't used except to check that there's space for the incoming data. And previously that check was more pessimistic than necessary.
--b.
Quoted 6 lines
fs/nfsd/nfs4xdr.c +0
-0READ64 (write->wr_offset);READ32 (write->wr_stable_how);if (write->wr_stable_how >2 )Signature · 1 line
1.5.6.5
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 15 lines · J. Bruce Fields
Quoted 14 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ;
Quoted 9 lines
fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 7 lines
fs/nfsd/nfs4xdr.c +4
-3-READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
It was sizeof(stateid_opaque_t) + 20 == sizeof(stateid_t) - 4 + 20 == sizeof(stateid_t) + 16.
Quoted 6 lines
OK, I guess this is another case of a preexisting arithmetic error. Anywhere that error would have had immediate consequences, but here thanks to the hand-coded write-data decoding, the incorrect
argp->pisn't used except to check that there's space for the incoming data. And previously that check was more pessimistic than necessary.
Oddly enough, the hand coded calculation was fine, as shown above... Note that the remaining 16 bytes correspond to one READ64 and two READ32s.
Benny
Quoted 9 lines
--b.
Quoted 6 lines
fs/nfsd/nfs4xdr.c +0
-0READ64 (write->wr_offset);READ32 (write->wr_stable_how);if (write->wr_stable_how >2 )Signature · 1 line
1.5.6.5
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 26 lines · Benny Halevy
Quoted 15 lines · J. Bruce Fields
Quoted 14 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ;Quoted 9 lines
fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 7 lines
fs/nfsd/nfs4xdr.c +4
-3-READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
Quoted 2 lines
It was
sizeof(stateid_opaque_t)+ 20 ==sizeof(stateid_t)- 4 + 20 ==sizeof(stateid_t)+ 16.
Whoops! Even now I still fall into the stateid_opaque_t/stateid_t trap!
Quoted 9 lines
Quoted 5 lines
OK, I guess this is another case of a preexisting arithmetic error. Anywhere that error would have had immediate consequences, but here thanks to the hand-coded write-data decoding, the incorrect
argp->pisn't used except to check that there's space for the incoming data. And previously that check was more pessimistic than necessary.Oddly enough, the hand coded calculation was fine, as shown above... Note that the remaining 16 bytes correspond to one READ64 and two READ32s.
Got it.
--b.
Quoted 12 lines
Benny
Quoted 9 lines
--b.
Quoted 6 lines
fs/nfsd/nfs4xdr.c +0
-0READ64 (write->wr_offset);READ32 (write->wr_stable_how);if (write->wr_stable_how >2 )Signature · 1 line
1.5.6.5
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 27 lines · J. Bruce Fields
Quoted 26 lines · Benny Halevy
Quoted 24 lines · J. Bruce Fields
Quoted 11 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 10 lines
fs/nfsd/nfs4xdr.c +4
-3@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
Quoted 4 lines
Quoted 2 lines
It was
sizeof(stateid_opaque_t)+ 20 ==sizeof(stateid_t)- 4 + 20 ==sizeof(stateid_t)+ 16.Whoops! Even now I still fall into the stateid_opaque_t/stateid_t trap!
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid {
__be32 generation;
char opaque[12];
} __attribute__((packed));Trond
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 33 lines · Trond Myklebust
Quoted 27 lines · J. Bruce Fields
Quoted 26 lines · Benny Halevy
Quoted 24 lines · J. Bruce Fields
Quoted 11 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 10 lines
fs/nfsd/nfs4xdr.c +4
-3@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
Quoted 4 lines
Quoted 2 lines
It was
sizeof(stateid_opaque_t)+ 20 ==sizeof(stateid_t)- 4 + 20 ==sizeof(stateid_t)+ 16.Whoops! Even now I still fall into the stateid_opaque_t/stateid_t trap!
Quoted 7 lines
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));
So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
Anyway, yes, I'd rather get rid of stateid_opaque_t.
--b.
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 11 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Cheers
Trond
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 13 lines · Trond Myklebust
Quoted 11 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
Quoted 6 lines
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Yeah, I was more curious about how to decide when it's necessary. (Why didn't we need it before? Is an embedded struct always aligned as if the fields of the embedded struct were declared directly in the containing struct? Or should we really just be using the packed attribute *any* time we depend on that alignment, even if it seems obvious the compiler wouldn't need to add padding?)
--b.
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 21 lines · J. Bruce Fields
Quoted 13 lines · Trond Myklebust
Quoted 11 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
Quoted 6 lines
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Quoted 6 lines
Yeah, I was more curious about how to decide when it's necessary. (Why didn't we need it before? Is an embedded struct always aligned as if the fields of the embedded struct were declared directly in the containing struct? Or should we really just be using the packed attribute *any* time we depend on that alignment, even if it seems obvious the compiler wouldn't need to add padding?)
The advantage of having it packed like the above is that you can still use WRITEMEM() to write out the whole structure in one fell swoop. If you don't specify 'packed', then the C standard allows the compiler to add padding between the fields in order align them. I'm not sure that compilers will usually do that for a 'char[]' field, but they will definitely for the integer types.
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 29 lines · Trond Myklebust
Quoted 21 lines · J. Bruce Fields
Quoted 13 lines · Trond Myklebust
Quoted 11 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
Quoted 6 lines
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Quoted 6 lines
Yeah, I was more curious about how to decide when it's necessary. (Why didn't we need it before? Is an embedded struct always aligned as if the fields of the embedded struct were declared directly in the containing struct? Or should we really just be using the packed attribute *any* time we depend on that alignment, even if it seems obvious the compiler wouldn't need to add padding?)
Quoted 2 lines
The advantage of having it packed like the above is that you can still use
WRITEMEM()to write out the whole structure in one fell swoop.
Right, I understand. But the code has been doing exactly that (a WRITEMEM of the whole thing) since the beginning, so I wondered if there was some reason you thought the switch to the extra char opaque[12] in particular was something that was likely to trigger the addition of padding.
Sounds instead like your policy would be just to declare any struct "packed" if we might depend on the absence of padding, without making any assumptions about what compilers might do. Which is fine.
--b.
Quoted 4 lines
If you don't specify 'packed', then the C standard allows the compiler to add padding between the fields in order align them. I'm not sure that compilers will usually do that for a 'char[]' field, but they will definitely for the integer types.
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 39 lines · J. Bruce Fields
Quoted 29 lines · Trond Myklebust
Quoted 21 lines · J. Bruce Fields
Quoted 13 lines · Trond Myklebust
Quoted 11 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
Quoted 6 lines
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Quoted 6 lines
Yeah, I was more curious about how to decide when it's necessary. (Why didn't we need it before? Is an embedded struct always aligned as if the fields of the embedded struct were declared directly in the containing struct? Or should we really just be using the packed attribute *any* time we depend on that alignment, even if it seems obvious the compiler wouldn't need to add padding?)
Quoted 2 lines
The advantage of having it packed like the above is that you can still use
WRITEMEM()to write out the whole structure in one fell swoop.Right, I understand. But the code has been doing exactly that (a WRITEMEM of the whole thing) since the beginning, so I wondered if there was some reason you thought the switch to the extra char opaque[12] in particular was something that was likely to trigger the addition of padding.
Quoted 10 lines
Sounds instead like your policy would be just to declare any struct "packed" if we might depend on the absence of padding, without making any assumptions about what compilers might do. Which is fine.
--b.
Quoted 4 lines
If you don't specify 'packed', then the C standard allows the compiler to add padding between the fields in order align them. I'm not sure that compilers will usually do that for a 'char[]' field, but they will definitely for the integer types.
If we go down this route, I suggest we add extra pre-processor checking to ensure that the sizeof() the structure is exactly what we expect. This would catch compiler bugs, platform-specific structure packing behaviors we didn't anticipate, and incorrect human assumptions. An added bonus might be using sizeof() such a structure to generate the maximum buffer size macros automatically.
I'm a little leary of using attribute(packed) structures, though.
Signature · 2 lines
"Officer. Ma'am. Squeaker." -- Mr. Incredible
Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
In reply to J. Bruce FieldsQuoted 20 lines · J. Bruce Fields
Quoted 19 lines · Trond Myklebust
Quoted 18 lines · J. Bruce Fields
Quoted 17 lines · Trond Myklebust
Quoted 10 lines · J. Bruce Fields
Quoted 7 lines · Trond Myklebust
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));So without the ((packed)), all arrays get aligned to 8-byte boundaries on 64-bit machines? (What do I need to read to catch up here?)
A quick google showed up:
http://sig9.com/articles/gcc-packed-structures
In any case, yes, the idea behind the packed attribute is to turn off the field alignment.
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Yeah, I was more curious about how to decide when it's necessary. (Why didn't we need it before? Is an embedded struct always aligned as if the fields of the embedded struct were declared directly in the containing struct? Or should we really just be using the packed attribute *any* time we depend on that alignment, even if it seems obvious the compiler wouldn't need to add padding?)
The advantage of having it packed like the above is that you can still use
WRITEMEM()to write out the whole structure in one fell swoop.
True, just you need to keep generation in network order in memory (hence Trond defined it as __be32...)
Quoted 10 lines
Right, I understand. But the code has been doing exactly that (a WRITEMEM of the whole thing) since the beginning, so I wondered if there was some reason you thought the switch to the extra char opaque[12] in particular was something that was likely to trigger the addition of padding.
Sounds instead like your policy would be just to declare any struct "packed" if we might depend on the absence of padding, without making any assumptions about what compilers might do. Which is fine.
Agreed. If you care about how the structure is laid out in memory then pack it.
Quoted 7 lines
--b.
Quoted 4 lines
If you don't specify 'packed', then the C standard allows the compiler to add padding between the fields in order align them. I'm not sure that compilers will usually do that for a 'char[]' field, but they will definitely for the integer types.
gcc (/c90) seems to align the field based on its size and arrays based on their element size.
Benny
Re: [pnfs] [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
In reply to Trond MyklebustQuoted 33 lines · Trond Myklebust
Quoted 25 lines · J. Bruce Fields
Quoted 24 lines · Benny Halevy
Quoted 23 lines · J. Bruce Fields
Quoted 11 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 10 lines
fs/nfsd/nfs4xdr.c +4
-3@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
Quoted 3 lines
Quoted 2 lines
It was
sizeof(stateid_opaque_t)+ 20 ==sizeof(stateid_t)- 4 + 20 ==sizeof(stateid_t)+ 16.Whoops! Even now I still fall into the stateid_opaque_t/stateid_t trap!
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
Quoted 7 lines
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));Trond
Please use the "__packed" Kernel macro for better compilers support.
And Yes I would suggest using "__packed" where ever a struct is defined for on-the-wire. Even just for indication to fellow programmers that this is going on the wire as is.
Boaz
Re: [pnfs] [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function
Quoted 39 lines · Boaz Harrosh
Quoted 38 lines · Trond Myklebust
Quoted 25 lines · J. Bruce Fields
Quoted 24 lines · Benny Halevy
Quoted 23 lines · J. Bruce Fields
Quoted 11 lines · Benny Halevy
Signed-off-by Benny Halevy <[email protected]>--- fs/nfsd/nfs4xdr.c | 99 +++++++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 44 deletions(-)fs/nfsd/nfs4xdr.c +0
-0static __be32fs/nfsd/nfs4xdr.c nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write) · line 945
Quoted 10 lines
fs/nfsd/nfs4xdr.c +4
-3@@ -929,9 +939,10 @@ nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)int len;DECODE_HEAD ; -READ_BUF (sizeof (stateid_opaque_t) +20 ); -READ32 (write->wr_stateid.si_generation); -COPYMEM (&write->wr_stateid.si_opaque,sizeof (stateid_opaque_t)); + status =nfsd4_decode_stateid (argp, &write->wr_stateid); +if (status) +return status; +READ_BUF (16 );How did that 20 become a 16?
Quoted 3 lines
Quoted 2 lines
It was
sizeof(stateid_opaque_t)+ 20 ==sizeof(stateid_t)- 4 + 20 ==sizeof(stateid_t)+ 16.Whoops! Even now I still fall into the stateid_opaque_t/stateid_t trap!
Which is a good reason for ditching the entire confusing typedef, and replacing it with a packed structure instead:
struct stateid { __be32 generation; char opaque[12]; } __attribute__((packed));
Quoted 8 lines
Quoted 2 lines
Trond
Please use the "
__packed" Kernel macro for better compilers support.And Yes I would suggest using "
__packed" where ever a struct is defined for on-the-wire. Even just for indication to fellow programmers that this is going on the wire as is.
OK. This all sounds reasonable, but I can't volunteer at the moment,
so: patches welcomed.
--b.
Quoted 22 lines · Benny Halevy
OK. It's better to separate the fixes from the refactoring of code. The 7-patch patchset in reply to this message does the following:
[PATCH 1/7] nfsd: properly xdr-encode stateid4.seqid as
uint32_tforcb_recallencoding fix for
cb_recall.There's no helper defined since there's only one use. I've defined a helper and used in the nfs41 series since there, there are 2 (
cb_recallandcb_layoutrecall).[PATCH 2/7] nfsd: properly xdr-encode deleg stateid returned from open [PATCH 3/7] nfsd: fix
nfsd4_encode_openbuffer space reservationxdr encoding fixes
[PATCH 4/7] nfsd: nfs4xdr
encode_stateidhelper function [PATCH 5/7] nfsd: don't declare p inENCODE_SEQID_OP_HEADxdr
encode_stateidrefactoring
Quoted 7 lines
[PATCH 6/7] nfsd: properly xdr-decode
NFS4_OPEN_CLAIM_DELEGATE_CURstateidxdr decoding fix
[PATCH 7/7] nfsd: nfs4xdr
decode_stateidhelper functionxdr
decode_stateidrefactoring
Thanks, all applied to
git://linux-nfs.org/~bfields/linux.git for-2.6.28
--b.
Quoted 25 lines · Benny Halevy
Quoted 17 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
Quoted 6 lines
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Quoted 6 lines
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Yipes. If delegations have always been broken and we haven't noticed, then there's a more serious problem here--we should at least look into why pynfs isn't catching that.
Oh, I see: si_generation is always zero for delegation stateid's!
Quoted 2 lines
Hence, I think these patches are appropriate for 2.6.27 and even to older stable releases.
So unless I've missed something, I think this still looks more like a style/consistency question?
--b.
Quoted 18 lines
Benny
Quoted 14 lines
--b.
Quoted 7 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
_______________________________________________
pNFS mailing list
[email protected]
http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
Quoted 31 lines · J. Bruce Fields
Quoted 21 lines · Benny Halevy
Quoted 20 lines · J. Bruce Fields
Quoted 2 lines · Benny Halevy
Bruce, in a couple locations the nfsd needs to encode the stateid.seqid as a
uint32_trather than as opaque.Agreed, thanks.
Though I have a hard time figuring out whether this has any impact in practice. Presumably the only change on the wire is that we'll get the endianness of the stateid4.seqid right? But that field is mostly opaque to the client anyway; 3530 says
The server is required to increment the seqid field monotonically at each transition of the stateid. This is important since the client will inspect the seqid in OPEN stateids to determine the order of OPEN processing done by the server.
but doesn't say why this is important. I'm sure this has been brought up on the ietf list before, but can't recall whether someone came up with a justification for the importance of this.
Quoted 8 lines
Quoted 2 lines
Anyway, so I figure these should be queued up for the next (2.6.28) merge window. Thanks!
Actually, I think this breaks delegreturn.
Since we decode the stateid.si_generation correctly, it will get swabbed in delegreturn on little-endian servers. This will cause nfs4_preprocess_stateid_op/check_stateid_generation as called bynfsd4_delegreturnto fail. And eventually,unhash_delegationwouldn't be called.
Quoted 5 lines
Yipes. If delegations have always been broken and we haven't noticed, then there's a more serious problem here--we should at least look into why pynfs isn't catching that.
Oh, I see:
si_generationis always zero for delegation stateid's!
Phew :-)
Quoted 6 lines
Quoted 3 lines
Hence, I think these patches are appropriate for 2.6.27 and even to older stable releases.
So unless I've missed something, I think this still looks more like a style/consistency question?
Yup.
Benny
Quoted 18 lines
--b.
Quoted 15 lines
Benny
Quoted 13 lines
--b.
Quoted 7 lines
Patch #1 fixes that for
cb_recall.
Patch #2 fixes the deleg stateid returned by open.The patches should apply to linux-2.6/master
commit796aadeb1b2db9b5d463946766c5bbfd7717158cBenny
_______________________________________________
pNFS mailing list
[email protected]
http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs