nfsd: some updates and cleanups

Layout is a state resource, nfsd should check it too.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +1 -0

index 61dfb33..e0a4556 100644
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
 	 * Also note we should probably be using this in 4.0 case too.
 	 */
 	return !list_empty(&clp->cl_openowners)
+		|| !list_empty(&clp->cl_lo_states)
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions);
 }

fs/nfsd/nfs4state.c +1 -0

+	    && clp->cl_cb_state != NFSD4_CB_UP)
 		goto out;
 	status = nfs_ok;
 out:

Signature · 1 line

2.4.3

Quoted 20 lines · Kinglong Mee

Layout is a state resource, nfsd should check it too.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +1 -0

index 61dfb33..e0a4556 100644
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
 	 * Also note we should probably be using this in 4.0 case too.
 	 */
 	return !list_empty(&clp->cl_openowners)
+		|| !list_empty(&clp->cl_lo_states)
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions);
 }

fs/nfsd/nfs4state.c nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, · line 4262

Quoted 7 lines

fs/nfsd/nfs4state.c +2 -2

@@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	clp = cstate->clp;
 	status = nfserr_cb_path_down;
-	if (!list_empty(&clp->cl_delegations)
-			&& clp->cl_cb_state != NFSD4_CB_UP)
+	if ((!list_empty(&clp->cl_delegations)
+	     || !list_empty(&clp->cl_lo_states))

nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.

That said, it's harmless--why don't we just call client_has_state() here instead?

Quoted 31 lines · J. Bruce Fields

Quoted 20 lines · Kinglong Mee

Layout is a state resource, nfsd should check it too.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +1 -0

index 61dfb33..e0a4556 100644
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
 	 * Also note we should probably be using this in 4.0 case too.
 	 */
 	return !list_empty(&clp->cl_openowners)
+		|| !list_empty(&clp->cl_lo_states)
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions);
 }

fs/nfsd/nfs4state.c nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, · line 4262

Quoted 7 lines

fs/nfsd/nfs4state.c +2 -2

@@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	clp = cstate->clp;
 	status = nfserr_cb_path_down;
