[PATCH 2/2 v2] nfsd41: check the size of request

[PATCH 2/2 v2] nfsd41: check the size of request

[Mi Jinlong]

This patch just check request's size when it consists SEQUENCE.

Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
 fs/nfsd/nfs4state.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e98f3c2..17e30bf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1736,6 +1736,21 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
 	return args->opcnt > session->se_fchannel.maxops;
 }
 
+static int nfsd4_check_request_size(struct nfsd4_compoundargs *args,
+				    struct nfsd4_session *session)
+{
+	struct xdr_buf *xb = &args->rqstp->rq_arg;
+
+	/* Only SEQUENCE operation */
+	if (args->opcnt == 1)
+		return 0;
+
+	if (xb->len > session->se_fchannel.maxreq_sz)
+		return nfserr_req_too_big;
+
+	return 0;
+}
+
 __be32
 nfsd4_sequence(struct svc_rqst *rqstp,
 	       struct nfsd4_compound_state *cstate,
@@ -1804,6 +1819,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	cstate->slot = slot;
 	cstate->session = session;
 
+	status = nfsd4_check_request_size(rqstp->rq_argp, session);
 out:
 	/* Hold a session reference until done processing the compound. */
 	if (cstate->session) {
-- 
1.7.5.4

Re: [PATCH 2/2 v2] nfsd41: check the size of request

[J. Bruce Fields]

On Sat, Jul 02, 2011 at 05:02:54PM +0800, Mi Jinlong wrote:
> This patch just check request's size when it consists SEQUENCE.
> 
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  fs/nfsd/nfs4state.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e98f3c2..17e30bf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1736,6 +1736,21 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
>  	return args->opcnt > session->se_fchannel.maxops;
>  }
>  
> +static int nfsd4_check_request_size(struct nfsd4_compoundargs *args,
> +				    struct nfsd4_session *session)
> +{
> +	struct xdr_buf *xb = &args->rqstp->rq_arg;
> +
> +	/* Only SEQUENCE operation */
> +	if (args->opcnt == 1)
> +		return 0;

Do we need this special check?

Sure, it's possible that a crazy client could set se_fchannel.maxreq_sz
too small, and then we'd get a failure here even when they only sent a
single sequence.  Such a client gets what it deserves.

Seems OK otherwise.

Of course, dealing with the maximum response size is going to be the
difficult part.

--b.

> +
> +	if (xb->len > session->se_fchannel.maxreq_sz)
> +		return nfserr_req_too_big;
> +
> +	return 0;
> +}
> +
>  __be32
>  nfsd4_sequence(struct svc_rqst *rqstp,
>  	       struct nfsd4_compound_state *cstate,
> @@ -1804,6 +1819,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	cstate->slot = slot;
>  	cstate->session = session;
>  
> +	status = nfsd4_check_request_size(rqstp->rq_argp, session);
>  out:
>  	/* Hold a session reference until done processing the compound. */
>  	if (cstate->session) {
> -- 
> 1.7.5.4
> 
> 
> 

Re: [PATCH 2/2 v2] nfsd41: check the size of request

[Mi Jinlong]

J. Bruce Fields 写道:
> On Sat, Jul 02, 2011 at 05:02:54PM +0800, Mi Jinlong wrote:
>> This patch just check request's size when it consists SEQUENCE.
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>>  fs/nfsd/nfs4state.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index e98f3c2..17e30bf 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1736,6 +1736,21 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
>>  	return args->opcnt > session->se_fchannel.maxops;
>>  }
>>  
>> +static int nfsd4_check_request_size(struct nfsd4_compoundargs *args,
>> +				    struct nfsd4_session *session)
>> +{
>> +	struct xdr_buf *xb = &args->rqstp->rq_arg;
>> +
>> +	/* Only SEQUENCE operation */
>> +	if (args->opcnt == 1)
>> +		return 0;
> 
> Do we need this special check?
> 
> Sure, it's possible that a crazy client could set se_fchannel.maxreq_sz
> too small, and then we'd get a failure here even when they only sent a
> single sequence.  Such a client gets what it deserves.

  If kernel err out when crazy client sets maxreq_sz to small, 
  only SEQUENCE operation's size will never exceed maxreq_sz.

  According to RFC5661 18.36.3, if client set maxreq_sz too small, 
  the server SHOULD return NFS4ERR_TOOSMALL in the CREATE_SESSION reply.


thanks,
Mi Jinlong

Re: [PATCH 2/2 v2] nfsd41: check the size of request

[J. Bruce Fields]

On Fri, Jul 08, 2011 at 05:13:44PM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields 写道:
> > On Sat, Jul 02, 2011 at 05:02:54PM +0800, Mi Jinlong wrote:
> >> This patch just check request's size when it consists SEQUENCE.
> >>
> >> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> >> ---
> >>  fs/nfsd/nfs4state.c |   16 ++++++++++++++++
> >>  1 files changed, 16 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index e98f3c2..17e30bf 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1736,6 +1736,21 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
> >>  	return args->opcnt > session->se_fchannel.maxops;
> >>  }
> >>  
> >> +static int nfsd4_check_request_size(struct nfsd4_compoundargs *args,
> >> +				    struct nfsd4_session *session)
> >> +{
> >> +	struct xdr_buf *xb = &args->rqstp->rq_arg;
> >> +
> >> +	/* Only SEQUENCE operation */
> >> +	if (args->opcnt == 1)
> >> +		return 0;
> > 
> > Do we need this special check?
> > 
> > Sure, it's possible that a crazy client could set se_fchannel.maxreq_sz
> > too small, and then we'd get a failure here even when they only sent a
> > single sequence.  Such a client gets what it deserves.
> 
>   If kernel err out when crazy client sets maxreq_sz to small, 
>   only SEQUENCE operation's size will never exceed maxreq_sz.
> 
>   According to RFC5661 18.36.3, if client set maxreq_sz too small, 
>   the server SHOULD return NFS4ERR_TOOSMALL in the CREATE_SESSION reply.

The rpc header size can vary.  If you reject any CREATE_SESSION attempts
with maxreq_sz smaller than

	largest possible rpc header + SEQUENCE

then you risk rejecting some legitimate CREATE_SESSIONs.

But if you allow maxreq_sz's less than that, then it becomes possible
for a client to send a single-SEQUENCE compound that still exceeds
maxreq_sz.

So let's not try to safe every broken client from itself.  Instead,
let's keep things simple, catch those client errors that are easy to
catch, and that we know are errors, and don't worry about the rest.

--b.

[PATCH 2/2 v3] nfsd41: check the size of request

[Mi Jinlong]

This patch checks request's size when it consists SEQUENCE.
Also modifies the format of some function which length exceed 80.

v3:
   remove the check about only SEQUENCE operation.
   check the size immediately after find session.
   modify some function's format which length exceed 80.

Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
 fs/nfsd/nfs4state.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e98f3c2..9cce11c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1645,7 +1645,8 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 	return status;
 }
 
-static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4_sessionid *sid)
+static bool nfsd4_compound_in_session(struct nfsd4_session *session,
+				      struct nfs4_sessionid *sid)
 {
 	if (!session)
 		return 0;
@@ -1695,7 +1696,8 @@ out:
 	return status;
 }
 
-static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt, struct nfsd4_session *s)
+static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt,
+					    struct nfsd4_session *s)
 {
 	struct nfsd4_conn *c;
 
@@ -1707,7 +1709,8 @@ static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt, struct nfsd4_s
 	return NULL;
 }
 
