[PATCH 0/4] Define common macro NFS4_MAXTAGLEN for nfs/nfsd

[PATCH 0/4] Define common macro NFS4_MAXTAGLEN for nfs/nfsd

[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

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

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

[Kinglong Mee]

According to rfc3530 and rfc5661, the max tag length should be
limited by opaque limited.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 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

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

[J. Bruce Fields]

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 <kinglongmee@gmail.com>
> ---
>  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

[PATCH 2/4] nfs: use NFS4_MAXTAGLEN for cb_taglen checking

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 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

[PATCH 3/4] nfsd: use NFS4_MAXTAGLEN for taglen checking

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 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

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

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 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

Re: [PATCH 0/4] Define common macro NFS4_MAXTAGLEN for nfs/nfsd

[J. Bruce Fields]

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

Re: [PATCH 0/4] Define common macro NFS4_MAXTAGLEN for nfs/nfsd

[Chuck Lever]

On Apr 29, 2015, at 4:25 PM, J. Bruce Fields <bfields@fieldses.org> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/4] Define common macro NFS4_MAXTAGLEN for nfs/nfsd

[Trond Myklebust]

On Wed, Apr 29, 2015 at 4:25 PM, J. Bruce Fields <bfields@fieldses.org> 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