-	if (!list_empty(&clp->cl_delegations)
-			&& clp->cl_cb_state != NFSD4_CB_UP)
+	if ((!list_empty(&clp->cl_delegations)
+	     || !list_empty(&clp->cl_lo_states))

nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.

Quoted 2 lines

That said, it's harmless--why don't we just call client_has_state() here instead?

I don't think so.

NFSv4.0's callback path is used for delegation recalling only, without delegation, callback path is useless.

Also, describing in rfc3530,

When the client holds delegations, it needs to use RENEW to detect when the server has determined that the callback path is down. When the server has made such a determination, only the RENEW operation will renew the lease on delegations. If the server determines the callback path is down, it returns NFS4ERR_CB_PATH_DOWN. Even though it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on the record locks and share reservations that the client has established on the server.

I will drop the second modify in version 2.

thanks,
Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

Quoted 2 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

This means the statement in that earlier commit is wrong:

With this, there's no need to keep two counters as they'd always be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the possibibility of returning the same confirm twice for a one clientid?

--b.

Quoted 29 lines

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

Quoted 4 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

Quoted 8 lines

This means the statement in that earlier commit is wrong:

With this, there's no need to keep two counters as they'd always be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the possibibility of returning the same confirm twice for a one clientid?

(but frankly I can never completely review changes to the setclientid/setclientid_confirm behavior without rereading RFC 7530 16.33.5 every time, which is a slog. Might help to contrive a pynfs test derived from that text which tests for this particular behavior.)

Quoted 32 lines

--b.

Quoted 29 lines

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

Quoted 8 lines

Quoted 8 lines

This means the statement in that earlier commit is wrong:

With this, there's no need to keep two counters as they'd always be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the possibibility of returning the same confirm twice for a one clientid?

Yes, nfsd will generate same confirm for one clientid in one second.

verf[0] = (__force __be32)jiffies;
verf[1] = (__force __be32)nn->clientid_counter;

for case 1: probable callback update, the new unconf client needs a different confirm.

Rereading rfc7530,
x be the value of the client.id subfield of the SETCLIENTID4args
structure.

v be the value of the client.verifier subfield of the
SETCLIENTID4args structure.

c be the value of the client ID field returned in the
SETCLIENTID4resok structure.

k represent the value combination of the callback and callback_ident
fields of the SETCLIENTID4args structure.

s be the setclientid_confirm value returned in the SETCLIENTID4resok
structure.

{ v, x, c, k, s } be a quintuple for a client record. A client
record is confirmed if there has been a SETCLIENTID_CONFIRM operation to confirm it. Otherwise, it is unconfirmed. An unconfirmed record is established by a SETCLIENTID call.

... /* case 1: probable callback update */ ...

o The server checks if it has recorded a confirmed record for { v, x, c, l, s }, where l may or may not equal k. If so, and since the id verifier v of the request matches that which is confirmed and recorded, the server treats this as a probable callback information update and records an unconfirmed { v, x, c, k, t } and leaves the confirmed { v, x, c, l, s } in place, such that t != s. It does not matter whether k equals l or not. Any pre-existing unconfirmed { v, x, c, *, * } is removed.

The server returns { c, t }. It is indeed returning the old clientid4 value c, because the client apparently only wants to update callback value k to value l. It's possible this request is one from the Byzantine router that has stale callback information, but this is not a problem. The callback information update is only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.

The server awaits confirmation of k via SETCLIENTID_CONFIRM { c, t }.

The server does NOT remove client (lock/share/delegation) state for x.

Quoted 6 lines

(but frankly I can never completely review changes to the setclientid/setclientid_confirm behavior without rereading RFC 7530 16.33.5 every time, which is a slog. Might help to contrive a pynfs test derived from that text which tests for this particular behavior.)

Make sense.
I will make it later.

thanks,
Kinglong Mee

Quoted 33 lines

Quoted 32 lines

--b.

Quoted 29 lines

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

Quoted 20 lines · Kinglong Mee

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

Quoted 8 lines

Quoted 8 lines

This means the statement in that earlier commit is wrong:

With this, there's no need to keep two counters as they'd always be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the possibibility of returning the same confirm twice for a one clientid?

Yes, nfsd will generate same confirm for one clientid in one second.

verf[0] = (__force __be32)jiffies;
verf[1] = (__force __be32)nn->clientid_counter;

Quoted 2 lines

for case 1: probable callback update, the new unconf client needs a different confirm.

Ignore this patch, and just revert commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" is a better solve.

thanks,
Kinglong Mee

Quoted 94 lines

Rereading rfc7530,
x be the value of the client.id subfield of the SETCLIENTID4args
structure.

v be the value of the client.verifier subfield of the
SETCLIENTID4args structure.

c be the value of the client ID field returned in the
SETCLIENTID4resok structure.

k represent the value combination of the callback and callback_ident
fields of the SETCLIENTID4args structure.

s be the setclientid_confirm value returned in the SETCLIENTID4resok
structure.

{ v, x, c, k, s } be a quintuple for a client record. A client
record is confirmed if there has been a SETCLIENTID_CONFIRM operation to confirm it. Otherwise, it is unconfirmed. An unconfirmed record is established by a SETCLIENTID call.

... /* case 1: probable callback update */ ...

o The server checks if it has recorded a confirmed record for { v, x, c, l, s }, where l may or may not equal k. If so, and since the id verifier v of the request matches that which is confirmed and recorded, the server treats this as a probable callback information update and records an unconfirmed { v, x, c, k, t } and leaves the confirmed { v, x, c, l, s } in place, such that t != s. It does not matter whether k equals l or not. Any pre-existing unconfirmed { v, x, c, *, * } is removed.

The server returns { c, t }. It is indeed returning the old clientid4 value c, because the client apparently only wants to update callback value k to value l. It's possible this request is one from the Byzantine router that has stale callback information, but this is not a problem. The callback information update is only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.

The server awaits confirmation of k via SETCLIENTID_CONFIRM { c, t }.

The server does NOT remove client (lock/share/delegation) state for x.

Quoted 6 lines

(but frankly I can never completely review changes to the setclientid/setclientid_confirm behavior without rereading RFC 7530 16.33.5 every time, which is a slog. Might help to contrive a pynfs test derived from that text which tests for this particular behavior.)

Make sense.
I will make it later.

thanks,
Kinglong Mee

Quoted 33 lines

Quoted 32 lines

--b.

Quoted 29 lines

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

Quoted 21 lines · Kinglong Mee

Quoted 20 lines · Kinglong Mee

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

Quoted 8 lines

Quoted 8 lines

This means the statement in that earlier commit is wrong:

With this, there's no need to keep two counters as they'd always be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the possibibility of returning the same confirm twice for a one clientid?

Yes, nfsd will generate same confirm for one clientid in one second.

verf[0] = (__force __be32)jiffies;
verf[1] = (__force __be32)nn->clientid_counter;

Quoted 6 lines

Quoted 2 lines

for case 1: probable callback update, the new unconf client needs a different confirm.

Ignore this patch, and just revert commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" is a better solve.

We can't revert that completely, it does fix a real locking bug at least, I think.

I'd agree to reinstating a separate counter for the verifier. That verifier probably also needs to be per-network namespace to make the per-network-namespace locking correct.

--b.

Quoted 98 lines

thanks,
Kinglong Mee

Quoted 94 lines

Rereading rfc7530,
x be the value of the client.id subfield of the SETCLIENTID4args
structure.

v be the value of the client.verifier subfield of the
SETCLIENTID4args structure.

c be the value of the client ID field returned in the
SETCLIENTID4resok structure.

k represent the value combination of the callback and callback_ident
fields of the SETCLIENTID4args structure.

s be the setclientid_confirm value returned in the SETCLIENTID4resok
structure.

{ v, x, c, k, s } be a quintuple for a client record. A client
record is confirmed if there has been a SETCLIENTID_CONFIRM operation to confirm it. Otherwise, it is unconfirmed. An unconfirmed record is established by a SETCLIENTID call.

... /* case 1: probable callback update */ ...

o The server checks if it has recorded a confirmed record for { v, x, c, l, s }, where l may or may not equal k. If so, and since the id verifier v of the request matches that which is confirmed and recorded, the server treats this as a probable callback information update and records an unconfirmed { v, x, c, k, t } and leaves the confirmed { v, x, c, l, s } in place, such that t != s. It does not matter whether k equals l or not. Any pre-existing unconfirmed { v, x, c, *, * } is removed.

The server returns { c, t }. It is indeed returning the old clientid4 value c, because the client apparently only wants to update callback value k to value l. It's possible this request is one from the Byzantine router that has stale callback information, but this is not a problem. The callback information update is only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.

The server awaits confirmation of k via SETCLIENTID_CONFIRM { c, t }.

The server does NOT remove client (lock/share/delegation) state for x.

Quoted 6 lines

(but frankly I can never completely review changes to the setclientid/setclientid_confirm behavior without rereading RFC 7530 16.33.5 every time, which is a slog. Might help to contrive a pynfs test derived from that text which tests for this particular behavior.)

Make sense.
I will make it later.

thanks,
Kinglong Mee

Quoted 33 lines

Quoted 32 lines

--b.

Quoted 29 lines

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

Signature · 1 line

2.4.3

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

If using clientid_counter, gen_confirm will generate same verifier in one second.

A new counter for client confirm verifier make sure gen_confirm generating different each calling for the same clientid.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/netns.h     | 1 +
 fs/nfsd/nfs4state.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

fs/nfsd/netns.h +1 -0

index ea6749a..d8b16c2 100644
@@ -110,6 +110,7 @@ struct nfsd_net {
 	unsigned int max_connections;
 
 	u32 clientid_counter;
+	u32 clverifier_counter;
 
 	struct svc_serv *nfsd_serv;
 };

fs/nfsd/nfs4state.c +0 -0

index 61dfb33..5a64757e 100644

Quoted 31 lines · On Sat, 18 Jul 2015 07:33:31 +0800 Kinglong Mee <[email protected]> wrote:

If using clientid_counter, gen_confirm will generate same verifier in one second.

A new counter for client confirm verifier make sure gen_confirm generating different each calling for the same clientid.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/netns.h     | 1 +
 fs/nfsd/nfs4state.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

fs/nfsd/netns.h +1 -0

index ea6749a..d8b16c2 100644
@@ -110,6 +110,7 @@ struct nfsd_net {
 	unsigned int max_connections;
 
 	u32 clientid_counter;
+	u32 clverifier_counter;
 
 	struct svc_serv *nfsd_serv;
 };

fs/nfsd/nfs4state.c +0 -0

index 61dfb33..5a64757e 100644
@@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
 	 * __force to keep sparse happy
 	 */

fs/nfsd/nfs4state.c static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn) · line 1900

