Skip to contentSkip to conversation

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

PATCH In review Mi Jinlong 2 participants 5 messages Apr 27, 2011 – May 9, 2011

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

Mi Jinlong

author
#0 lore raw

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

Make sure nfs server can distinguish request contains more ops than channel allowed.

Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4state.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/nfsd/nfs4state.c +5 -0
index fbde6f7..4f9fc68 100644
@@ -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 >
+				 session->se_fchannel.maxops)
+		goto out;
+
 	status = nfserr_badslot;
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out;
Signature · 1 line
1.7.4.1
#1 lore raw
Quoted 2 lines · Mi Jinlong

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.

Quoted 10 lines
Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4state.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/nfsd/nfs4state.c +0 -0
index fbde6f7..4f9fc68 100644

fs/nfsd/nfs4state.c nfsd4_sequence(struct svc_rqst *rqstp, · line 1753

Quoted 5 lines
fs/nfsd/nfs4state.c +2 -0
@@ -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 <[email protected]>
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 <[email protected]>
[[email protected]: use helper function]
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4state.c +11 -0
index fbde6f7..487ba47 100644
@@ -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;
author
#2 lore raw

J. Bruce Fields:

Quoted 22 lines · Mi Jinlong
Quoted 2 lines

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.

Quoted 13 lines
Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4state.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/nfsd/nfs4state.c +0 -0
index fbde6f7..4f9fc68 100644
@@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (!session)
 		goto out;
Quoted 6 lines

fs/nfsd/nfs4state.c nfsd4_sequence(struct svc_rqst *rqstp, · line 1753

Quoted 2 lines
fs/nfsd/nfs4state.c +2 -0
+	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!

Signature · 3 lines
----
thanks
Mi Jinlong
Quoted 51 lines

--b.

commit d5eee1629fb9d3a55e5793d156026248c14cb46c

Author: Mi Jinlong <[email protected]>
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 <[email protected]>
[[email protected]: use helper function]
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4state.c +11 -1
index fbde6f7..487ba47 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 25 lines · Mi Jinlong

J. Bruce Fields:

Quoted 22 lines · Mi Jinlong
Quoted 2 lines

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.

Quoted 13 lines
Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4state.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/nfsd/nfs4state.c +0 -0
index fbde6f7..4f9fc68 100644
@@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (!session)
 		goto out;
Quoted 8 lines
Quoted 6 lines

fs/nfsd/nfs4state.c nfsd4_sequence(struct svc_rqst *rqstp, · line 1753

Quoted 2 lines
fs/nfsd/nfs4state.c +2 -0
+	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.

Quoted 58 lines
Signature · 4 lines
-- 
----
thanks
Mi Jinlong
Quoted 51 lines

--b.

commit d5eee1629fb9d3a55e5793d156026248c14cb46c

Author: Mi Jinlong <[email protected]>
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 <[email protected]>
[[email protected]: use helper function]
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4state.c +11 -1
index fbde6f7..487ba47 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw

J. Bruce Fields:

Quoted 28 lines · Mi Jinlong
Quoted 27 lines

J. Bruce Fields:

Quoted 25 lines · Mi Jinlong
Quoted 2 lines

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.

Quoted 10 lines
Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4state.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/nfsd/nfs4state.c +0 -0
index fbde6f7..4f9fc68 100644

fs/nfsd/nfs4state.c nfsd4_sequence(struct svc_rqst *rqstp, · line 1753

Quoted 5 lines
fs/nfsd/nfs4state.c +2 -0
@@ -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.

Quoted 7 lines
Quoted 2 lines
Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Signature · 3 lines
----
thanks
Mi Jinlong
Quoted 54 lines
Quoted 35 lines
Quoted 35 lines

commit d5eee1629fb9d3a55e5793d156026248c14cb46c

Author: Mi Jinlong <[email protected]>
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 <[email protected]>
[[email protected]: use helper function]
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4state.c +7 -0
index fbde6f7..487ba47 100644
@@ -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;
Quoted 13 lines
Quoted 8 lines
fs/nfsd/nfs4state.c +4 -1
+	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;
--
Quoted 5 lines

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

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