-static void nfsd4_sequence_check_conn(struct nfsd4_conn *new, struct nfsd4_session *ses)
+static void nfsd4_sequence_check_conn(struct nfsd4_conn *new,
+				      struct nfsd4_session *ses)
 {
 	struct nfs4_client *clp = ses->se_client;
 	struct nfsd4_conn *c;
@@ -1729,13 +1732,22 @@ static void nfsd4_sequence_check_conn(struct nfsd4_conn *new, struct nfsd4_sessi
 	return;
 }
 
-static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_session *session)
+static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp,
+				       struct nfsd4_session *session)
 {
 	struct nfsd4_compoundargs *args = rqstp->rq_argp;
 
 	return args->opcnt > session->se_fchannel.maxops;
 }
 
+static bool nfsd4_request_too_big(struct svc_rqst *rqstp,
+				  struct nfsd4_session *session)
+{
+	struct xdr_buf *xb = &rqstp->rq_arg;
+
+	return xb->len > session->se_fchannel.maxreq_sz;
+}
+
 __be32
 nfsd4_sequence(struct svc_rqst *rqstp,
 	       struct nfsd4_compound_state *cstate,
@@ -1768,6 +1780,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (nfsd4_session_too_many_ops(rqstp, session))
 		goto out;
 
+	status = nfserr_req_too_big;
+	if (nfsd4_request_too_big(rqstp, session))
+		goto out;
+
 	status = nfserr_badslot;
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out;
-- 
1.7.5.4

Re: [PATCH 2/2 v3] nfsd41: check the size of request

[J. Bruce Fields]

On Sat, Jul 09, 2011 at 10:19:56AM +0800, Mi Jinlong wrote:
> This patch checks request's size when it consists SEQUENCE.
> Also modifies the format of some function which length exceed 80.
> 
> v3:
>    remove the check about only SEQUENCE operation.
>    check the size immediately after find session.
>    modify some function's format which length exceed 80.

No, please don't ever mix unrelated formatting changes into a patch.

We can leave those lines alone; the occasional long line isn't a big
problem.

--b.

> 
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  fs/nfsd/nfs4state.c |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e98f3c2..9cce11c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1645,7 +1645,8 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
>  	return status;
>  }
>  
> -static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4_sessionid *sid)
> +static bool nfsd4_compound_in_session(struct nfsd4_session *session,
> +				      struct nfs4_sessionid *sid)
>  {
>  	if (!session)
>  		return 0;
> @@ -1695,7 +1696,8 @@ out:
>  	return status;
>  }
>  
> -static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt, struct nfsd4_session *s)
> +static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt,
> +					    struct nfsd4_session *s)
>  {
>  	struct nfsd4_conn *c;
>  
> @@ -1707,7 +1709,8 @@ static struct nfsd4_conn *__nfsd4_find_conn(struct svc_xprt *xpt, struct nfsd4_s
>  	return NULL;
>  }
>  
> -static void nfsd4_sequence_check_conn(struct nfsd4_conn *new, struct nfsd4_session *ses)
> +static void nfsd4_sequence_check_conn(struct nfsd4_conn *new,
> +				      struct nfsd4_session *ses)
>  {
>  	struct nfs4_client *clp = ses->se_client;
>  	struct nfsd4_conn *c;
> @@ -1729,13 +1732,22 @@ static void nfsd4_sequence_check_conn(struct nfsd4_conn *new, struct nfsd4_sessi
>  	return;
>  }
>  
> -static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_session *session)
> +static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp,
> +				       struct nfsd4_session *session)
>  {
>  	struct nfsd4_compoundargs *args = rqstp->rq_argp;
>  
>  	return args->opcnt > session->se_fchannel.maxops;
>  }
>  
> +static bool nfsd4_request_too_big(struct svc_rqst *rqstp,
> +				  struct nfsd4_session *session)
> +{
> +	struct xdr_buf *xb = &rqstp->rq_arg;
> +
> +	return xb->len > session->se_fchannel.maxreq_sz;
> +}
> +
>  __be32
>  nfsd4_sequence(struct svc_rqst *rqstp,
>  	       struct nfsd4_compound_state *cstate,
> @@ -1768,6 +1780,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	if (nfsd4_session_too_many_ops(rqstp, session))
>  		goto out;
>  
> +	status = nfserr_req_too_big;
> +	if (nfsd4_request_too_big(rqstp, session))
> +		goto out;
> +
>  	status = nfserr_badslot;
>  	if (seq->slotid >= session->se_fchannel.maxreqs)
>  		goto out;
> -- 
> 1.7.5.4
> 
> 
> 

[PATCH 2/2 v4] nfsd41: check the size of request

[Mi Jinlong]

This patch checks request's size when it consists SEQUENCE.

v4:
   delete format fixing which length exceed 80

v3:
   remove the check about only SEQUENCE operation.
   check the size immediately after find session.
   modify some function's format which length exceed 80.

Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
 fs/nfsd/nfs4state.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 72899ec..cc20587 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1763,6 +1763,14 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
 	return args->opcnt > session->se_fchannel.maxops;
 }
 