Quoted 6 lines

fs/nfsd/nfs4state.c +1 -1

 	verf[0] = (__force __be32)get_seconds();
-	verf[1] = (__force __be32)nn->clientid_counter;
+	verf[1] = (__force __be32)nn->clverifier_counter++;
 	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
 }
 

Reviewed-by Jeff Layton <[email protected]>

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +3 -2

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

This one needs be applied with
"nfsd: New counter for generating client confirm verifier" that is only update client confirm verifier without fix this problem.

thanks,
Kinglong Mee

Quoted 20 lines · Kinglong Mee

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock" have moved gen_confirm() to gen_clid().

After it, setclientid will return a bad reply with all zero confirms after copy_clid().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/nfsd/nfs4state.c +0 -0

index e0a4556..b1f84fc 100644
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);

fs/nfsd/nfs4state.c nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, · line 3052

Quoted 10 lines

fs/nfsd/nfs4state.c +3 -2

-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);

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

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

fs/nfsd/nfs4state.c +7 -3

index b1f84fc..e5e14fa 100644
@@ -3316,8 +3316,10 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 	if (ret == NULL) {
 		hash_openowner(oo, clp, strhashval);
 		ret = oo;
-	} else
+	} else {
+		kfree(oo->oo_owner.so_owner.data);
 		nfs4_free_openowner(&oo->oo_owner);
+	}
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }

