[PATCH] nfs4: allow server to change forechannel max_ops

[PATCH] nfs4: allow server to change forechannel max_ops

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

Section 18.36.3 of rfc 5661 says that "For the fore channel, the server
MAY change the requested value."

Also, there's no reason why the client would have to care if the server
is willing to accept *more* operations per compound than the client
requested.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfs/nfs4proc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

On the other hand, if the server *decreases* max_ops on the forechannel,
the client may need to do something.  (Probably just fail for now.)  Why
aren't we checking for that case?

--b.

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 071fced..a5a3690 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4880,7 +4880,6 @@ static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
 
 	ret |= _verify_fore_channel_attr(headerpadsz);
 	ret |= _verify_fore_channel_attr(max_resp_sz);
-	ret |= _verify_fore_channel_attr(max_ops);
 
 	ret |= _verify_back_channel_attr(headerpadsz);
 	ret |= _verify_back_channel_attr(max_rqst_sz);
-- 
1.7.0.4

Re: [PATCH] nfs4: allow server to change forechannel max_ops

[Trond Myklebust]

On Tue, 2010-05-25 at 12:42 -0400, J. Bruce Fields wrote: 
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> Section 18.36.3 of rfc 5661 says that "For the fore channel, the server
> MAY change the requested value."
> 
> Also, there's no reason why the client would have to care if the server
> is willing to accept *more* operations per compound than the client
> requested.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  fs/nfs/nfs4proc.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> On the other hand, if the server *decreases* max_ops on the forechannel,
> the client may need to do something.  (Probably just fail for now.)  Why
> aren't we checking for that case?
> 
> --b.
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 071fced..a5a3690 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4880,7 +4880,6 @@ static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
>  
>  	ret |= _verify_fore_channel_attr(headerpadsz);
>  	ret |= _verify_fore_channel_attr(max_resp_sz);
> -	ret |= _verify_fore_channel_attr(max_ops);
>  
>  	ret |= _verify_back_channel_attr(headerpadsz);
>  	ret |= _verify_back_channel_attr(max_rqst_sz);

Yes. That all looks wrong.

Can we please just get rid of that senseless macro, and open code those
checks instead of the above patch? The current code is just pure
obfuscation...

Trond

Re: [PATCH] nfs4: allow server to change forechannel max_ops

[J. Bruce Fields]

On Tue, May 25, 2010 at 12:57:04PM -0400, Trond Myklebust wrote:
> On Tue, 2010-05-25 at 12:42 -0400, J. Bruce Fields wrote: 
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > Section 18.36.3 of rfc 5661 says that "For the fore channel, the server
> > MAY change the requested value."
> > 
> > Also, there's no reason why the client would have to care if the server
> > is willing to accept *more* operations per compound than the client
> > requested.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  fs/nfs/nfs4proc.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > On the other hand, if the server *decreases* max_ops on the forechannel,
> > the client may need to do something.  (Probably just fail for now.)  Why
> > aren't we checking for that case?
> > 
> > --b.
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 071fced..a5a3690 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4880,7 +4880,6 @@ static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
> >  
> >  	ret |= _verify_fore_channel_attr(headerpadsz);
> >  	ret |= _verify_fore_channel_attr(max_resp_sz);
> > -	ret |= _verify_fore_channel_attr(max_ops);
> >  
> >  	ret |= _verify_back_channel_attr(headerpadsz);
> >  	ret |= _verify_back_channel_attr(max_rqst_sz);
> 
> Yes. That all looks wrong.
> 
> Can we please just get rid of that senseless macro, and open code those
> checks instead of the above patch? The current code is just pure
> obfuscation...

Sounds good to me.  I'm hoping Andy can be roped into it....

--b.

Re: [PATCH] nfs4: allow server to change forechannel max_ops

[J. Bruce Fields]