+static bool nfsd4_request_too_big(struct svc_rqst *rqstp,
+				  struct nfsd4_session *session)
+{
+	struct xdr_buf *xb = &rqstp->rq_arg;
+
+	return xb->len > session->se_fchannel.maxreq_sz;
+}
+
 __be32
 nfsd4_sequence(struct svc_rqst *rqstp,
 	       struct nfsd4_compound_state *cstate,
@@ -1795,6 +1803,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (nfsd4_session_too_many_ops(rqstp, session))
 		goto out;
 
+	status = nfserr_req_too_big;
+	if (nfsd4_request_too_big(rqstp, session))
+		goto out;
+
 	status = nfserr_badslot;
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out;
-- 
1.7.5.4

Re: [PATCH 2/2 v4] nfsd41: check the size of request

[J. Bruce Fields]

On Thu, Jul 14, 2011 at 02:56:02PM +0800, Mi Jinlong wrote:
> This patch checks request's size when it consists SEQUENCE.
> 
> v4:
>    delete format fixing which length exceed 80
> 
> v3:
>    remove the check about only SEQUENCE operation.
>    check the size immediately after find session.
>    modify some function's format which length exceed 80.

Both applied, thanks!

--b.

> 
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  fs/nfsd/nfs4state.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 72899ec..cc20587 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1763,6 +1763,14 @@ static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_sess
>  	return args->opcnt > session->se_fchannel.maxops;
>  }
>  
> +static bool nfsd4_request_too_big(struct svc_rqst *rqstp,
> +				  struct nfsd4_session *session)
> +{
> +	struct xdr_buf *xb = &rqstp->rq_arg;
> +
> +	return xb->len > session->se_fchannel.maxreq_sz;
> +}
> +
>  __be32
>  nfsd4_sequence(struct svc_rqst *rqstp,
>  	       struct nfsd4_compound_state *cstate,
> @@ -1795,6 +1803,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	if (nfsd4_session_too_many_ops(rqstp, session))
>  		goto out;
>  
> +	status = nfserr_req_too_big;
> +	if (nfsd4_request_too_big(rqstp, session))
> +		goto out;
> +
>  	status = nfserr_badslot;
>  	if (seq->slotid >= session->se_fchannel.maxreqs)
>  		goto out;
> -- 
> 1.7.5.4
> 
> 
>