@@ -5217,8 +5219,10 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 		list_add(&lo->lo_owner.so_strhash,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);
 		ret = lo;
-	} else
-		nfs4_free_lockowner(&lo->lo_owner);
+	} else {
+		kfree(lo->lo_owner.so_owner.data);
+		nfs4_free_openowner(&lo->lo_owner);
+	}
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }

Signature · 1 line

2.4.3

v2, new helper nfs4_free_stateowner for freeing so_owner.data and sop

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

fs/nfsd/nfs4state.c +11 -4

index 00a0f8f..3c24c72 100644
@@ -990,6 +990,12 @@ release_all_access(struct nfs4_ol_stateid *stp)
 	}
 }
 
+static inline void nfs4_free_stateowner(struct nfs4_stateowner *sop)
+{
+	kfree(sop->so_owner.data);
+	sop->so_ops->so_free(sop);
+}
+
 static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
 {
 	struct nfs4_client *clp = sop->so_client;

@@ -1000,8 +1006,7 @@ static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
 		return;
 	sop->so_ops->so_unhash(sop);
 	spin_unlock(&clp->cl_lock);
-	kfree(sop->so_owner.data);
-	sop->so_ops->so_free(sop);
+	nfs4_free_stateowner(sop);
 }
 
 static void unhash_ol_stateid(struct nfs4_ol_stateid *stp)

@@ -3316,7 +3321,8 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 		hash_openowner(oo, clp, strhashval);
 		ret = oo;
 	} else
-		nfs4_free_openowner(&oo->oo_owner);
+		nfs4_free_stateowner(&oo->oo_owner);
+
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }

@@ -5216,7 +5222,8 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);
 		ret = lo;
 	} else
-		nfs4_free_lockowner(&lo->lo_owner);
+		nfs4_free_stateowner(&lo->lo_owner);
+
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }

Signature · 1 line

2.4.3

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

fs/nfsd/nfs4state.c +1 -0

index e5e14fa..998166d 100644
@@ -3938,6 +3938,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	if (!filp) {
 		/* We should always have a readable file here */
 		WARN_ON_ONCE(1);
+		locks_free_lock(fl);
 		return -EBADF;
 	}
 	fl->fl_file = filp;

Signature · 1 line

2.4.3

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4layouts.c | 5 ++---
 fs/nfsd/nfs4state.c   | 2 +-
 fs/nfsd/state.h       | 1 +
 3 files changed, 4 insertions(+), 4 deletions(-)

fs/nfsd/nfs4layouts.c +2 -3

index 6904213..5c0f1d6 100644
@@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 			goto out;
 	} else {
 		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
-
-		status = nfserr_bad_stateid;
-		if (stateid->si_generation > stid->sc_stateid.si_generation)
+		status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
+		if (status)
 			goto out_put_stid;
 		if (layout_type != ls->ls_layout_type)
 			goto out_put_stid;

fs/nfsd/nfs4state.c +1 -1

index 998166d..2edfedc 100644
@@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 	return (s32)(a->si_generation - b->si_generation) > 0;
 }
 
