There are four macro defines for max tag length,
in fs/nfs/nfs4xdr.c,
/* NFSv4 COMPOUND tags are only wanted for debugging purposes */
#ifdef DEBUG
#define NFS4_MAXTAGLEN 20
#else
#define NFS4_MAXTAGLEN 0
#endif
in fs/nfs/callback_xdr.c,
#define CB_OP_TAGLEN_MAXSZ (512)
in fs/nfsd/xdr4.h,
#define NFSD4_MAX_TAGLEN 128
in fs/nfsd/xdr4cb.h,
#define NFS4_MAXTAGLEN 20
But, according to rfc3530 and rfc5661, all the length should be
limited by opaque limited.
The patch site defines a common macro named NFS4_MAXTAGLEN for
all of them, limited to opaque limited.
Kinglong Mee (4):
nfs: define NFS4_MAXTAGLEN to OPAQUE limits
nfs: use NFS4_MAXTAGLEN for cb_taglen checking
nfsd: use NFS4_MAXTAGLEN for nfsd taglen checking
nfsd: use NFS4_MAXTAGLEN defined in include/linux/nfs4.h
fs/nfs/callback_xdr.c | 5 ++---
fs/nfs/nfs4xdr.c | 7 -------
fs/nfsd/nfs4xdr.c | 2 +-
fs/nfsd/xdr4.h | 1 -
fs/nfsd/xdr4cb.h | 3 ++-
include/linux/nfs4.h | 1 +
6 files changed, 6 insertions(+), 13 deletions(-)
--
2.3.4
According to rfc3530 and rfc5661, the max tag length should be
limited by opaque limited.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfs/nfs4xdr.c | 7 -------
include/linux/nfs4.h | 1 +
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5c399ec..1c2ad01 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -67,13 +67,6 @@
static int nfs4_stat_to_errno(int);
-/* NFSv4 COMPOUND tags are only wanted for debugging purposes */
-#ifdef DEBUG
-#define NFS4_MAXTAGLEN 20
-#else
-#define NFS4_MAXTAGLEN 0
-#endif
-
/* lock,open owner id:
* we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT >> 2)
*/
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index ed43cb7..ba54ec2 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -33,6 +33,7 @@ struct nfs4_acl {
};
#define NFS4_MAXLABELLEN 2048
+#define NFS4_MAXTAGLEN NFS4_OPAQUE_LIMIT
struct nfs4_label {
uint32_t lfs;
--
2.3.4
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfs/callback_xdr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 1737c2e..ea6d4de 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -18,8 +18,7 @@
#include "internal.h"
#include "nfs4session.h"
-#define CB_OP_TAGLEN_MAXSZ (512)
-#define CB_OP_HDR_RES_MAXSZ (2 + CB_OP_TAGLEN_MAXSZ)
+#define CB_OP_HDR_RES_MAXSZ (2 + NFS4_MAXTAGLEN)
#define CB_OP_GETATTR_BITMAP_MAXSZ (4)
#define CB_OP_GETATTR_RES_MAXSZ (CB_OP_HDR_RES_MAXSZ + \
CB_OP_GETATTR_BITMAP_MAXSZ + \
@@ -157,7 +156,7 @@ static __be32 decode_compound_hdr_arg(struct xdr_stream *xdr, struct cb_compound
if (unlikely(status != 0))
return status;
/* We do not like overly long tags! */
- if (hdr->taglen > CB_OP_TAGLEN_MAXSZ) {
+ if (hdr->taglen > NFS4_MAXTAGLEN) {
printk("NFS: NFSv4 CALLBACK %s: server sents tag of length %u\n",
__func__, hdr->taglen);
return htonl(NFS4ERR_RESOURCE);
--
2.3.4
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4xdr.c | 2 +-
fs/nfsd/xdr4.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index eff0a94..4db519e 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1808,7 +1808,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
argp->opcnt = be32_to_cpup(p++);
max_reply += 4 + (XDR_QUADLEN(argp->taglen) << 2);
- if (argp->taglen > NFSD4_MAX_TAGLEN)
+ if (argp->taglen > NFS4_MAXTAGLEN)
goto xdr_error;
if (argp->opcnt > 100)
goto xdr_error;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 8bae5d8..613cece 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -40,7 +40,6 @@
#include "state.h"
#include "nfsd.h"
-#define NFSD4_MAX_TAGLEN 128
#define XDR_LEN(n) (((n) + 3) & ~3)
#define CURRENT_STATE_ID_FLAG (1<<0)
--
2.3.4
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/xdr4cb.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
index c47f6fd..008dd2b 100644
--- a/fs/nfsd/xdr4cb.h
+++ b/fs/nfsd/xdr4cb.h
@@ -1,4 +1,5 @@
-#define NFS4_MAXTAGLEN 20
+
+#include <linux/nfs4.h>
#define NFS4_enc_cb_null_sz 0
#define NFS4_dec_cb_null_sz 0
--
2.3.4
On Sat, Mar 28, 2015 at 11:48:21PM +0800, Kinglong Mee wrote:
> According to rfc3530 and rfc5661, the max tag length should be
> limited by opaque limited.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfs/nfs4xdr.c | 7 -------
> include/linux/nfs4.h | 1 +
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5c399ec..1c2ad01 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -67,13 +67,6 @@
>
> static int nfs4_stat_to_errno(int);
>
> -/* NFSv4 COMPOUND tags are only wanted for debugging purposes */
> -#ifdef DEBUG
> -#define NFS4_MAXTAGLEN 20
> -#else
> -#define NFS4_MAXTAGLEN 0
> -#endif
As far as I can tell, DEBUG is never defined, and there's no logic
elsewhere that could actually make use of the tag if DEBUG was defined.
So maybe simplest would be just to remove the DEBUG case and leave
#define NFS4_MAXTAGLEN 0
--b.
> -
> /* lock,open owner id:
> * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT >> 2)
> */
> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> index ed43cb7..ba54ec2 100644
> --- a/include/linux/nfs4.h
> +++ b/include/linux/nfs4.h
> @@ -33,6 +33,7 @@ struct nfs4_acl {
> };
>
> #define NFS4_MAXLABELLEN 2048
> +#define NFS4_MAXTAGLEN NFS4_OPAQUE_LIMIT
>
> struct nfs4_label {
> uint32_t lfs;
> --
> 2.3.4
On Sat, Mar 28, 2015 at 11:46:35PM +0800, Kinglong Mee wrote:
> There are four macro defines for max tag length,
> in fs/nfs/nfs4xdr.c,
> /* NFSv4 COMPOUND tags are only wanted for debugging purposes */
> #ifdef DEBUG
> #define NFS4_MAXTAGLEN 20
> #else
> #define NFS4_MAXTAGLEN 0
> #endif
>
> in fs/nfs/callback_xdr.c,
> #define CB_OP_TAGLEN_MAXSZ (512)
>
> in fs/nfsd/xdr4.h,
> #define NFSD4_MAX_TAGLEN 128
>
> in fs/nfsd/xdr4cb.h,
> #define NFS4_MAXTAGLEN 20
>
> But, according to rfc3530 and rfc5661, all the length should be
> limited by opaque limited.
Neither server nor client really make any use of tags. The client at
least is never going to send a tag. The server does echo back the tag
the client received.
The one arguable bug here is that the spec doesn't appear to forbid the
server returning a non-zero-length tag when the client sent a
zero-length tag. And I don't think the client would handle that?
If so, that might be better handled as a spec bug: if the most popular
client has never handled it then we know that no server's ever done it.
And it'd be annoying server behavior anyway, so, if it's de-facto
forbidden, great.
In short, maybe best to just leave all this alone unless somebody's
actually seen this cause real-world problems....
--b.
>
> The patch site defines a common macro named NFS4_MAXTAGLEN for
> all of them, limited to opaque limited.
>
> Kinglong Mee (4):
> nfs: define NFS4_MAXTAGLEN to OPAQUE limits
> nfs: use NFS4_MAXTAGLEN for cb_taglen checking
> nfsd: use NFS4_MAXTAGLEN for nfsd taglen checking
> nfsd: use NFS4_MAXTAGLEN defined in include/linux/nfs4.h
>
> fs/nfs/callback_xdr.c | 5 ++---
> fs/nfs/nfs4xdr.c | 7 -------
> fs/nfsd/nfs4xdr.c | 2 +-
> fs/nfsd/xdr4.h | 1 -
> fs/nfsd/xdr4cb.h | 3 ++-
> include/linux/nfs4.h | 1 +
> 6 files changed, 6 insertions(+), 13 deletions(-)
>
> --
> 2.3.4
On Apr 29, 2015, at 4:25 PM, J. Bruce Fields <[email protected]> wrote:
> On Sat, Mar 28, 2015 at 11:46:35PM +0800, Kinglong Mee wrote:
>> There are four macro defines for max tag length,
>> in fs/nfs/nfs4xdr.c,
>> /* NFSv4 COMPOUND tags are only wanted for debugging purposes */
>> #ifdef DEBUG
>> #define NFS4_MAXTAGLEN 20
>> #else
>> #define NFS4_MAXTAGLEN 0
>> #endif
>>
>> in fs/nfs/callback_xdr.c,
>> #define CB_OP_TAGLEN_MAXSZ (512)
>>
>> in fs/nfsd/xdr4.h,
>> #define NFSD4_MAX_TAGLEN 128
>>
>> in fs/nfsd/xdr4cb.h,
>> #define NFS4_MAXTAGLEN 20
>>
>> But, according to rfc3530 and rfc5661, all the length should be
>> limited by opaque limited.
>
> Neither server nor client really make any use of tags. The client at
> least is never going
Fwiw, Solaris clients send tags.
> to send a tag. The server does echo back the tag
> the client received.
>
> The one arguable bug here is that the spec doesn't appear to forbid the
> server returning a non-zero-length tag when the client sent a
> zero-length tag. And I don't think the client would handle that?
>
> If so, that might be better handled as a spec bug: if the most popular
> client has never handled it then we know that no server's ever done it.
> And it'd be annoying server behavior anyway, so, if it's de-facto
> forbidden, great.
>
> In short, maybe best to just leave all this alone unless somebody's
> actually seen this cause real-world problems....
>
> --b.
>
>>
>> The patch site defines a common macro named NFS4_MAXTAGLEN for
>> all of them, limited to opaque limited.
>>
>> Kinglong Mee (4):
>> nfs: define NFS4_MAXTAGLEN to OPAQUE limits
>> nfs: use NFS4_MAXTAGLEN for cb_taglen checking
>> nfsd: use NFS4_MAXTAGLEN for nfsd taglen checking
>> nfsd: use NFS4_MAXTAGLEN defined in include/linux/nfs4.h
>>
>> fs/nfs/callback_xdr.c | 5 ++---
>> fs/nfs/nfs4xdr.c | 7 -------
>> fs/nfsd/nfs4xdr.c | 2 +-
>> fs/nfsd/xdr4.h | 1 -
>> fs/nfsd/xdr4cb.h | 3 ++-
>> include/linux/nfs4.h | 1 +
>> 6 files changed, 6 insertions(+), 13 deletions(-)
>>
>> --
>> 2.3.4
> --
> 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
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Wed, Apr 29, 2015 at 4:25 PM, J. Bruce Fields <[email protected]> wrote:
> On Sat, Mar 28, 2015 at 11:46:35PM +0800, Kinglong Mee wrote:
>> There are four macro defines for max tag length,
>> in fs/nfs/nfs4xdr.c,
>> /* NFSv4 COMPOUND tags are only wanted for debugging purposes */
>> #ifdef DEBUG
>> #define NFS4_MAXTAGLEN 20
>> #else
>> #define NFS4_MAXTAGLEN 0
>> #endif
>>
>> in fs/nfs/callback_xdr.c,
>> #define CB_OP_TAGLEN_MAXSZ (512)
>>
>> in fs/nfsd/xdr4.h,
>> #define NFSD4_MAX_TAGLEN 128
>>
>> in fs/nfsd/xdr4cb.h,
>> #define NFS4_MAXTAGLEN 20
>>
>> But, according to rfc3530 and rfc5661, all the length should be
>> limited by opaque limited.
>
> Neither server nor client really make any use of tags. The client at
> least is never going to send a tag. The server does echo back the tag
> the client received.
>
> The one arguable bug here is that the spec doesn't appear to forbid the
> server returning a non-zero-length tag when the client sent a
> zero-length tag. And I don't think the client would handle that?
>
> If so, that might be better handled as a spec bug: if the most popular
> client has never handled it then we know that no server's ever done it.
> And it'd be annoying server behavior anyway, so, if it's de-facto
> forbidden, great.
It is a SHOULD. From RFC7530:
The definition of the "tag" in the request is left to the
implementer. It may be used to summarize the content of the COMPOUND
request for the benefit of packet sniffers and engineers debugging
implementations. However, the value of "tag" in the response SHOULD
be the same value as the value provided in the request. This applies
to the tag field of the CB_COMPOUND procedure as well.
> In short, maybe best to just leave all this alone unless somebody's
> actually seen this cause real-world problems....
ACK. In theory, the CB_COMPOUND issue could arise on the client, but
in practice I think that we add enough padding to avoid it.
Cheers
Trond