On Tue, May 25, 2010 at 02:32:23PM -0400, J. Bruce Fields wrote:
> On Tue, May 25, 2010 at 12:57:04PM -0400, Trond Myklebust wrote:
> > On Tue, 2010-05-25 at 12:42 -0400, J. Bruce Fields wrote: 
> > > From: J. Bruce Fields <bfields@citi.umich.edu>
> > > 
> > > Section 18.36.3 of rfc 5661 says that "For the fore channel, the server
> > > MAY change the requested value."
> > > 
> > > Also, there's no reason why the client would have to care if the server
> > > is willing to accept *more* operations per compound than the client
> > > requested.
> > > 
> > > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > > ---
> > >  fs/nfs/nfs4proc.c |    1 -
> > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > 
> > > On the other hand, if the server *decreases* max_ops on the forechannel,
> > > the client may need to do something.  (Probably just fail for now.)  Why
> > > aren't we checking for that case?
> > > 
> > > --b.
> > > 
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index 071fced..a5a3690 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -4880,7 +4880,6 @@ static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
> > >  
> > >  	ret |= _verify_fore_channel_attr(headerpadsz);
> > >  	ret |= _verify_fore_channel_attr(max_resp_sz);
> > > -	ret |= _verify_fore_channel_attr(max_ops);
> > >  
> > >  	ret |= _verify_back_channel_attr(headerpadsz);
> > >  	ret |= _verify_back_channel_attr(max_rqst_sz);
> > 
> > Yes. That all looks wrong.
> > 
> > Can we please just get rid of that senseless macro, and open code those
> > checks instead of the above patch? The current code is just pure
> > obfuscation...
> 
> Sounds good to me.  I'm hoping Andy can be roped into it....

Is anyone willing to work on this?

--b.

[PATCH] nfs4: fix channel attribute sanity-checks

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

The sanity checks here are incorrect; in the worst case they allow
values that can later cause client crashes.

They're also over-reliant on the preprocessor.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c |   75 +++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 34 deletions(-)

On Tue, Sep 07, 2010 at 06:32:13PM -0400, J. Bruce Fields wrote:
> On Tue, May 25, 2010 at 02:32:23PM -0400, J. Bruce Fields wrote:
> > On Tue, May 25, 2010 at 12:57:04PM -0400, Trond Myklebust wrote:
> > > Yes. That all looks wrong.
> > > 
> > > Can we please just get rid of that senseless macro, and open code those
> > > checks instead of the above patch? The current code is just pure
> > > obfuscation...
> > 
> > Sounds good to me.  I'm hoping Andy can be roped into it....
> 
> Is anyone willing to work on this?

OK, I cave.  How about something like this?

Minimally tested.  But that goes for the existing code, too....  A pynfs
test or two might not be a bad idea.

--b.

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 089da5b..eed13d6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4914,49 +4914,56 @@ static void nfs4_init_channel_attrs(struct nfs41_create_session_args *args)
 		args->bc_attrs.max_reqs);
 }
 
-static int _verify_channel_attr(char *chan, char *attr_name, u32 sent, u32 rcvd)
+static int nfs4_verify_fore_channel_attrs(struct nfs41_create_session_args *args, struct nfs4_session *session)
 {
-	if (rcvd <= sent)
-		return 0;
-	printk(KERN_WARNING "%s: Session INVALID: %s channel %s increased. "
-		"sent=%u rcvd=%u\n", __func__, chan, attr_name, sent, rcvd);
-	return -EINVAL;
+	struct nfs4_channel_attrs *sent = &args->fc_attrs;
+	struct nfs4_channel_attrs *rcvd = &session->fc_attrs;
+
+	if (rcvd->headerpadsz > sent->headerpadsz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz > sent->max_resp_sz)
+		return -EINVAL;
+	/*
+	 * Our requested max_ops is the minimum we need; we're not
+	 * prepared to break up compounds into smaller pieces than that.
+	 * So, not point even trying to continue if the server won't
+	 * cooperate:
+	 */
+	if (rcvd->max_ops < sent->max_ops)
+		return -EINVAL;
+	if (rcvd->max_reqs == 0)
+		return -EINVAL;
+	return 0;
 }
 