-static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
 {
 	/*
 	 * When sessions are used the stateid generation number is ignored

fs/nfsd/state.h +1 -0

index 4874ce5..18d015d 100644
@@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
 struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
 		struct kmem_cache *slab);
 void nfs4_unhash_stid(struct nfs4_stid *s);

Signature · 1 line

2.4.3

I don't think layout stateid handling is the same as handling of other stateid's. See

	https://tools.ietf.org/html/rfc5661#section-12.5.3

I haven't reviewed it in detail yet.

--b.

Quoted 50 lines · Kinglong Mee

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4layouts.c | 5 ++---
 fs/nfsd/nfs4state.c   | 2 +-
 fs/nfsd/state.h       | 1 +
 3 files changed, 4 insertions(+), 4 deletions(-)

fs/nfsd/nfs4layouts.c +2 -3

index 6904213..5c0f1d6 100644
@@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 			goto out;
 	} else {
 		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
-
-		status = nfserr_bad_stateid;
-		if (stateid->si_generation > stid->sc_stateid.si_generation)
+		status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
+		if (status)
 			goto out_put_stid;
 		if (layout_type != ls->ls_layout_type)
 			goto out_put_stid;

fs/nfsd/nfs4state.c +1 -1

index 998166d..2edfedc 100644
@@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 	return (s32)(a->si_generation - b->si_generation) > 0;
 }
 
-static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
 {
 	/*
 	 * When sessions are used the stateid generation number is ignored

fs/nfsd/state.h +1 -0

index 4874ce5..18d015d 100644
@@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
 struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
 		struct kmem_cache *slab);
 void nfs4_unhash_stid(struct nfs4_stid *s);

Signature · 1 line

2.4.3

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4layouts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nfsd/nfs4layouts.c +1 -1

index 5c0f1d6..c5af4ccb 100644
@@ -162,7 +162,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
 	fl = locks_alloc_lock();
 	if (!fl)
 		return -ENOMEM;
-	locks_init_lock(fl);
+
 	fl->fl_lmops = &nfsd4_layouts_lm_ops;
 	fl->fl_flags = FL_LAYOUT;
 	fl->fl_type = F_RDLCK;

Signature · 1 line

2.4.3

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4state.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

fs/nfsd/nfs4state.c +6 -10

index 2704270..b6c134d 100644
@@ -5131,8 +5131,7 @@ nevermind:
 }
 
 static struct nfs4_lockowner *
-find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfs4_client *clp)
+find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
 	unsigned int strhashval = ownerstr_hashval(owner);
 	struct nfs4_stateowner *so;

@@ -5150,13 +5149,12 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
 }
 
 static struct nfs4_lockowner *
-find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfs4_client *clp)
+find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
 	struct nfs4_lockowner *lo;
 
 	spin_lock(&clp->cl_lock);
-	lo = find_lockowner_str_locked(clid, owner, clp);
+	lo = find_lockowner_str_locked(clp, owner);
 	spin_unlock(&clp->cl_lock);
 	return lo;
 }

@@ -5200,8 +5198,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 	lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
 	lo->lo_owner.so_ops = &lockowner_ops;
 	spin_lock(&clp->cl_lock);
-	ret = find_lockowner_str_locked(&clp->cl_clientid,
-			&lock->lk_new_owner, clp);
+	ret = find_lockowner_str_locked(clp, &lock->lk_new_owner);
 	if (ret == NULL) {
 		list_add(&lo->lo_owner.so_strhash,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);

@@ -5322,7 +5319,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct nfs4_lockowner *lo;
 	unsigned int strhashval;
 
-	lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
+	lo = find_lockowner_str(cl, &lock->lk_new_owner);
 	if (!lo) {
 		strhashval = ownerstr_hashval(&lock->lk_new_owner);
 		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);

@@ -5597,8 +5594,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
-				cstate->clp);
+	lo = find_lockowner_str(cstate->clp, &lockt->lt_owner);
 	if (lo)
 		file_lock->fl_owner = (fl_owner_t)lo;
 	file_lock->fl_pid = current->tgid;

Signature · 1 line

2.4.3