Skip to contentSkip to conversation

Define common macro NFS4_MAXTAGLEN for nfs/nfsd

PATCH In review Kinglong Mee 4 participants 9 messages Mar 28, 2015 – Apr 29, 2015

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Kinglong Mee

author
#0 lore raw

cover Define common macro NFS4_MAXTAGLEN for nfs/nfsd

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(-)
Signature · 1 line
2.3.4
author
#1 lore raw

PATCH 1/4 nfs: define NFS4_MAXTAGLEN to OPAQUE limits

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(-)
fs/nfs/nfs4xdr.c +0 -7
index 5c399ec..1c2ad01 100644
@@ -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)
  */
include/linux/nfs4.h +1 -0
index ed43cb7..ba54ec2 100644
@@ -33,6 +33,7 @@ struct nfs4_acl {
 };
 
 #define NFS4_MAXLABELLEN	2048
+#define NFS4_MAXTAGLEN		NFS4_OPAQUE_LIMIT
 
 struct nfs4_label {
 	uint32_t	lfs;
Signature · 1 line
2.3.4
#2 lore raw

Re: [PATCH 1/4] nfs: define NFS4_MAXTAGLEN to OPAQUE limits

Quoted 14 lines · Kinglong Mee

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(-)
fs/nfs/nfs4xdr.c +0 -0
index 5c399ec..1c2ad01 100644

fs/nfs/nfs4xdr.c line 75

Quoted 9 lines
fs/nfs/nfs4xdr.c +0 -6
@@ -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.

Quoted 18 lines
fs/nfs/nfs4xdr.c +0 -1
-
 /* lock,open owner id:
  * we currently use size 2 (u64) out of (NFS4_OPAQUE_LIMIT  >> 2)
  */
include/linux/nfs4.h +1 -0
index ed43cb7..ba54ec2 100644
@@ -33,6 +33,7 @@ struct nfs4_acl {
 };
 
 #define NFS4_MAXLABELLEN	2048
+#define NFS4_MAXTAGLEN		NFS4_OPAQUE_LIMIT
 
 struct nfs4_label {
 	uint32_t	lfs;
Signature · 1 line
2.3.4
author
#3 lore raw

PATCH 2/4 nfs: use NFS4_MAXTAGLEN for cb_taglen checking

In reply to Kinglong Mee
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfs/callback_xdr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
fs/nfs/callback_xdr.c +2 -3
index 1737c2e..ea6d4de 100644
@@ -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);
Signature · 1 line
2.3.4
author
#4 lore raw

PATCH 3/4 nfsd: use NFS4_MAXTAGLEN for taglen checking

In reply to Kinglong Mee
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/nfs4xdr.c | 2 +-
 fs/nfsd/xdr4.h    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +1 -1
index eff0a94..4db519e 100644
@@ -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;
fs/nfsd/xdr4.h +0 -1
index 8bae5d8..613cece 100644
@@ -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)
Signature · 1 line
2.3.4
author
#5 lore raw

PATCH 4/4 nfsd: use NFS4_MAXTAGLEN defined in include/linux/nfs4.h

In reply to Kinglong Mee
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/xdr4cb.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/nfsd/xdr4cb.h +2 -1
index c47f6fd..008dd2b 100644
@@ -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
Signature · 1 line
2.3.4
#6 lore raw
In reply to Kinglong Mee
Quoted 12 lines · Kinglong Mee

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)

Quoted 8 lines

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.

Quoted 20 lines

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(-)
Signature · 1 line
2.3.4
#7 lore raw
Quoted 16 lines · J. Bruce Fields
Quoted 15 lines · Kinglong Mee

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

Quoted 8 lines
Quoted 5 lines

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.

Quoted 35 lines

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.

Quoted 18 lines

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(-)
Quoted 6 lines
Quoted 2 lines
Signature · 1 line
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

#8 lore raw
In reply to J. Bruce Fields
Quoted 26 lines · J. Bruce Fields
Quoted 12 lines · Kinglong Mee

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)

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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