-#define _verify_fore_channel_attr(_name_) \
-	_verify_channel_attr("fore", #_name_, \
-			     args->fc_attrs._name_, \
-			     session->fc_attrs._name_)
+static int nfs4_verify_back_channel_attrs(struct nfs41_create_session_args *args, struct nfs4_session *session)
+{
+	struct nfs4_channel_attrs *sent = &args->bc_attrs;
+	struct nfs4_channel_attrs *rcvd = &session->bc_attrs;
 
-#define _verify_back_channel_attr(_name_) \
-	_verify_channel_attr("back", #_name_, \
-			     args->bc_attrs._name_, \
-			     session->bc_attrs._name_)
+	if (rcvd->max_rqst_sz > sent->max_rqst_sz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz < sent->max_resp_sz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz_cached > sent->max_resp_sz_cached)
+		return -EINVAL;
+	/* These would render the backchannel useless: */
+	if (rcvd->max_ops  == 0)
+		return -EINVAL;
+	if (rcvd->max_reqs == 0)
+		return -EINVAL;
+	return 0;
+}
 
-/*
- * The server is not allowed to increase the fore channel header pad size,
- * maximum response size, or maximum number of operations.
- *
- * The back channel attributes are only negotiatied down: We send what the
- * (back channel) server insists upon.
- */
 static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
 				     struct nfs4_session *session)
 {
-	int ret = 0;
-
-	ret |= _verify_fore_channel_attr(headerpadsz);
-	ret |= _verify_fore_channel_attr(max_resp_sz);
-	ret |= _verify_fore_channel_attr(max_ops);
-
-	ret |= _verify_back_channel_attr(headerpadsz);
-	ret |= _verify_back_channel_attr(max_rqst_sz);
-	ret |= _verify_back_channel_attr(max_resp_sz);
-	ret |= _verify_back_channel_attr(max_resp_sz_cached);
-	ret |= _verify_back_channel_attr(max_ops);
-	ret |= _verify_back_channel_attr(max_reqs);
+	int ret;
 
-	return ret;
+	ret = nfs4_verify_fore_channel_attrs(args, session);
+	if (ret)
+		return ret;
+	return nfs4_verify_back_channel_attrs(args, session);
 }
 
 static int _nfs4_proc_create_session(struct nfs_client *clp)
-- 
1.7.0.4

Re: [PATCH] nfs4: fix channel attribute sanity-checks

[Jim Rees]

J. Bruce Fields wrote:

  +	 * Our requested max_ops is the minimum we need; we're not
  +	 * prepared to break up compounds into smaller pieces than that.
  +	 * So, not point even trying to continue if the server won't
  +	 * cooperate:

"not" should be "no".

Also seems odd that max_ops is actually the minimum, but I guess the code is
clear enough.

[PATCH v2] nfs4: fix channel attribute sanity-checks

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

The sanity checks here are incorrect; in the worst case they allow
values that crash the client.

They're also over-reliant on the preprocessor.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c |   75 +++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 34 deletions(-)

On Sat, Oct 02, 2010 at 05:04:44AM -0400, Jim Rees wrote:
> J. Bruce Fields wrote:
> 
>   +	 * Our requested max_ops is the minimum we need; we're not
>   +	 * prepared to break up compounds into smaller pieces than that.
>   +	 * So, not point even trying to continue if the server won't
>   +	 * cooperate:
> 
> "not" should be "no".

Whoops, fixed.

> Also seems odd that max_ops is actually the minimum, but I guess the code is
> clear enough.

Yeah.  The naming's a little odd, at least; however this field was
imagined being used when the spec was written, I don't think clients are
likely to dynamically adjust their compounds somehow to optimize for the
max_ops that the server gives them.  The linux client behavior seems
more likely: calculate the length of the longest compound the client
needs to be able to send, send that as max_ops, error out if the server
won't give at least much, and ignore the server if it grants more....

