diff mbox

nfsd41: compare request's opcnt with session's maxops at nfsd4_sequence

Message ID 20110427024126.GB26541@fieldses.org (mailing list archive)
State New, archived
Headers show

Commit Message

Bruce Fields April 27, 2011, 2:41 a.m. UTC
On Wed, Apr 27, 2011 at 09:09:58AM +0800, Mi Jinlong wrote:
> Make sure nfs server can distinguish request contains more ops
> than channel allowed.

Yes, sequence looks like a reasonable op to catch this error.  The spec
doesn't care as far as I can tell (and it's a buggy-client case, so why
should it), and we already check that any compound not starting with a
sequence has only one op.

> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  fs/nfsd/nfs4state.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fbde6f7..4f9fc68 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	if (!session)
>  		goto out;
>  
> +	status = nfserr_too_many_ops;
> +	if (((struct nfsd4_compoundargs *)rqstp->rq_argp)->opcnt >

Kind of a cumbersome construction, though.

Eh, maybe overkill, but how about this?:

--b.

commit d5eee1629fb9d3a55e5793d156026248c14cb46c
Author: Mi Jinlong <mijinlong@cn.fujitsu.com>
Date:   Wed Apr 27 09:09:58 2011 +0800

    nfsd41: compare request's opcnt with session's maxops at nfsd4_sequence
    
    Make sure nfs server errors out if request contains more ops
    than channel allows.
    
    Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
    [bfields@redhat.com: use helper function]
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

--
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

Comments

Mi Jinlong April 27, 2011, 5:48 a.m. UTC | #1
J. Bruce Fields:
> On Wed, Apr 27, 2011 at 09:09:58AM +0800, Mi Jinlong wrote:
>> Make sure nfs server can distinguish request contains more ops
>> than channel allowed.
> 
> Yes, sequence looks like a reasonable op to catch this error.  The spec
> doesn't care as far as I can tell (and it's a buggy-client case, so why
> should it), and we already check that any compound not starting with a
> sequence has only one op.
> 
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>>  fs/nfsd/nfs4state.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index fbde6f7..4f9fc68 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>  	if (!session)
>>  		goto out;
>>  
>> +	status = nfserr_too_many_ops;
>> +	if (((struct nfsd4_compoundargs *)rqstp->rq_argp)->opcnt >
> 
> Kind of a cumbersome construction, though.
> 
> Eh, maybe overkill, but how about this?:

  That's great!
Bruce Fields May 3, 2011, 3:48 a.m. UTC | #2
On Wed, Apr 27, 2011 at 01:48:09PM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields:
> > On Wed, Apr 27, 2011 at 09:09:58AM +0800, Mi Jinlong wrote:
> >> Make sure nfs server can distinguish request contains more ops
> >> than channel allowed.
> > 
> > Yes, sequence looks like a reasonable op to catch this error.  The spec
> > doesn't care as far as I can tell (and it's a buggy-client case, so why
> > should it), and we already check that any compound not starting with a
> > sequence has only one op.
> > 
> >> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> >> ---
> >>  fs/nfsd/nfs4state.c |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index fbde6f7..4f9fc68 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>  	if (!session)
> >>  		goto out;
> >>  
> >> +	status = nfserr_too_many_ops;
> >> +	if (((struct nfsd4_compoundargs *)rqstp->rq_argp)->opcnt >
> > 
> > Kind of a cumbersome construction, though.
> > 
> > Eh, maybe overkill, but how about this?:
> 
>   That's great!

But note we're making a bunch of the pynfs tests fail now, since they
aren't careful about how many ops they use in a compound.  For example,
LKPP1d fails for me now.  Note it might not fail for you if you put the
pynfs test directory right at the root of the export tree?

I don't think pynfs hsould be using absolute paths for everything.  And
it should probably be doing single-component lookups in a loop instead
of depending on being able to do the whole lookup in one compound,
unless it's willing to adapt the compounds to take into account
maxops--which sounds to me like more trouble than it's worth.

The server could probably also raise its limit, though.

--b.

> 
> -- 
> ----
> thanks
> Mi Jinlong
> 
> > 
> > --b.
> > 
> > commit d5eee1629fb9d3a55e5793d156026248c14cb46c
> > Author: Mi Jinlong <mijinlong@cn.fujitsu.com>
> > Date:   Wed Apr 27 09:09:58 2011 +0800
> > 
> >     nfsd41: compare request's opcnt with session's maxops at nfsd4_sequence
> >     
> >     Make sure nfs server errors out if request contains more ops
> >     than channel allows.
> >     
> >     Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> >     [bfields@redhat.com: use helper function]
> >     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fbde6f7..487ba47 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1721,6 +1721,13 @@ 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)
> > +{
> > +	struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > +
> > +	return args->opcnt > session->se_fchannel.maxops;
> > +}
> > +
> >  __be32
> >  nfsd4_sequence(struct svc_rqst *rqstp,
> >  	       struct nfsd4_compound_state *cstate,
> > @@ -1749,6 +1756,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >  	if (!session)
> >  		goto out;
> >  
> > +	status = nfserr_too_many_ops;
> > +	if (nfsd4_session_too_many_ops(rqstp, session))
> > +		goto out;
> > +
> >  	status = nfserr_badslot;
> >  	if (seq->slotid >= session->se_fchannel.maxreqs)
> >  		goto out;
> > --
> > 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
> > 
> > 
> 
--
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
Mi Jinlong May 9, 2011, 9:26 a.m. UTC | #3
J. Bruce Fields:
> On Wed, Apr 27, 2011 at 01:48:09PM +0800, Mi Jinlong wrote:
>>
>> J. Bruce Fields:
>>> On Wed, Apr 27, 2011 at 09:09:58AM +0800, Mi Jinlong wrote:
>>>> Make sure nfs server can distinguish request contains more ops
>>>> than channel allowed.
>>> Yes, sequence looks like a reasonable op to catch this error.  The spec
>>> doesn't care as far as I can tell (and it's a buggy-client case, so why
>>> should it), and we already check that any compound not starting with a
>>> sequence has only one op.
>>>
>>>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>>> ---
>>>>  fs/nfsd/nfs4state.c |    5 +++++
>>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index fbde6f7..4f9fc68 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>>  	if (!session)
>>>>  		goto out;
>>>>  
>>>> +	status = nfserr_too_many_ops;
>>>> +	if (((struct nfsd4_compoundargs *)rqstp->rq_argp)->opcnt >
>>> Kind of a cumbersome construction, though.
>>>
>>> Eh, maybe overkill, but how about this?:
>>   That's great!
> 
> But note we're making a bunch of the pynfs tests fail now, since they
> aren't careful about how many ops they use in a compound.  For example,
> LKPP1d fails for me now.  Note it might not fail for you if you put the
> pynfs test directory right at the root of the export tree?

  Yes, I don't get LKPP1d fail.

> I don't think pynfs hsould be using absolute paths for everything.  And
> it should probably be doing single-component lookups in a loop instead
> of depending on being able to do the whole lookup in one compound,
> unless it's willing to adapt the compounds to take into account
> maxops--which sounds to me like more trouble than it's worth.

  I agree the second idea although it's more trouble. 
  Because simulating a NFS4.1 client, we should make sure pynfs41 can 
  make request with ops which less than maxops.

> 
> The server could probably also raise its limit, though.

  I think, we can do it as raising the maxops at CREATE_SESSION ops
  when create session, I don't test it.
diff mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fbde6f7..487ba47 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1721,6 +1721,13 @@  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)
+{
+	struct nfsd4_compoundargs *args = rqstp->rq_argp;
+
+	return args->opcnt > session->se_fchannel.maxops;
+}
+
 __be32
 nfsd4_sequence(struct svc_rqst *rqstp,
 	       struct nfsd4_compound_state *cstate,
@@ -1749,6 +1756,10 @@  nfsd4_sequence(struct svc_rqst *rqstp,
 	if (!session)
 		goto out;
 
+	status = nfserr_too_many_ops;
+	if (nfsd4_session_too_many_ops(rqstp, session))
+		goto out;
+
 	status = nfserr_badslot;
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out;