--b.

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 089da5b..fdc79e5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4914,49 +4914,56 @@ static void nfs4_init_channel_attrs(struct nfs41_create_session_args *args)
 		args->bc_attrs.max_reqs);
 }
 
-static int _verify_channel_attr(char *chan, char *attr_name, u32 sent, u32 rcvd)
+static int nfs4_verify_fore_channel_attrs(struct nfs41_create_session_args *args, struct nfs4_session *session)
 {
-	if (rcvd <= sent)
-		return 0;
-	printk(KERN_WARNING "%s: Session INVALID: %s channel %s increased. "
-		"sent=%u rcvd=%u\n", __func__, chan, attr_name, sent, rcvd);
-	return -EINVAL;
+	struct nfs4_channel_attrs *sent = &args->fc_attrs;
+	struct nfs4_channel_attrs *rcvd = &session->fc_attrs;
+
+	if (rcvd->headerpadsz > sent->headerpadsz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz > sent->max_resp_sz)
+		return -EINVAL;
+	/*
+	 * Our requested max_ops is the minimum we need; we're not
+	 * prepared to break up compounds into smaller pieces than that.
+	 * So, no point even trying to continue if the server won't
+	 * cooperate:
+	 */
+	if (rcvd->max_ops < sent->max_ops)
+		return -EINVAL;
+	if (rcvd->max_reqs == 0)
+		return -EINVAL;
+	return 0;
 }
 
-#define _verify_fore_channel_attr(_name_) \
-	_verify_channel_attr("fore", #_name_, \
-			     args->fc_attrs._name_, \
-			     session->fc_attrs._name_)
+static int nfs4_verify_back_channel_attrs(struct nfs41_create_session_args *args, struct nfs4_session *session)
+{
+	struct nfs4_channel_attrs *sent = &args->bc_attrs;
+	struct nfs4_channel_attrs *rcvd = &session->bc_attrs;
 
-#define _verify_back_channel_attr(_name_) \
-	_verify_channel_attr("back", #_name_, \
-			     args->bc_attrs._name_, \
-			     session->bc_attrs._name_)
+	if (rcvd->max_rqst_sz > sent->max_rqst_sz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz < sent->max_resp_sz)
+		return -EINVAL;
+	if (rcvd->max_resp_sz_cached > sent->max_resp_sz_cached)
+		return -EINVAL;
+	/* These would render the backchannel useless: */
+	if (rcvd->max_ops  == 0)
+		return -EINVAL;
+	if (rcvd->max_reqs == 0)
+		return -EINVAL;
+	return 0;
+}
 
-/*
- * The server is not allowed to increase the fore channel header pad size,
- * maximum response size, or maximum number of operations.
- *
- * The back channel attributes are only negotiatied down: We send what the
- * (back channel) server insists upon.
- */
 static int nfs4_verify_channel_attrs(struct nfs41_create_session_args *args,
 				     struct nfs4_session *session)
 {
-	int ret = 0;
-
-	ret |= _verify_fore_channel_attr(headerpadsz);
-	ret |= _verify_fore_channel_attr(max_resp_sz);
-	ret |= _verify_fore_channel_attr(max_ops);
-
-	ret |= _verify_back_channel_attr(headerpadsz);
-	ret |= _verify_back_channel_attr(max_rqst_sz);
-	ret |= _verify_back_channel_attr(max_resp_sz);
-	ret |= _verify_back_channel_attr(max_resp_sz_cached);
-	ret |= _verify_back_channel_attr(max_ops);
-	ret |= _verify_back_channel_attr(max_reqs);
+	int ret;
 
-	return ret;
+	ret = nfs4_verify_fore_channel_attrs(args, session);
+	if (ret)
+		return ret;
+	return nfs4_verify_back_channel_attrs(args, session);
 }
 
 static int _nfs4_proc_create_session(struct nfs_client *clp)
-- 
1.7.0.4